D17816: Support for xattrs on kio copy/move

[Pino Toscano]

pino added a comment.


  Nice progresses, thanks for the fixes. I added few more notes, just mentioning the first occurrence of each.
  One more thing is to print errno (and possibly its string representation using `strerror`/`strerror_r`) on failure, so that the debugging is easier.

INLINE COMMENTS

> jobtest.cpp:454
>      const int perms = 0666;
> +
>      // copy the file with file_copy

extra empty line

> jobtest.cpp:534
> +     * Set xattrs on source file using default linux commandline
> +     * TODO: port to multiplatfom
> +     * TODO: write tests for copyAs

you can use `QStandardPaths::findExecutable()` to check whether `setfattr` is available, and QSKIP the test if not

> jobtest.cpp:540
> +    QString command = QString("setfattr -n user.baloo.rating -v 1 %1").arg(src);
> +    xattrwriter.start(command);
> +    xattrwriter.waitForFinished(-1);

please use the `QProcess::start(QString,QStringList)` variant, splitting the arguments; this way there is no need to manual quoting the paths, and thus avoid problems with spaces

> jobtest.cpp:540
> +    QString command = QString("setfattr -n user.baloo.rating -v 1 %1").arg(src);
> +    xattrwriter.start(command);
> +    xattrwriter.waitForFinished(-1);

check using `QVERIFY`/`QCOMPARE` that the process was started correctly

> jobtest.cpp:541
> +    xattrwriter.start(command);
> +    xattrwriter.waitForFinished(-1);
> +    command = QString("setfattr -n user.fnoValue %1").arg(src);

check using `QVERIFY`/`QCOMPARE` that the process ended correctly

> CMakeLists.txt:5-6
>  
> +check_include_files(sys/xattr.h HAVE_SYS_XATTR_H)
> +check_include_files(sys/extattr.h HAVE_SYS_EXTATTR_H)
> +

good checks, although they fit better in `ConfigureChecks.cmake` (which is there for a reasons ;) )

> copyjob.cpp:2192
> +    if (!QFileInfo::exists(source.toLocalFile()) && !QFileInfo::exists(dest.toLocalFile())) {
> +        qCWarning(KIO_COPYJOB_DEBUG) << "failed to open source and destiny";
> +        return;

destiny is another thing ;) "destination" in this case

> copyjob.cpp:2205-2209
> +    if (listlen < 0) {
> +        qCWarning(KIO_COPYJOB_DEBUG) << "libc failed to extract xattr from " << xattrsrc;
> +    } else if (listlen == 0) {
> +        qCDebug(KIO_COPYJOB_DEBUG) << "File " << xattrsrc << " don't have any xattr.";
> +    } else {

this is more of a personal taste rather than an issue: IMHO you can avoid the "if .. else if ... else" that adds a nesting level more, especially when all the checks lead to early return; so something like:

  if (listlen < 0) {
    if (errno != ENOTSUP) {
      qCWarning(KIO_COPYJOB_DEBUG) << "failed to extract xattr from" << xattrsrc;
    }
    return;
  } 
  if (listlen == 0) {
    qCDebug(KIO_COPYJOB_DEBUG) << "File" << xattrsrc << "doesn't have any xattr.";
    return;
  }
  [etc..]

> copyjob.cpp:2256
> +                if (errno == ENOTSUP) {
> +                    qCWarning(KIO_COPYJOB_DEBUG) << "destination filesystem don't support xattrs.";
> +                } else {

IMHO this is not a warning worth situation

REPOSITORY
  R241 KIO

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

To: cochise, dfaure
Cc: abika, pino, davidedmundson, ngraham, atha.kane, spoorun, nicolasfella, kde-frameworks-devel, michaelh, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20190103/af4dbc83/attachment-0001.html>