D23579: WIP: port ftp slave to new error reporting system

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> ftp.cpp:2153
> +    const auto storResult = ftpOpenCommand("stor", dest, '?', ERR_CANNOT_WRITE, offset);
> +    qDebug() << "!!! storresult" << storResult;
> +    if (!storResult.success) {

remember to remove this

> ftp.cpp:2465
> +    if (QT_CLOSE(iCopyFile) == 0 && !result.success) {
> +        // If Closing the file failed but there isn't an error yet, switch
> +        // into an error!

s/Closing/closing/

> sitter wrote in ftp.cpp:696
> I know what you mean, I originally started out with consts but moved away because it was getting fairly repetitive and in the end the constness offers little, the objects are fairly "ephemeral" much like errno.
> 
> The result handling is indeed very "wordy", but I think there is value to be had in the expressiveness there:
> A Result is not really a binary affair. Yes it is failure or success, but if it is a failure the developer **must** make a conscious choice to not use the context information of. If Result was implicitly usable as a bool we'd not force this conscious choice, opening us up for mistakes further down the line when everyone has forgotten if ftpSendCmd needs/deserves result handling... or worse yet, looking at old code it won't be obvious if the author ignored the result context by accident or intentionally.
> 
> So, I would stay away from treating a Result like a bool.
> 
> To get to the heart of the issue though: I actually think ftpSendCmd probably shouldn't even return a Result but rather a bool. Probably 99% of all callers entirely ignore the Result context and treat it as a bool, which seems like a strong indication that the return type is unnecessarily complex. Should I go ahead with making it bool?

Forcing to use a local variable isn't forcing to test the error code, though. So I'm not convinced by this argument. But yes, let's go for your suggestion, ftpSendCmd should return a bool, good compromise.

The only error codes it currently returns are "unsupported action" (unlikely case of programmer error) and "cannot login". So false obviously means "cannot login" anyway. Not sure all callers of ftpSendCmd realized this, though :)

> dfaure wrote in ftp.cpp:801
> Oh... it's a recursive call... wow, tricky.
> 
> Yeah, that means this method returns false even if the nested method managed to login. Very confusing, and probably wrong...

How about adding a check for the ret val?

> dfaure wrote in ftp.cpp:2032
> ?

`finished(Q_FUNC_INFO)` looks like a local hack?

> sitter wrote in ftp.cpp:2363
> I do need to declare it though, and by design a Result cannot have a "null" state, so I either need to initialize a failure or pass here. Do you have a better suggestion?

Ah ok, never mind then.

> sitter wrote in ftp.cpp:2596
> Since you mention it... where even is the eventloop for any of this?
> SlaveBase has no event processing, neither does the FTP slave. I get the distinct feeling that the entire auth code never worked, or at least not since kf5. To that end I'd actually be in favor of removing it since no one complained (or I cannot find any bug report anyway).
> 
> From some limited testing your assessment would be mostly correct though. When trying to waitForConnected there'd be a ProxyAuthenticationRequiredError. Which we can handle somewhat awkwardly by querying the info from the user and then setting a newly "enhanced" proxy QNetworkProxy::setApplicationProxy before creating a new socket I guess.
> 
> I'm not too excited about fixing proxy auth support no one apparently needs.
> 
> Originally introduced without review as well:
> https://git.reviewboard.kde.org/r/102923/

The (restricted) event loop is within waitFor*. It's not a full event loop, but it processes socket events, which in turn leads to that proxy handling.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D23579

To: sitter, dfaure
Cc: anthonyfieroni, dfaure, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20191009/2d02b7f8/attachment-0001.html>