port ftp slave to new error reporting system
ClosedPublic

Authored by sitter on Aug 30 2019, 1:11 PM.

Details

Summary

This system is a bit inspired by an earlier patch I threw together in
a half hour to prevent the actual logic code from calling state-changing
functions by splitting the interface class (implements SlaveBase) from
the actual FTP client implementation (used by the interface class).
The advantage here is that the future internal code cannot call error
or finished directly (and cause state violations in SlaveBase - e.g.
double-error call) because there is no trivial way to access them.

Instead the internals return a new Result class if they need to return
anything. This is largely inspired by how Error handling works in Go.
The caller of an internal function needs to check the result or explicitly
disregard it. The caller may then decide to ignore an internal error result
(e.g. chmod after upload failed), create its own higher level result, or
forward it unchanged. On the interface level we then convert the result
to appropriate state changes of the slave.
This is assisted by Q_REQUIRED_RESULT so capable compilers will warn if
a Result is not explicitly ignored as that'd be an indication of missing
error handling in new code, again future proofing the slave.

All things put together this turned out really well I think.

This also adds a primitive test because it broke a million times while
porting. Starts a simple ruby ftpd (with some monkey patching applied).

TODO:

  • there are still a bunch of pending todos littered over the code

Diff Detail

Repository
R241 KIO
Branch
ftp
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 17562
Build 17580: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a project: Frameworks. · View Herald TranscriptAug 30 2019, 1:11 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
sitter requested review of this revision.Aug 30 2019, 1:11 PM
dfaure requested changes to this revision.Sep 21 2019, 3:50 AM

Nice work!

autotests/ftptest.cpp
34

this method should be const

51

typo: teh -> the

54

"talk t" ? missing 'o'?

62

QCOMPARE so we see the state if it fails

98

QCOMPARE

115

QVERIFY2(job->exec(), qUtf8Printable(job->errorString()));

168

QEXPECT_FAIL is for known bugs.

Here, what you want is

QVERIFY(!job->exec());
181

missing a 's' in Inaccessible

I'm confused whether this test is about inaccessible path or something around resuming.

187

what's bad quot?

Is this test about a bug, or the job failing is what we want?

207

I should ask for a clazy rule about QVERIFY(job->exec()) :-)
[same as above, QVERIFY2...]

218

Now *that* is what QEXPECT_FAIL is about :-)

You can re-enable the code, and use QEXPECT_FAIL above the failing QVERIFY or QCOMPARE.

src/ioslaves/ftp/ftp.cpp
663

I don't think we want to return on failure here.
If the server doesn't support this command, we might as well try proceeding.
That's why the old code actually ignored errors here.

682

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.

785

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...

807

I think this is all quite redundant.

openConnection() sets m_bLoggedOn to true if it succeeds, and to false if it fails.
So the ret val from openConnection and m_bLoggedOn are the same thing.
It makes little sense to test both independently and then even have a third case of failure in this line.
It's all the same, I suggest to simplify this.

I think this can all be simplified to

if (!openResult.success) {
    if (m_control) { // ...
        closeConnection();
    }
    return openResult; // or Result::fail(ERR_CANNOT_LOGIN, m_host), if the error code from openResult is no good
}
1101–1102

why not use errorcode and errormessage here?
or in fact just removing that line, so that it goes to the correct return Result::fail(errorcode...) below.

I believe this was a bug in the orig code, setting two local vars and then returning false is nonsense, the return false shouldn't have been there.

1192–1193

ERR_CANNOT_ENTER_DIRECTORY, src

1466

That's because it was a recursive call, so it left the callee in charge of emitting error/finished.
Not doing it here again was actually correct.

Now that error codes are returned, your change is correct, you can remove the pragma.

1602

What happened to those two lines of comments? Can I suggest to keep them?

1857

Wasn't that just a way to propagate the error from the low-level method to this method? Your new mechanism replaces that, no?

1884

Same as above: why do we still need this distinction? Is this enum useful?

1966

Yep. Pass sCopyFile as argument, when ftpCopyGet calls ftpGet, just like what happens with iCopyFile.

2006

?

2059–2060

outdated warning?

2140

because of the useless enum? ;)

2162

yeah, feel free to change it.

2304–2305

Well, that's part of your refactoring. It was used to decide if a method should emit error on failure or just return false. It was part of the big mess that you're cleaning up. Kill bReportError completely.

2323

optimistic but useless initializations, all code paths set (or ignore) result.

2379

probably needs the same treatment, i.e. removing iError?

(ifdef'ed out code)

2458

no, we keep going in order to delete the .part file

2529–2530

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...

2764

This looks wrong. openConnection() should emit connected() or error(), but never finished().

2824

remember to clean up the qDebug()s before the final version

src/ioslaves/ftp/ftp.h
70

I think you can make all members const?

(This will prevent bypassing the "factory methods"...)

95

This class is an aggregate, so you could probably remove this ctor and use Result{...} syntax in the above three methods.

135

Does this method serve any purpose?

321

Q_REQUIRED_RESULT on a method returning void??

This revision now requires changes to proceed.Sep 21 2019, 3:50 AM
anthonyfieroni added inline comments.
src/ioslaves/ftp/ftp.h
49

That's BIC we cannot remove, reorder nor insert parent(s)

146

Should be hidden?

pino added a subscriber: pino.Sep 21 2019, 5:48 AM
pino added inline comments.
src/ioslaves/ftp/ftp.h
49

That's BIC we cannot remove, reorder nor insert parent(s)

This is a kioslave, built as plugin. There is no such thing as BC for this kind of stuff.

pino removed a subscriber: pino.Sep 21 2019, 5:49 AM
sitter updated this revision to Diff 66747.Sep 24 2019, 1:20 PM
sitter marked 31 inline comments as done.

address numerous comments (marked done on phab)

sitter added inline comments.Sep 24 2019, 1:20 PM
autotests/ftptest.cpp
187

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

src/ioslaves/ftp/ftp.cpp
682

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?

1602

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

1857

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!

2323

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?

2529–2530

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/

2764

You may wish to take a look at D23537 ;)

src/ioslaves/ftp/ftp.h
70

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.

135

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.

146

Does that make a useful difference for a plugin?

sitter edited the summary of this revision. (Show Details)Sep 24 2019, 1:21 PM
dfaure requested changes to this revision.Oct 9 2019, 5:41 AM
dfaure added inline comments.
src/ioslaves/ftp/ftp.cpp
682

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 :)

785

How about adding a check for the ret val?

2006

finished(Q_FUNC_INFO) looks like a local hack?

2138

remember to remove this

2323

Ah ok, never mind then.

2455

s/Closing/closing/

2529–2530

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.

This revision now requires changes to proceed.Oct 9 2019, 5:41 AM
sitter updated this revision to Diff 67695.Oct 11 2019, 11:30 AM
sitter marked 24 inline comments as done.
  • rebase
  • ftpSendCmd now returns a bool (continues to require_result)
  • openConnection now emits opened()
  • listDir now finalize()s
  • debug--
  • ftpSendCmd's lazy login now checks the return value of the recrusive cmd call

ftpOpenControlConnection's proxy handling is completely redone:

  • loosened nesting
  • socks5 is now also supported as scheme
  • reliance on eventloop is gone and replaced with actually sync behavior for the sync synchronousConnectToHost
    • the way proxy management is done is no longer using applicationproxy but instead managing the proxy on the socket(s) directly. this makes things somewhat more explicit and gives less opportunity for errors inside Qt behavior
    • now returns a composite Result type to carry both context and socket to the caller. contextual error forwarding is handy here because there is value in differntiating why authentication isn't working when e.g. kiod is defunct.
    • qauthenticator use gone as it has no api outside the auth signal
    • proxyAuthentication and saveProxyAuthentication have been merged into synchronousConnectToHost and been simplified
      • now only uses one AuthInfo object
      • realm is no longer set (came from QAuthenticator but as far as I can tell the QA created by QAbstractSocket has no realm set, so this never did anything anyway)
      • as a result the comment field in password queries is now only showing the hostname instead of a broken " at $hostname"
  • added ProxyTesting.md with some guidance on how to test this stuff
sitter retitled this revision from WIP: port ftp slave to new error reporting system to port ftp slave to new error reporting system.Oct 11 2019, 11:31 AM
sitter edited the summary of this revision. (Show Details)
sitter updated this revision to Diff 67697.Oct 11 2019, 11:39 AM

move away from helper functions to redirect to q-> and instaed call via q-> directly

sitter edited the summary of this revision. (Show Details)Oct 11 2019, 1:35 PM
dfaure accepted this revision.Oct 12 2019, 9:39 PM

Excellent work!

src/ioslaves/ftp/ftp.cpp
2635

yeah, looks like wishful thinking.... remove?

src/ioslaves/ftp/ftp.h
70

Copying bool+int+QString seems rather cheap, we lived with that until C++11 move semantics ;-)

Private members and inline getters sounds good to me.

But yeah, no big deal, we can also trust the programmer :-)

This revision is now accepted and ready to land.Oct 12 2019, 9:39 PM
This revision was automatically updated to reflect the committed changes.
sitter marked 3 inline comments as done.

@sitter The unittest just failed on CI, the test can't connect to the local FTP server, can you look into it?

https://build.kde.org/job/Frameworks/job/kio/job/kf5-qt5%20SUSEQt5.13/211/testReport/junit/projectroot/autotests/kiocore_ftptest/

I'll see about adding some debug output for the daemon state. Hard to say why it's flakey with the info at hand.

Wait, nevermind, it was a regression in the mapConfig() related code, fixed meanwhile. KIO is green again on CI. Sorry for the false alert.