[Interest] Behavioural change in QFile/QDir rename

[Elvis Stansvik]

Den mån 17 juni 2019 kl 09:37 skrev Federico Buti <fed.buti at gmail.com>:
>
> Thanks all.
>
> My bad, I referred the crosscompilation for Windows as a reference point from where we started the Qt bump, mostly because that was the major showstopper. But on Linux we use vanilla Qt from qt.io installers. Hence I think any compilation issue in Qt can be ruled out; or at least it feels quite unlikely to be the real problem.

The official binaries I think are built on CentOS 7 (?). It seems to
have glibc 2.17, so those binaries would not use renameat2.

(I could be wrong because I don't know exactly how the build machines
are set up).

Elvis

> The meld screenshot, from the first mail, is a comparison between vanilla Qt 5.6.2 and vanilla Qt 5.12.2, both from the Qt installer.
>
> As a secondary note, we do not use QTemporaryFile isntances in the test but plain QFile intances, created inside a "source" QTemporaryDir from which files are moved.
>
> Bests,
> F.
>
>
> On Mon, 17 Jun 2019 at 08:48, Elvis Stansvik <elvstone at gmail.com> wrote:
>>
>> Den mån 17 juni 2019 07:47Elvis Stansvik <elvstone at gmail.com> skrev:
>>>
>>> Den mån 17 juni 2019 kl 01:29 skrev Federico Buti <fed.buti at gmail.com>:
>>> >
>>> > Hi Thiago.
>>> >
>>> > Thanks for the feedback.
>>> > I'm on Fedora 29 so definitely not on an old kernel as that. My colleague is on Ubuntu but I'm pretty sure it is a 16.xx release. I guess the usage of renameat2() is failing for some other reason. What could that be? Any idea?
>>>
>>> Apart from what Thiago said (you need a Qt built on a kernel that
>>> supports the syscall), another reason could be the file system that
>>> does not support renameat2.
>>
>>
>> To be completely correct, it may support the syscall, just not the RENAME_NOREPLACE flag.
>>
>> Elvis
>>
>>>
>>> For example non-local file systems (e.g. Samba/NFS/...) and eCryptfs
>>> do not support it.
>>>
>>> (I once made a bugreport against QTemporaryFile::rename about this,
>>> https://bugreports.qt.io/browse/QTBUG-64008 , and Thiago fixed it by
>>> making QFileSystemEngine fall back to link/unlink if renameat2 fails
>>> with EINVAL)
>>>
>>> Elvis
>>>
>>> Elvis
>>>
>>> >
>>> > Just in case, here is the code of the test. It basically create two temporary directories, starts watching on one of them via our FileEventsGatherer class and once the files are moved via QFile::rename, it compares the number of registered move/renames with the expect ones. "mkFile" just creates the file via QFile. No more, no less.
>>> >
>>> > void <---->::testMoveFile()
>>> > {
>>> >     QFETCH(QStringList,
>>> >            fileNames);
>>> >     QFETCH(qint32,
>>> >            expectedResult);
>>> >     QFETCH(bool,
>>> >            rename);
>>> >
>>> >     QTemporaryDir monitoredDir;
>>> >     QTemporaryDir externalDir;
>>> >     FileEventsGatherer feg;
>>> >     feg.setPath(monitoredDir.path(),
>>> >                 FileEventsGatherer::FILE_EVENT__ALL,
>>> >                 OUTBOX_FILE_NAME_FILTER);
>>> >     QSet<QString> overallMoved;
>>> >     connect(&feg,
>>> >             &FileEventsGatherer::filesMoved,
>>> >             this,
>>> >             [&overallMoved](QSet<QString> const &movedFiles)
>>> >             {
>>> >                 overallMoved.unite(movedFiles);
>>> >             });
>>> >
>>> >     for (auto const &file : fileNames)
>>> >     {
>>> >         QString const choosenPath {rename
>>> >                                    ? monitoredDir.path()
>>> >                                    : externalDir.path()};
>>> >         auto const sourcePath = QString("%1/%2").arg(choosenPath).arg("TempName");
>>> >         auto const destPath = QString("%1/%2").arg(monitoredDir.path()).arg(file);
>>> >         mkFile(sourcePath);
>>> >         QFile mover(sourcePath);
>>> >         mover.rename(destPath);
>>> >     }
>>> >
>>> >     QTest::qWait(SIGNALS_TIMEOUT_MS);
>>> >     QCOMPARE(overallMoved.count(),
>>> >              expectedResult);
>>> > }
>>> >
>>> > Thanks,
>>> > F.
>>> >
>>> >
>>> > On Sat, 15 Jun 2019 at 20:50, Thiago Macieira <thiago.macieira at intel.com> wrote:
>>> >>
>>> >> On Saturday, 15 June 2019 08:58:31 PDT Federico Buti wrote:
>>> >> > We gave strace a go and we noticed that 5.12 runtime acts differently for
>>> >> > rename/move operations (see attached compare screenshot). In particular, on
>>> >> > 5.6 we have an actual rename whereas a link/unlink sequence happens on
>>> >> > 5.12. That sequence is not detected by our code, generating the failure.
>>> >>
>>> >> Alternative: upgrade your system to a version that supports the renameat2()
>>> >> system call. You need a 3.16 Linux kernel and glibc 2.28.
>>> >>
>>> >> The link()/unlink() sequence is used on Unix filesystems to prevent rename()
>>> >> clobbering existing files, as promised in our API, but they're only attempted
>>> >> if renameat2() fails.
>>> >>
>>> >> --
>>> >> Thiago Macieira - thiago.macieira (AT) intel.com
>>> >>   Software Architect - Intel System Software Products
>>> >>
>>> >>
>>> >>
>>> >> _______________________________________________
>>> >> Interest mailing list
>>> >> Interest at qt-project.org
>>> >> https://lists.qt-project.org/listinfo/interest
>>> >
>>> > _______________________________________________
>>> > Interest mailing list
>>> > Interest at qt-project.org
>>> > https://lists.qt-project.org/listinfo/interest