Remove EOF Check

[timway]

SUMMARY

• Cisco SCP server will close the channel after retreiving a file and calling ssh_scp_pull_request will return -1 and raise an exception aborting get

Fixes #785

ISSUE TYPE

• Bugfix Pull Request

[packit-as-a-service]

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

• sudo yum install -y dnf-plugins-core on RHEL 8
• sudo dnf install -y dnf-plugins-core on Fedora
• dnf copr enable packit/ansible-pylibssh-786
• And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

[webknjaz]

This would also require a test.

[webknjaz]

Instead of using a TODO-list style imperative mood, use the writing style that explains the effect for the end-users compared to the previous release as if this is already included in a release. Sometimes, using the past tense helps.

[webknjaz]

Here's where you can see the surrounding writing style to adjust your release note: https://ansible-pylibssh--786.org.readthedocs.build/en/786/changelog/#changelog.

[webknjaz]

Avoid periods right before the em dash.

Suggested change

	get files from Cisco SCP servers. -- by :user:`timway`.
	get files from Cisco SCP servers -- by :user:`timway`.

[webknjaz]

cc @Jakuje could you check this out?

[Jakuje]

Given that the CI with OpenSSH is happy about this, I think it is ok.

Out of the curiosity, do you have more information what is the libssh returning with the cisco case on the line you removed? From my reading of the code, its something I would expect it should work, but I assume the timing or order of the messages is different from the OpenSSH case.

[timway]

Thanks @Jakuje and @webknjaz ! I'll work on rewording and linking of the changelog. I also looked at trying to get a test case that closes the channel after the bytes are read out but I haven't landed on something that works.

Lastly we could keep the EOF check but gate it behind a check on the channel itself. I can look at that as well. In the linked issue I copied in the -vvvv output from the Cisco case. It does send an EOF and then closes the channel. I'm assuming it's a race condition with my older Cisco switches that work intermittently but fail 100% of the time on newer IOS-XE models. I'm thinking the EOF and channel close messages are handled out of band of the get operation and complete prior to reaching the check on the channel for an EOF. In the Cisco case the rc of the EOF check in the Cisco case is -1 and does not match the EOF value so it fails and asserts.

[Jakuje]

Lastly we could keep the EOF check but gate it behind a check on the channel itself

If this is possible, I think this would be preferred, if it could work. Thank you for digging into this!

[packit-as-a-service]

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

• sudo dnf install -y 'dnf*-command(copr)'
• dnf copr enable packit/ansible-pylibssh-786
• And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

[Jakuje]

As commented on https://gitlab.com/libssh/libssh-mirror/-/issues/345, I think this is the best we can do now. We will try to solve this in libssh, but it will take some time and given that this part is mostly sanity check that there are no other pending data, I think it can be safely dropped. Given that we do not have easy way to test it against problematic server, I think we can live without additional tests.
Not sure whats up with the CI though. @webknjaz do you have some idea?

[webknjaz]

@Jakuje could you help rephrase this so it'd be mergable? We need something for the end-users to understand how they are going to be affected and when.

[Jakuje]

Would something like this work for you?

Removed non-mandatory EOF check after the SCP download procedure, which was causing race conditions with some Cisco devices.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.62%. Comparing base (27ce4de) to head (8051bcd).
⚠️ Report is 117 commits behind head on devel.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.