Support UNC path capacity on Windows #22202

DoubleSpicy · 2025-01-24T22:04:46Z

Add UNC path capacity calculation capability by invoking Windows API directly.

For #21954,
Can we get a confirmation on linux as well?

DoubleSpicy · 2025-01-24T22:33:21Z

QT seems doesn't have full support of UNC paths in general, a workaround is to mount it as a drive, mentioned at PCSX2/pcsx2#6258 and https://forum.qt.io/topic/96418/how-to-create-a-folder-and-write-a-file-in-it-using-the-unc-path/2

src/gui/addnewtorrentdialog.cpp

DoubleSpicy · 2025-01-28T19:40:56Z

hi @glassez I have reworked this feature and added a bunch of testcases to avoid impacting normal path handling.

Chocobo1 · 2025-01-29T08:05:06Z

src/base/utils/fs.cpp

@@ -209,6 +209,17 @@ Path Utils::Fs::toValidPath(const QString &name, const QString &pad)

 qint64 Utils::Fs::freeDiskSpaceOnPath(const Path &path)
 {
+#if defined(Q_OS_WIN)


Is it possible to further simplify this PR?
I suppose you only need the following to make it work:

bool Path::isUNCPath() const
It will check if the path is a valid UNC path.

This code at Utils::Fs::freeDiskSpaceOnPath()

I especially would like to retain the old QString cleanPath(const QString &path) code if it won't block/get in the way of UNC paths.

Agree perhaps that the cleanPath(path) can be done in an more elegant way. Something like path.toStdWString().find_last_not_of(u"\\"_s)+1 can replace the loops.

Few tricky things to consider when distinguishing UNC paths from normal paths:

QDir::cleanPath is designed for "normal paths", it normalizes all back slashes into forward slashes and try to eliminate extra slashes.
\\host\drive\folder will become //host/drive/folder

PathIsUNCW() from win32 api in my 1st iteration is not reliable as well, it returns true whenever the string starts with double back slashes \\

Path::rootItem() and Path::parentPath() is required in queryFreeDiskSpace(const Path &path) in order to traverse through folders for capacity and create new folders when input path does not exist, but the current version depends on forward slashes /

Wouldn't it be better to just declare UNC paths officially unsupported?

src/base/path.cpp

glassez · 2025-01-30T09:02:35Z

src/base/path.h

@@ -51,6 +51,11 @@ class Path final
    bool isAbsolute() const;
    bool isRelative() const;

+#if defined(Q_OS_WIN)
+    bool isUNCPath() const;
+    QString getUNCRootPathStr() const;


I believe it is part of implementation.

I believe it is part of implementation.

So?

src/base/path.h

src/base/path.cpp

src/base/utils/fs.cpp

src/base/path.cpp

glassez · 2025-01-30T09:29:51Z

test/testpath.cpp

+
+        QCOMPARE(Path(uR"(\\nas01\drive)"_s).rootItem(), Path(uR"(\\nas01\drive)"_s));
+        QCOMPARE(Path(uR"(\\nas01\drive\xxx)"_s).rootItem(), Path(uR"(\\nas01\drive)"_s));
+        QCOMPARE(Path(uR"(\\nas01\drive\xxx\yyy)"_s).rootItem(), Path(uR"(\\nas01\drive)"_s));


Why \\nas01\drive is root item?

It is the ordinary usecase where we mount a network drive on windows, are there usecases where a UNC path is usable in form of \\host without specifying the share component?

I don't have a nas at home handy, the test is done by mounting 2 folder as network drive

C:\Users\Z>net use New connections will be remembered. Status Local Remote Network ------------------------------------------------------------------------------- OK X: \\mydesktop\mount1 Microsoft Windows Network OK Y: \\mydesktop\mount2 Microsoft Windows Network The command completed successfully.

edit: Found a doc from microsoft stating that it must have >= 2 components: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-dfsc/149a3039-98ce-491a-9268-2f5ddef08192

Haven't done any tests with this PR yet, but here's what net use returns on my setup (xxx.xxx.xxx.xxx = IP) with 3x NAS connected.

C:\WINDOWS\system32>net use New connections will be remembered. Status Local Remote Network ------------------------------------------------------------------------------- OK T: \\xxx.xxx.xxx.xxx\Movies Microsoft Windows Network OK V: \\xxx.xxx.xxx.xxx\NAS3 Microsoft Windows Network OK W: \\xxx.xxx.xxx.xxx\Tv Shows Microsoft Windows Network The command completed successfully.

Why \\nas01\drive is root item?

Can \\servername\share not be used like the docs show?

Chocobo1

ps. Some preliminary review. Haven't done a full round.

Chocobo1 · 2025-01-30T15:06:39Z

src/base/path.cpp

+    if (isUNCPath())
+        return true;


Suggested change

if (isUNCPath())

return true;

#if defined(Q_OS_WIN)

if (isUNCPath())

return true;

#endif

Chocobo1 · 2025-01-30T15:11:00Z

src/base/path.cpp

    QString cleanPath(const QString &path)
    {
+#if defined(Q_OS_WIN)
+        // QT's cleanPath does not support UNC path
+        if (isQStrUNCPath(path))


IMO this is full validation which is too expensive here. You should use fast/lightweight checks instead. This function doesn't need validation for constructing paths.

Chocobo1 · 2025-01-30T15:16:08Z

src/base/path.cpp

+    bool isQStrUNCPath(const QString &path)
+    {
+        // strict format of \\host\drive\folder with no forbidden char to avoid conflict with normal path
+        if (path.left(2) != uR"(\\)"_s)


use QStringView::startsWith

Chocobo1 · 2025-01-30T15:16:31Z

src/base/path.cpp

+        // strict format of \\host\drive\folder with no forbidden char to avoid conflict with normal path
+        if (path.left(2) != uR"(\\)"_s)
+            return false;
+        const auto lastSlash = path.lastIndexOf(u'\\');


no auto here please.

Chocobo1 · 2025-01-30T15:16:48Z

src/base/path.cpp

+        const auto lastSlash = path.lastIndexOf(u'\\');
+        if ((lastSlash - 1) <= 2)
+            return false;
+        const QRegularExpression regex {u"[\\0-\\37:?\"*<>|:/]"_s}; // no drive letter allowed C:/


Suggested change

const QRegularExpression regex {u"[\\0-\\37:?\"*<>|:/]"_s}; // no drive letter allowed C:/

static const QRegularExpression regex {u"[\\0-\\37:?\"*<>|:/]"_s}; // no drive letter allowed C:/

Chocobo1 · 2025-01-30T15:17:36Z

src/base/path.cpp

+        if ((lastSlash - 1) <= 2)
+            return false;
+        const QRegularExpression regex {u"[\\0-\\37:?\"*<>|:/]"_s}; // no drive letter allowed C:/
+        return !regex.match(path.data()).hasMatch();


use QStringView::contains

Chocobo1 · 2025-01-30T15:24:31Z

src/base/path.cpp

    QString cleanPath(const QString &path)
    {
+#if defined(Q_OS_WIN)
+        // QT's cleanPath does not support UNC path


Another idea.
You could try to split the UNC path into 2 fields. First is host-name and the second is the rest of the path. host-name will have its own private QString variable in Path class and the second part will be stored in m_pathStr. The second part will look like a relative path.
This way you could reuse existing code and you only need to concat the host-name back when full path is expected in various methods.

https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-dtyp/62e862f4-2a51-452e-8eeb-dc4ff5ee33cc

Chocobo1 · 2025-01-30T15:28:29Z

src/base/path.cpp

@@ -52,8 +52,35 @@ const int PATHLIST_TYPEID = qRegisterMetaType<PathList>("PathList");

 namespace
 {
+#if defined(Q_OS_WIN)
+    bool isQStrUNCPath(const QString &path)


Try to use QStringView as input parameter type.

Chocobo1 · 2025-01-30T15:35:14Z

src/base/path.cpp

+            // only trim off trailing \ which is meaningless
+            int index = path.size();
+            while (path[index - 1] == u'\\')
+                --index;
+            if (index <= (path.size() - 1))
+                return path.left(index);
+            return path;


I don't really like this way. All paths are QDir::cleanPath processed except UNC paths. This creates discrepancy behavior which might create unforeseen surprises in the future.

imo it can be removed with the idea you posted of storing \\host\share root as a string, it also helps to handle extra slashes like \\host\share\xxx\\\\yyy

Chocobo1 · 2025-01-30T15:38:47Z

src/base/path.h

+#if defined(Q_OS_WIN)
+    bool isUNCPath() const;
+    QString getUNCRootPathStr() const;
+#endif


Please move them to the bottom of public: section. To right before private: section.

glassez · 2025-01-30T16:59:03Z

src/base/path.cpp

@@ -52,8 +52,35 @@ const int PATHLIST_TYPEID = qRegisterMetaType<PathList>("PathList");

 namespace
 {
+#if defined(Q_OS_WIN)
+    bool isQStrUNCPath(const QString &path)


Suggested change

bool isQStrUNCPath(const QString &path)

bool isUNCPath(const QString &path)

Neustradamus · 2025-01-31T02:15:06Z

WinDirStat supports it, maybe you can look how it has been done:

https://github.com/search?q=repo%3Awindirstat%2Fwindirstat+unc&type=code

glassez changed the title ~~Support UNC path capacity on windows~~ Support UNC path capacity on Windows Jan 25, 2025

glassez reviewed Jan 25, 2025

View reviewed changes

src/gui/addnewtorrentdialog.cpp Outdated Show resolved Hide resolved

glassez requested changes Jan 25, 2025

View reviewed changes

DoubleSpicy force-pushed the UNC branch 7 times, most recently from c66a932 to 4a58443 Compare January 28, 2025 19:39

DoubleSpicy force-pushed the UNC branch from 4a58443 to 2583114 Compare January 28, 2025 19:45

Chocobo1 reviewed Jan 29, 2025

View reviewed changes

glassez reviewed Jan 30, 2025

View reviewed changes

src/base/path.cpp Outdated Show resolved Hide resolved

glassez reviewed Jan 30, 2025

View reviewed changes

DoubleSpicy force-pushed the UNC branch from 8cefbd8 to 19f3b8d Compare January 30, 2025 10:02

Support UNC path capacity on windows

ccc2bad

DoubleSpicy force-pushed the UNC branch from 19f3b8d to ccc2bad Compare January 30, 2025 10:37

Chocobo1 reviewed Jan 30, 2025

View reviewed changes

glassez reviewed Jan 30, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support UNC path capacity on Windows #22202

Support UNC path capacity on Windows #22202

DoubleSpicy commented Jan 24, 2025 •

edited by glassez

Loading

DoubleSpicy commented Jan 24, 2025

DoubleSpicy commented Jan 28, 2025

Chocobo1 Jan 29, 2025

DoubleSpicy Jan 29, 2025

glassez Jan 30, 2025

glassez Jan 30, 2025

glassez Jan 30, 2025

glassez Jan 30, 2025

DoubleSpicy Jan 30, 2025 •

edited

Loading

xavier2k6 Jan 30, 2025 •

edited

Loading

xavier2k6 Jan 30, 2025

Chocobo1 left a comment

Chocobo1 Jan 30, 2025

Chocobo1 Jan 30, 2025

Chocobo1 Jan 30, 2025

Chocobo1 Jan 30, 2025

Chocobo1 Jan 30, 2025

Chocobo1 Jan 30, 2025

Chocobo1 Jan 30, 2025 •

edited

Loading

Chocobo1 Jan 30, 2025

Chocobo1 Jan 30, 2025

DoubleSpicy Jan 31, 2025

Chocobo1 Jan 30, 2025

glassez Jan 30, 2025

Neustradamus commented Jan 31, 2025

	const QRegularExpression regex {u"[\\0-\\37:?\"*<>\|:/]"_s}; // no drive letter allowed C:/
	static const QRegularExpression regex {u"[\\0-\\37:?\"*<>\|:/]"_s}; // no drive letter allowed C:/

	bool isQStrUNCPath(const QString &path)
	bool isUNCPath(const QString &path)

Support UNC path capacity on Windows #22202

Are you sure you want to change the base?

Support UNC path capacity on Windows #22202

Conversation

DoubleSpicy commented Jan 24, 2025 • edited by glassez Loading

DoubleSpicy commented Jan 24, 2025

DoubleSpicy commented Jan 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DoubleSpicy Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

xavier2k6 Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chocobo1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chocobo1 Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Neustradamus commented Jan 31, 2025

DoubleSpicy commented Jan 24, 2025 •

edited by glassez

Loading

DoubleSpicy Jan 30, 2025 •

edited

Loading

xavier2k6 Jan 30, 2025 •

edited

Loading

Chocobo1 Jan 30, 2025 •

edited

Loading