Repository navigation
Conversation
github-actions
Bot
added
needs triage
|
Wow, what you are doing here is massive. Very impressive. This, all in all, would finally actually make SFTP usable, performant, and reliable after all these decades since its introduction. It's a real shame that I've burned all my credits on #5156, which is now ready to merge in as far as I'm concerned. If I had more, I could have actually off-loaded the grunt work to the agents and had everything reviewed even while still preparing for the exams 😭 |
zyv
added
area: vfs
zyv
left a comment
zyv
left a comment
There was a problem hiding this comment.
I did what I could here, and hope this helps. My suggestions are to build and run tested.
One observation is that some messages show the suffix twice; otherwise, all seems to work correctly:
sftp: connection to server failed: Connection refused (111) (111)
0ab2668 to
7da76de
Compare
Signed-off-by: Phil Krylov <phil@krylov.eu>
Signed-off-by: Phil Krylov <phil@krylov.eu>
Signed-off-by: Phil Krylov <phil@krylov.eu>
7da76de to
9df9ded
Compare
|
I've applied most review suggestions, added localization for the error messages, and tried to fix the errno reset to EIO in |
9df9ded to
9cbdc76
Compare
|
I've fixed missing known_hosts to return E_REMOTE for now, until #5161 gets merged. |
zyv
left a comment
zyv
left a comment
There was a problem hiding this comment.
I've built and run-tested my suggestions, and if you agree with them, I think that this is ready for merge in as far as I'm concerned. Thank you very much for your work on this!
@mc-worker, can you possibly test if this fixes the Android stuff for you? It seems that you were able to reproduce it?
The errors which can be represented with `errno` without losing detail are handled by the calling code. The libssh/SFTP-specific errors which cannot be cleanly mapped to an `errno` are displayed as a separate popup the same way as earlier, and a `E_REMOTE` is returned to the calling code. Signed-off-by: Phil Krylov <phil@krylov.eu> Co-authored-by: Yury V. Zaytsev <yury@shurup.com>
Some VFSes try hard to set a correct verrno in their open_archive() method, some don't. vfs_s_get_path(), the only consumer of the method, has historically set verrno to EIO on any error. From now on, set verrno to EIO in vfs_get_path() only when the method has not set any other verrno, so that a more specific error message than "I/O error" is shown. Signed-off-by: Phil Krylov <phil@krylov.eu>
680dff3 to
16dbfc8
Compare
|
@zyv Yes, I've merged all your suggestions and added a comment in sftpfs_cb_open_archive() regarding the double error message. |
tuffnatty
changed the title
tuffnatty
changed the title
3 participants
Proposed changes
It's a proof-of-concept patch to fix error handling in vfs/sftpfs.
The first two commits implement @zyv 's suggestions about blocking mode and file write API usage from #3654.
The third patch fixes a couple of typos in comments.
The fourth patch improves error handling. It tries to always set an
errnofor the calling code, not try to return-errnoinstead of-1as it's not handled by the calling code, display an additional SFTP-specific error message only when no exactly correspondingerrnoexists.The error handling patch has not been tested well enough but seems to mostly work for me.
The fifth patch changes
vfs_s_get_path()to keepverrnoset byopen_archive()VFS method and only reset it to EIO if a failedopen_archive()call has not set any.This PR has conflicts with my other sftpfs PRs. I'm going to fix these conflicts one by one if something gets merged to master.
Checklist
git commit --amend -smake indent && make check)