Differential D27153

port sftp to Result system to force serialization of error/finish condition
ClosedPublic
Actions

Authored by sitter on Feb 4 2020, 12:29 PM.

Details

Reviewers
dfaure
feverfew
Commits
R320:74850eb21654: sftp: port to Result system to force serialization of error/finish condition
Summary

the Result system was originally introduced to the FTP slave and now also
makes an appearance in the SFTP slave. the system introduces a separation
between logic and fronting API class to more tightly control when state
changing calls (finished()/error()) are made. since these calls may only
be made once during a given command multiple calls are at the very least
indicative of bad code and at worst cause severe state confusion for the
slavebase that it won't be able to recover from, rendering the slave
instance broken.

in the internal class Results are returned whenever an error can appear and
these Results must be handled in some form. the only way to effectively
produce user visible errors is to forward results up the call chain.

Test Plan
  • connect
  • copy remotely
  • overwrite remotely
  • copy to local
  • overwrite to local
  • copy from local to remote
  • copy form local to remote and overwrite

Diff Detail

Repository
R320 KIO Extras
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
sitter created this revision.Feb 4 2020, 12:29 PM
Restricted Application added projects: Dolphin, Frameworks. · View Herald TranscriptFeb 4 2020, 12:29 PM
Restricted Application added subscribers: kfm-devel, kde-frameworks-devel. · View Herald Transcript
sitter requested review of this revision.Feb 4 2020, 12:29 PM
feverfew requested changes to this revision.Feb 13 2020, 10:40 PM

Good stuff, I'll admit I kind of skimmed over the bits that were rewrites error(...); return; -> Result::fail(), I assume you've mapped correctly there. Only minor comments from me.

sftp/kio_sftp.cpp
145

Technically unrelated to your changes, but does this even work? isn't the check supposed to be against errno in this case, not offset?

302

`if (result.success) { ...} else { ... } seems clearer to me

328

I'm a bit confused, why is this necessary?

418

Should then be changed from 0LL to 0U?

1495–1498

We should be calling finished() (or Result::pass) on close() IIRC? I'm not seeing it called.

sftp/kio_sftp.h
77

TODO KF6 https://en.cppreference.com/w/cpp/language/attributes/nodiscard ?

This revision now requires changes to proceed.Feb 13 2020, 10:40 PM
sitter updated this revision to Diff 75698.Feb 14 2020, 3:35 PM
sitter marked 2 inline comments as done.

s/LL/U

call finished() from frontend close() (internal close cannot finish and cannot error). " // must call finish(), which will set d->inOpenLoop=false" one of many expectations not asserted by slavebase :(

sitter added inline comments.Feb 14 2020, 3:41 PM
sftp/kio_sftp.cpp
145

True enough. I expect, like many things in the slaves, this never actually worked. that line was introduced as KDE_lseek and that too was just an alias for lseek :|
Also lseek doesn't even errno EAGAIN according to the manpage. I expect seeking as a whole needs fixing, I'll record a bug.

302

That function is a verbatim copy from ftp so I'd rather not change it.

328

Kinda unrelated to the diff, I only changed it because this class has way too many type coercions.
i goes into ssh_userauth_kbdint_getprompt which has an unsigned signature, the counter n coming out of ssh_userauth_kbdint_getnprompts is signed though. The explicit cast silences the warning of the type coercion between int and unsigned.

Mind you, I think n<1 cannot even happen. The server challenged us, so there's always going to be at least one prompt, so the fact that it returns a signed prompt count is silly to begin with.

sftp/kio_sftp.h
77

I guess, that applies to all slaves though. ftp already has it and smb is also going to get it at some point. If we want a todo comment I'd put it in the ftp code. Though TBH I think that should just be a clazy check or deprecation inside Qt when built with c++17.

dfaure requested changes to this revision.Feb 22 2020, 8:52 AM

Nice work! Found 2 bugs though.

sftp/kio_sftp.cpp
1308

Warning, you changed the logic. This "else" wasn't there, in order to catch the case where the second call to ssh_channel_poll failed too. I think you need to revert this "else".

2077

missing "return" in front

sftp/kio_sftp.h
48

typo: forwar*d*ed

This revision now requires changes to proceed.Feb 22 2020, 8:52 AM
sitter updated this revision to Diff 76300.Feb 24 2020, 2:45 PM

fix problems found by dfaure

I've also added Q_REQUIRED_RESULT to fail() and pass() to help not forget to do something with
the return value. there's no reason to call them and ignore the constructed object

dfaure accepted this revision.Feb 24 2020, 8:44 PM
feverfew accepted this revision.Feb 24 2020, 11:22 PM
This revision is now accepted and ready to land.Feb 24 2020, 11:22 PM
Closed by commit R320:74850eb21654: sftp: port to Result system to force serialization of error/finish condition (authored by sitter). · Explain WhyFeb 26 2020, 11:27 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Msftp/kio_sftp.h (239 lines)
Msftp/kio_sftp.cpp (1222 lines)

Diff 76452

View Options

sftp/kio_sftp.h

Loading...
View Options

sftp/kio_sftp.cpp

Loading...