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

[Harald Sitter]

sitter added inline comments.

INLINE COMMENTS

> dfaure wrote in ftptest.cpp:186
> what's bad quot?
> 
> Is this test about a bug, or the job failing is what we want?

That was meant to read quota. I've changed it to QVERIFY(!job->exec()); as per your earlier comment.

> dfaure wrote in ftp.cpp:696
> The reuse of the `result` var irks me a bit.
> const vars rock :-)
> 
> Would it be a bad idea to make Result implicitly convertible to bool, so you can keep doing this inside the if? ` if (!ftpSendCmd(QByteArrayLiteral("PWD")) || (m_iRespType != 2)) {` would still work fine then.
> 
> This would simplify all cases where we test for failures, but ignore the actual error message on failure (because we'll provide a more high-level one, like here). In other cases, we indeed need a result var.

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?

> dfaure wrote in ftp.cpp:1602
> What happened to those two lines of comments? Can I suggest to keep them?

Oh! Collateral damage from removing the nesting I guess. Comments are back now.

> dfaure wrote in ftp.cpp:1880
> Wasn't that just a way to propagate the error from the low-level method to this method? Your new mechanism replaces that, no?

I could have sworn there was code that differentiated between server and client. I can't find it anymore... maybe it was all a dream, or maybe I saw it in the history.

Oh well, down with the useless enum!

> dfaure wrote in ftp.cpp:2363
> optimistic but useless initializations, all code paths set (or ignore) result.

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?

> dfaure wrote in ftp.cpp:2596
> Yes, clearly. But the question is where to check it.
> 
> Hmm, how does this method even get called, without an event loop? From `m_control->waitForReadyRead`, maybe?
> 
> The old code was broken here, it would for sure emit error+error or error+finished, since the main code had no idea that this slot emitted error already.
> 
> Hmm. Shouldn't we do this connect *before* the waitForConnected in Ftp::synchronousConnectToHost? If a proxy exists, it will surely act at connection time?
> And then it becomes easier, the place where to check the errno-like member is just after the call to Ftp::synchronousConnectToHost...

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/

> dfaure wrote in ftp.cpp:2706
> This looks wrong. openConnection() should emit connected() or error(), but never finished().

You may wish to take a look at D23537 <https://phabricator.kde.org/D23537> ;)

> dfaure wrote in ftp.h:70
> I think you can make all members const?
> 
> (This will prevent bypassing the "factory methods"...)

It's a good idea. Would that work though?

Currently the results rely on the implicit move operator when collecting the returned result

  result = ftpGet()

if the members are const we couldn't move/copy like that anymore.

If we consider the mutability a problem I think I'd just make the members private and give them getters. I don't mind much either way.

> dfaure wrote in ftp.h:156
> Does this method serve any purpose?

Its sole purpose is to make it harder to call

`q->finished()` in the internal class (and that way bypass the result system). Same for error() above.

> anthonyfieroni wrote in ftp.h:167
> Should be hidden?

Does that make a useful difference for a plugin?

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/20190924/70697a77/attachment-0001.html>