[stable-3.2] Temporary! Windows. VFS. Block Virtual Files for partition root sync folders.

[backportbot-nextcloud]

backport of #3290

[allexzander]

/rebase

[mgallien]

why are you doing a hand made implementation of that instead of using Qt API ?

[allexzander]

why are you doing a hand made implementation of that instead of using Qt API ?

@mgallien

• there is no known API for achieving this or similar behavior and I am not surprised, as, Qt offers some cross-platform APIs, but, the concept of a root partition is only present in Windows
• Qt API has proven to bring issues (e.g. QFileInfo leads to a freeze when calling its methods on .lnk files on Windows, while Windows API works with no problems in that case
• Algorithm is simple enough to be confident of custom code will work as expected
• Are you aware which Qt API allows achieving this, or at least something close to this that I could use? Could you provide a link?

[mgallien]

why are you doing a hand made implementation of that instead of using Qt API ?

@mgallien

* there is no known API for achieving this or similar behavior and I am not surprised, as, Qt offers some cross-platform APIs, but, the concept of a root partition is only present in Windows

* Qt API has proven to bring issues (e.g. QFileInfo leads to a freeze when calling its methods on **.lnk** files on Windows, while Windows API works with no problems in that case

That was specific to the case of VFS, implicit hydration of a file linked by a .lnk file and the current behavior of QFileInfo that consider that .lnk are not real file and jump to the linked file.
So we had this scenario:

1. We are within a sync folder with VFS CfApi enabled
2. We build a QFileInfo on a .lnk file
3. QFileInfo try to access the linked file (let's suppose it is within the same sync folder)
4. CfApi triggers an implicit hydration of the file
5. A callback is called in a thread within desktop client
6. An HydrationJob is created and moved to the main thread
7. We have a deadlock due to the main thread being blocked within QFileInfo waiting for the job to complete and HydrationJob cannot start because the thread is blocked

In the case of Windows drives, we will have nothing of that. We are safe.

* Algorithm is simple enough to be confident of custom code will work as expected

Yes but we add a few more branches in the code adding complexity that we will have to maintain by ourselves. Including some code that is specific to one platform and cannot really be tested when testing other platforms.

* Are you aware which Qt API allows achieving this, or at least something close to this that I could use? Could you provide a link?

I am talking about that QDir::drives(). Did you try to use it ?

[allexzander]

@mgallien that's exactly what I've tried. The problem with that method is, it always returns "C" even if I am calling it for a directory that is in "D". How can you explain such behavior? Maybe the documentation is not clear?

[allexzander]

@mgallien This method returns all drive letters available in the system. What I need instead is, to check if the path I choose is any drive letter without a subfolder. Let's say I choose "d:/subfolder" - then it is not a drive letter (not a drive partition root). Now, if I choose just "d:/" or "d:", or "d:" - then - it is a drive letter (or a drive partition root). More detailed explanation and a screenshot below:

So, let's say, I choose "d:" as my sync root. This is perfectly fine, and, this will work. However, this does not compare exactly with ["C:/", "D:/"], I'd still have to compare if any of ["C:/, "D:/"] starts with my "d:". Then, I could also choose "d:\" as my local sync dir (which is perfectly fine under Windows as it is using a backslash instead of slash by default). In that case, I am having even more problems, I still have to convert backslash to a slash before comparing it against ["C:/", "D:/"].

Now, I am not interested in checking whether the partition I choose is a valid partition on the current system. For example, I don't want to check if "b:/" is incorrect by trying to find it in ["C:/", "D:/"].

Last but not least, imagine, a system has many partitions ["C:/", "D:/", "E:/", "F:/", G:/", ...]

if using this API and a method and looping through a list of partitions trying to find if any of those equal to my sync folder that I choose to be say "S:/", I will have to loop every time, through entire list of partitions.

My goal is to just compare if my path equals to a pattern "AlphaBeticCharacter" + ":" + "slash or backslash or no slash at all".

[allexzander]

@mgallien yes, I've now noticed, dir() is a static method. Then again, there is no way to "just check if a path is a drive partition root" via Qt API. This method does not provide me this information. I still have to compare strings.

[mgallien]

Can you please add an automated test to check the behavior or Utility::isPathWindowsDrivePartitiionRoot ?
Put it only on master and we can omit the backport but I would feel much more confident with one test

[allexzander]

Can you please add an automated test to check the behavior or Utility::isPathWindowsDrivePartitiionRoot ?
Put it only on master and we can omit the backport but I would feel much more confident with one test

@mgallien Added unit tests https://github.com/nextcloud/desktop/pull/3393/files

[mgallien]

Please let's try to see if we really cannot improve this code in master

[allexzander]

Tested this API again. It always returns a list of 2 drives in my case ("C:/" and "D:/") for a dir that is in drive "d:". Now, I am confused, how do I decide which of the 2 should I choose? From what I see, this method always returns a list of partitions not related to the path that the QDir instance has.

…

On Wed, 2 Jun 2021 at 11:04, Matthieu Gallien ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/common/utility.cpp <#3359 (comment)>: > @@ -676,6 +676,26 @@ QByteArray Utility::conflictFileBaseNameFromPattern(const QByteArray &conflictNa return conflictName.left(tagStart) + conflictName.mid(tagEnd); } +bool Utility::isPathWindowsDrivePartitionRoot(const QString &path) +{ + Q_UNUSED(path) +#ifdef Q_OS_WIN + // should be 2 or 3 characters length why are you doing a hand made implementation of that instead of using Qt API ? @mgallien <https://github.com/mgallien> * there is no known API for achieving this or similar behavior and I am not surprised, as, Qt offers some cross-platform APIs, but, the concept of a root partition is only present in Windows * Qt API has proven to bring issues (e.g. QFileInfo leads to a freeze when calling its methods on **.lnk** files on Windows, while Windows API works with no problems in that case That was specific to the case of VFS, implicit hydration of a file linked by a .lnk file and the current behavior of QFileInfo that consider that .lnk are not real file and jump to the linked file. So we had this scenario: 1. We are within a sync folder with VFS CfApi enabled 2. We build a QFileInfo on a .lnk file 3. QFileInfo try to access the linked file (let's suppose it is within the same sync folder) 4. CfApi triggers an implicit hydration of the file 5. A callback is called in a thread within desktop client 6. An HydrationJob is created and moved to the main thread 7. We have a deadlock due to the main thread being blocked within QFileInfo waiting for the job to complete and HydrationJob cannot start because the thread is blocked In the case of Windows drives, we will have nothing of that. We are safe. * Algorithm is simple enough to be confident of custom code will work as expected Yes but we add a few more branches in the code adding complexity that we will have to maintain by ourselves. Including some code that is specific to one platform and cannot really be tested when testing other platforms. * Are you aware which Qt API allows achieving this, or at least something close to this that I could use? Could you provide a link? I am talking about that QDir::drives() <https://doc.qt.io/qt-5/qdir.html#drives>. Did you try to use it ? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#3359 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AC5IMDVNXTWKCF63IQCPBDDTQXQX5ANCNFSM45OYHVYQ> .

[allexzander]

/rebase

[nextcloud-desktop-bot]

AppImage file: Nextcloud-PR-3359-16a58db75ddd40dae5d59a43b8a52486bb2acb49-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.