dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed.
Nice work! INLINE COMMENTS > ftptest.cpp:33 > +public: > + QUrl url(const QString &path) > + { this method should be const > ftptest.cpp:50 > + // works around the fact that kioslave would load the slave from the > system > + // as first choice instead of the one from teh build dir. > + qputenv("QT_PLUGIN_PATH", > QCoreApplication::applicationDirPath().toUtf8()); typo: teh -> the > ftptest.cpp:53 > + > + // Run ftpd to talk t. > + QVERIFY(m_remoteDir.isValid()); "talk t" ? missing 'o'? > ftptest.cpp:61 > + QVERIFY(m_daemonProc.waitForStarted()); > + QVERIFY(m_daemonProc.state() == QProcess::Running); > + // Wait for the daemon to print its port. That tells us both where > it's listening QCOMPARE so we see the state if it fails > ftptest.cpp:97 > + { > + Q_ASSERT(m_daemonProc.state() == QProcess::Running); > + } QCOMPARE > ftptest.cpp:114 > + job->setUiDelegate(nullptr); > + QVERIFY(job->exec()); > + QCOMPARE(job->data(), data); QVERIFY2(job->exec(), qUtf8Printable(job->errorString())); > ftptest.cpp:167 > + job->setUiDelegate(nullptr); > + QEXPECT_FAIL("", "Trying to write to an inacessible path fails.", > Continue); > + QVERIFY2(job->exec(), qUtf8Printable(job->errorString())); QEXPECT_FAIL is for known bugs. Here, what you want is QVERIFY(!job->exec()); > ftptest.cpp:180 > + inaccessibleUrl.setPassword("password"); > + const QString remoteInaccesiblePath = m_remoteDir.path() + > inaccessiblePath; > + QVERIFY(QFile::copy(QFINDTESTDATA("ftp/testCopy1"), missing a 's' in Inaccessible I'm confused whether this test is about inaccessible path or something around resuming. > ftptest.cpp:186 > + job->setUiDelegate(nullptr); > + QEXPECT_FAIL("", "Trying to write with bad quot doesn't work.", > Continue); > + QVERIFY2(job->exec(), qUtf8Printable(job->errorString())); what's bad quot? Is this test about a bug, or the job failing is what we want? > ftptest.cpp:206 > + job->setUiDelegate(nullptr); > + QVERIFY(job->exec()); > + QCOMPARE(job->error(), 0); I should ask for a clazy rule about QVERIFY(job->exec()) :-) [same as above, QVERIFY2...] > ftptest.cpp:217 > + > +// FIXME: Actually BROKEN > + Now *that* is what QEXPECT_FAIL is about :-) You can re-enable the code, and use QEXPECT_FAIL above the failing QVERIFY or QCOMPARE. > ftp.cpp:671 > + if (!result.success) { > + return result; > + } 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. > ftp.cpp:696 > qCDebug(KIO_FTP) << "Searching for pwd"; > - if (!ftpSendCmd(QByteArrayLiteral("PWD")) || (m_iRespType != 2)) { > + result = ftpSendCmd(QByteArrayLiteral("PWD")); > + if (!result.success || (m_iRespType != 2)) { 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. > ftp.cpp:801 > + const auto result = ftpOpenConnection(LoginMode::Defered); > +#pragma message "we ignore success of the new command its unclear if that is > intentional" > + if (result.success) { 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... > ftp.cpp:828 > } > - return false; > + return Result::fail(); // else we don't know what went > wrong > } 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 } > ftp.cpp:1120 > errormessage = m_host; > - return false; > + return Result::fail(); > } 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. > ftp.cpp:1213 > if (!ftpFolder(src.left(pos + 1), false)) { > - return false; > + return Result::fail(); > } `ERR_CANNOT_ENTER_DIRECTORY, src` > ftp.cpp:1490 > } > - Ftp::stat(linkURL); > - return; > +#pragma message "this used to return nothing and ignored the stat what gives" > + return FtpInternal::stat(linkURL); 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. > ftp.cpp:1602 > - // Servers running with Turkish locale having problems > converting 'i' letter to upper case. > - // So we send correct upper case command as last resort. > - if (!ftpOpenCommand("LIST -la", QString(), 'I', > ERR_CANNOT_ENTER_DIRECTORY)) { What happened to those two lines of comments? Can I suggest to keep them? > ftp.cpp:1880 > + if (!result.success) { > +#pragma message "was server error here" > + return result; Wasn't that just a way to propagate the error from the low-level method to this method? Your new mechanism replaces that, no? > ftp.cpp:1908 > qCWarning(KIO_FTP) << "Can't open for reading"; > - return statusServerError; > +#pragma message "was server error here" > + return result; Same as above: why do we still need this distinction? Is this enum useful? > ftp.cpp:1991 > + } else if ((writeError = WriteToFile(iCopyFile, buffer, n)) != 0) { > +#pragma message "we cannot construct the final error here because we do not > know what the target fiel path is" > + return Result::fail(writeError, QString(), > StatusCode::ClientError); Yep. Pass sCopyFile as argument, when ftpCopyGet calls ftpGet, just like what happens with iCopyFile. > ftp.cpp:2032 > qCDebug(KIO_FTP) << "finished"; > - finished(); > + finished2(Q_FUNC_INFO); > qCDebug(KIO_FTP) << "after finished"; ? > ftp.cpp:2085 > > -Ftp::StatusCode Ftp::ftpPut(int &iError, int iCopyFile, const QUrl &dest_url, > - int permissions, KIO::JobFlags flags) > +#pragma message "ierrror should be moot with results" > +Result FtpInternal::ftpPut(int iCopyFile, const QUrl &dest_url, outdated warning? > ftp.cpp:2169 > + if (!storResult.success) { > +#pragma message "re-constructing failure" > + return Result::fail(storResult.error, storResult.errorString, > StatusCode::ServerError); because of the useless enum? ;) > ftp.cpp:2193 > if (result < 0) { > - iError = ERR_CANNOT_WRITE; > +#pragma message "isnt this cannot read we fail to read the input file here > no" > + writeError = ERR_CANNOT_WRITE; yeah, feel free to change it. > ftp.cpp:2341 > if (m_iRespType != 2) { > - if (bReportError) { > - error(ERR_CANNOT_ENTER_DIRECTORY, path); > - } > +#pragma message "reporterror has been broken not sure what to do with it" > +// if (bReportError) { 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. > ftp.cpp:2363 > > + Result result = Result::pass(); > if (bSrcLocal && !bDestLocal) { // File -> Ftp optimistic but useless initializations, all code paths set (or ignore) result. > ftp.cpp:2409 > #else > return ftpPut(iError, iCopyFile, url, permissions, flags | KIO::Resume); > #endif probably needs the same treatment, i.e. removing iError? (ifdef'ed out code) > ftp.cpp:2488 > + result = Result::fail(ERR_CANNOT_WRITE, StatusCode::ClientError); > +#pragma message "shouldnt we return here" > } no, we keep going in order to delete the .part file > ftp.cpp:2596 > qCDebug(KIO_FTP) << "user canceled proxy authentication, or > communication error."; > - error(errorCode, m_proxyURL.host()); > +#pragma message "problem this is a slot so we cant return a result up the > call chain maybe do it like errno or something" > +// error(errorCode, m_proxyURL.host()); 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... > ftp.cpp:2706 > +{ > + finalize(d->openConnection()); > +} This looks wrong. openConnection() should emit connected() or error(), but never finished(). > ftp.cpp:2766 > +{ > + qDebug() << result; > + if (!result.success) { remember to clean up the qDebug()s before the final version > ftp.h:70 > +struct Result { > + bool success; > + int error; I think you can make all members const? (This will prevent bypassing the "factory methods"...) > ftp.h:95 > +private: > + Result(bool s, int e, const QString &errString, StatusCode code) > + : success(s) This class is an aggregate, so you could probably remove this ctor and use `Result{...}` syntax in the above three methods. > ftp.h:156 > + > + void finished() > + { Does this method serve any purpose? > ftp.h:340 > > - void ftpShortStatAnswer(const QString &filename, bool isDir); > + void ftpShortStatAnswer(const QString &filename, bool isDir) > Q_REQUIRED_RESULT; > Q_REQUIRED_RESULT on a method returning void?? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D23579 To: sitter, dfaure Cc: dfaure, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns