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

[David Faure]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20190921/7a8fc5d9/attachment-0001.html>