[sftp] treat all errors as errors
ClosedPublic

Authored by sitter on Sep 12 2018, 3:13 PM.

Details

Summary

certain libssh versions sometimes give out bogus errors. namely SSH_FX_OK
can sometimes be the current err even after a function returned with an
"something errord" return value. when converting these bogus errors
to KIO errors we'd sometimes end up reporting NoError when in fact there
was an error, we just don't know what it was because err is incorrect.
this ultimately resulted in KIO ending up in infinite loops because a job
neither errors nor correctly finishes

a scenario where this happens a lot is using dolphin to enter an sftp dir,
then deleting that dir on the CLI. dolphin would keep on querying
sftpProtocol::fileSystemFreeSpace which would run into server errors get
correctly reported by libssh 0.6 but 99% of the time err is SSH_FX_OK which
then locks KIO into an infinite loop of asking for freespace and getting
no answer but also no error. once this happens sftp will be broken from
a user's perspective as opening a new tab/dolphin will not work because
the slave is locked in this infinite loop.

to prevent this and similar issues we'll not let toKIOError ever return
NoError. toKIOError is only called after errors actually happend, so
NoError is not ever correct even when technically mapping the SSH err to
KIO would be NoError. for KIO we need a definitive result, and the
definitive result is always an error.

Diff Detail

Repository
R320 KIO Extras
Branch
always-error
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 2765
Build 2783: arc lint + arc unit
sitter requested review of this revision.Sep 12 2018, 3:13 PM
sitter created this revision.
broulik added inline comments.Sep 12 2018, 3:26 PM
sftp/kio_sftp.cpp
104

Q_UNREACHABLE

sitter updated this revision to Diff 41515.Sep 13 2018, 8:30 AM

s/q_assert/q_unreachable

Fixes the issues I had with sftp hanging in certain situations. (I had to kill sftp.so in the past for accessing sftp shares again)

broulik accepted this revision.Sep 13 2018, 1:49 PM
This revision is now accepted and ready to land.Sep 13 2018, 1:49 PM
broulik added inline comments.Sep 13 2018, 1:50 PM
sftp/kio_sftp.cpp
100–105

Actually we can't get here given the default case, only before when the case SSH_FX_OK would fall-through we could

sitter added inline comments.Sep 14 2018, 9:17 AM
sftp/kio_sftp.cpp
100–105

Yeah. The comment as well as the unreachable /assertion are primarily for future KDE to know not to bring back bad code mappings.

This revision was automatically updated to reflect the committed changes.