D27153: port sftp to Result system to force serialization of error/finish condition
sitter created this revision. sitter added reviewers: dfaure, feverfew. Herald added projects: Dolphin, Frameworks. Herald added subscribers: kfm-devel, kde-frameworks-devel. sitter requested review of this revision. REVISION 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 REPOSITORY R320 KIO Extras BRANCH sftp-errors REVISION DETAIL https://phabricator.kde.org/D27153 AFFECTED FILES sftp/kio_sftp.cpp sftp/kio_sftp.h To: sitter, dfaure, feverfew Cc: kde-frameworks-devel, kfm-devel, pberestov, iasensio, fprice, LeGast00n, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov
D27153: port sftp to Result system to force serialization of error/finish condition
feverfew requested changes to this revision.
feverfew added a comment.
This revision now requires changes to proceed.
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.
INLINE COMMENTS
> kio_sftp.cpp:145
> KIO::fileoffset_t offset = -1;
> while ((offset = QT_LSEEK(fd, pos, mode)) == EAGAIN);
> return offset;
Technically unrelated to your changes, but does this even work? isn't the check
supposed to be against `errno` in this case, not offset?
> kio_sftp.cpp:302
> +{
> +if (!result.success) {
> +error(result.error, result.errorString);
`if (result.success) { ...} else { ... } seems clearer to me
> kio_sftp.cpp:328
> +for (int iInt = 0; iInt < n; ++iInt) {
> +unsigned int i = static_cast(iInt); // can only be
> >0
> char echo;
I'm a bit confused, why is this necessary?
> kio_sftp.cpp:418
> long long fileType = QT_STAT_REG;
> -long long size = 0LL;
> +uint64_t size = 0LL;
>
Should then be changed from `0LL` to `0U`?
> kio_sftp.cpp:1493
>
> -void sftpProtocol::close() {
> -closeWithoutFinish();
> -finished();
> +void SFTPInternal::close()
> +{
We should be calling `finished()` (or `Result::pass`) on `close()` IIRC? I'm
not seeing it called.
> kio_sftp.h:77
> +void setHost(const QString &h, quint16 port, const QString& user, const
> QString& pass);
> +Q_REQUIRED_RESULT Result get(const QUrl &url);
> +Q_REQUIRED_RESULT Result listDir(const QUrl &url);
TODO KF6 https://en.cppreference.com/w/cpp/language/attributes/nodiscard ?
REPOSITORY
R320 KIO Extras
REVISION DETAIL
https://phabricator.kde.org/D27153
To: sitter, dfaure, feverfew
Cc: kde-frameworks-devel, kfm-devel, pberestov, iasensio, fprice, LeGast00n,
cblack, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven,
michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns,
emmanuelp, mikesomov
D27153: port sftp to Result system to force serialization of error/finish condition
sitter updated this revision to Diff 75698. sitter marked 2 inline comments as done. sitter added a comment. 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 :( REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27153?vs=74991&id=75698 BRANCH sftp-errors REVISION DETAIL https://phabricator.kde.org/D27153 AFFECTED FILES sftp/kio_sftp.cpp sftp/kio_sftp.h To: sitter, dfaure, feverfew Cc: kde-frameworks-devel, kfm-devel, pberestov, iasensio, fprice, LeGast00n, cblack, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov
D27153: port sftp to Result system to force serialization of error/finish condition
sitter added inline comments.
INLINE COMMENTS
> feverfew wrote in 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?
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.
> feverfew wrote in kio_sftp.cpp:302
> `if (result.success) { ...} else { ... } seems clearer to me
That function is a verbatim copy from ftp so I'd rather not change it.
> feverfew wrote in kio_sftp.cpp:328
> I'm a bit confused, why is this necessary?
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.
> feverfew wrote in kio_sftp.h:77
> TODO KF6 https://en.cppreference.com/w/cpp/language/attributes/nodiscard ?
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.
REPOSITORY
R320 KIO Extras
REVISION DETAIL
https://phabricator.kde.org/D27153
To: sitter, dfaure, feverfew
Cc: kde-frameworks-devel, kfm-devel, pberestov, iasensio, fprice, LeGast00n,
cblack, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven,
michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns,
emmanuelp, mikesomov
D27153: port sftp to Result system to force serialization of error/finish condition
dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.
Nice work! Found 2 bugs though.
INLINE COMMENTS
> kio_sftp.cpp:1306
> rc = ssh_channel_poll(mSftp->channel, 1);
> -}
> -
> -if (rc < 0) {
> +} else if (rc < 0) {
> qCDebug(KIO_SFTP_LOG) << "ssh_channel_poll failed: " <<
> ssh_get_error(mSession);
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".
> kio_sftp.cpp:2075
> if (cPath.isEmpty()) {
> -error(KIO::ERR_MALFORMED_URL, url.toDisplayString());
> -return;
> +Result::fail(KIO::ERR_MALFORMED_URL, url.toDisplayString());
> }
missing "return" in front
> kio_sftp.h:48
> + *
> + * The Result is forwared all the way to the frontend API where it is
> + * turned into an error() or finished() call.
typo: forwar*d*ed
REPOSITORY
R320 KIO Extras
REVISION DETAIL
https://phabricator.kde.org/D27153
To: sitter, dfaure, feverfew
Cc: kde-frameworks-devel, kfm-devel, pberestov, iasensio, fprice, LeGast00n,
cblack, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven,
michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns,
emmanuelp, mikesomov
D27153: port sftp to Result system to force serialization of error/finish condition
sitter updated this revision to Diff 76300. sitter added a comment. 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 REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27153?vs=75698&id=76300 BRANCH sftp-errors REVISION DETAIL https://phabricator.kde.org/D27153 AFFECTED FILES sftp/kio_sftp.cpp sftp/kio_sftp.h To: sitter, dfaure, feverfew Cc: kde-frameworks-devel, kfm-devel, pberestov, iasensio, fprice, LeGast00n, cblack, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov
D27153: port sftp to Result system to force serialization of error/finish condition
dfaure accepted this revision. REPOSITORY R320 KIO Extras REVISION DETAIL https://phabricator.kde.org/D27153 To: sitter, dfaure, feverfew Cc: kde-frameworks-devel, kfm-devel, pberestov, iasensio, fprice, LeGast00n, cblack, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov
D27153: port sftp to Result system to force serialization of error/finish condition
feverfew accepted this revision. This revision is now accepted and ready to land. REPOSITORY R320 KIO Extras BRANCH sftp-errors REVISION DETAIL https://phabricator.kde.org/D27153 To: sitter, dfaure, feverfew Cc: kde-frameworks-devel, kfm-devel, pberestov, iasensio, fprice, LeGast00n, cblack, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov
D27153: port sftp to Result system to force serialization of error/finish condition
This revision was automatically updated to reflect the committed changes. Closed by commit R320:74850eb21654: sftp: port to Result system to force serialization of error/finish condition (authored by sitter). REPOSITORY R320 KIO Extras CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27153?vs=76300&id=76452 REVISION DETAIL https://phabricator.kde.org/D27153 AFFECTED FILES sftp/kio_sftp.cpp sftp/kio_sftp.h To: sitter, dfaure, feverfew Cc: kde-frameworks-devel, kfm-devel, pberestov, iasensio, fprice, LeGast00n, cblack, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, emmanuelp, mikesomov
