D27153: port sftp to Result system to force serialization of error/finish condition

[Alexander Saoutkin]

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<unsigned int>(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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20200213/089595ed/attachment.htm>