Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support UNC path capacity on Windows #22202

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
+117 −13
Draft

Support UNC path capacity on Windows #22202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 62 additions & 12 deletions src/base/path.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ namespace
{
QString cleanPath(const QString &path)
{

const bool hasSeparator = std::any_of(path.cbegin(), path.cend(), [](const QChar c)
{
return (c == u'/') || (c == u'\\');
Expand All @@ -74,8 +75,18 @@ namespace
static_assert(Stringable<Path>);

Path::Path(const QString &pathStr)
: m_pathStr {cleanPath(pathStr)}
{
#if defined(Q_OS_WIN)
if (validateUNCPath(pathStr))
{
auto [rootPath, filename] = splitUNCPath(pathStr);
isUNCPath = true;
rootStr = rootPath;
m_pathStr = cleanPath(filename).replace(u"/"_s, u"\\"_s);
return;
}
#endif
m_pathStr = cleanPath(pathStr);
}

Path::Path(const std::string &pathStr)
Expand All @@ -85,13 +96,14 @@ Path::Path(const std::string &pathStr)

bool Path::isValid() const
{
// does not support UNC path

if (isEmpty())
return false;

// https://stackoverflow.com/a/31976060
#if defined(Q_OS_WIN)
if (isUNCPath)
return true;

QStringView view = m_pathStr;
if (hasDriveLetter(view))
view = view.mid(3);
Expand All @@ -109,7 +121,7 @@ bool Path::isValid() const

bool Path::isEmpty() const
{
return m_pathStr.isEmpty();
return isUNCPath ? false : m_pathStr.isEmpty();
}

bool Path::isAbsolute() const
Expand All @@ -128,15 +140,47 @@ bool Path::isRelative() const
return QDir::isRelativePath(m_pathStr);
}

#if defined(Q_OS_WIN)
bool Path::validateUNCPath() const
{
return validateUNCPath(data());
}

bool Path::validateUNCPath(const QString &pathStr) const
{
const QRegularExpression forbidden {u"[\\0-\\37:?\"*<>|:/]"_s}; // no drive letter allowed C:/
const QRegularExpression pattern{uR"(^\\(\\[^\\]+){2,}[\\]*$)"_s}; // need raw \\ to match back slash
return pattern.match(pathStr).hasMatch() && !pathStr.contains(forbidden);
}

QPair<QString, QString> Path::splitUNCPath(const QString &pathStr) const
{
// only call this only if validateUNCPath() returns true
int slashCount = 0;
int index = 0;
for (index = 0; index < pathStr.size(); ++index)
{
if (pathStr[index] == u'\\')
{
if (++slashCount == 4)
break;
}
}
return {pathStr.left(index), pathStr.right(pathStr.size() - index)};
}
#endif

bool Path::exists() const
{
return !isEmpty() && QFileInfo::exists(m_pathStr);
}

Path Path::rootItem() const
{
// does not support UNC path

#ifdef Q_OS_WIN
if (isUNCPath)
return createUnchecked(rootStr, true);
#endif
const int slashIndex = m_pathStr.indexOf(u'/');
if (slashIndex < 0)
return *this;
Expand All @@ -154,8 +198,14 @@ Path Path::rootItem() const

Path Path::parentPath() const
{
// does not support UNC path

#ifdef Q_OS_WIN
if (isUNCPath)
{
const int backSlashIndex = m_pathStr.lastIndexOf(u'\\');
const QString parent = rootStr + m_pathStr.left(backSlashIndex);
return createUnchecked(parent, true);
}
#endif
const int slashIndex = m_pathStr.lastIndexOf(u'/');
if (slashIndex == -1)
return {};
Expand Down Expand Up @@ -241,12 +291,12 @@ Path Path::removedExtension(const QStringView ext) const

QString Path::data() const
{
return m_pathStr;
return (isUNCPath ? (rootStr + m_pathStr) : m_pathStr);
}

QString Path::toString() const
{
return QDir::toNativeSeparators(m_pathStr);
return QDir::toNativeSeparators(data());
}

std::filesystem::path Path::toStdFsPath() const
Expand Down Expand Up @@ -333,11 +383,11 @@ void Path::addRootFolder(PathList &filePaths, const Path &rootFolder)
filePath = rootFolder / filePath;
}

Path Path::createUnchecked(const QString &pathStr)
Path Path::createUnchecked(const QString &pathStr, const bool isUNC)
{
Path path;
path.m_pathStr = pathStr;

path.isUNCPath = isUNC;
return path;
}

Expand Down
13 changes: 12 additions & 1 deletion src/base/path.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,12 @@ class Path final
bool isEmpty() const;
bool isAbsolute() const;
bool isRelative() const;
bool isUNCPath = false;

#if defined(Q_OS_WIN)
bool validateUNCPath() const;
bool validateUNCPath(const QString &pathStr) const;
#endif

bool exists() const;

Expand Down Expand Up @@ -86,9 +92,14 @@ class Path final
private:
// this constructor doesn't perform any checks
// so it's intended for internal use only
static Path createUnchecked(const QString &pathStr);
static Path createUnchecked(const QString &pathStr, const bool isUNCPath = false);

QString m_pathStr;

#if defined(Q_OS_WIN)
QString rootStr;
QPair<QString, QString> splitUNCPath(const QString &pathStr) const;
#endif
};

Q_DECLARE_METATYPE(Path)
Expand Down
11 changes: 11 additions & 0 deletions src/base/utils/fs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. bool Path::isUNCPath() const
    It will check if the path is a valid UNC path.
  2. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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

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

  3. 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 /

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Also, this could be a workaround: https://superuser.com/questions/210824/creating-a-symbolic-link-to-mapped-network-drive-in-windows
But I have not tested it.

Copy link
Contributor Author

@DoubleSpicy DoubleSpicy Jan 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made another iteration with @Chocobo1 's suggestions - storing the "root path" of a UNC path and a flag indicating a path is UNC or not, with a refined regex validator.

However It seem there is much more traps than I first expected, many existing logic assuming / as the seperator by default. The codebase is likely to get messy by adding UNC support to anywhere with /. This version still has some problem such as open the containing folder.

Btw, mounting UNC path as a network drive also works as it normalizes into normal path.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I lean towards keeping-as-it-is unless there is reasonable usecase to keep downloading new UNC path instead of mounting it as a network drive.

const auto wStrPath = path.data().toStdWString();
if (path.isUNCPath)
{
ULARGE_INTEGER FreeBytesAvailable = {0};
const BOOL ok = GetDiskFreeSpaceEx(wStrPath.c_str(), &FreeBytesAvailable, nullptr, nullptr);
if (ok)
return FreeBytesAvailable.QuadPart;
}
#endif

return QStorageInfo(path.data()).bytesAvailable();
}

Expand Down
32 changes: 32 additions & 0 deletions test/testpath.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ private slots:
QVERIFY(Path(uR"(\\?\C:\)"_s) == Path(std::string(R"(\\?\C:\)")));

QVERIFY(Path(uR"(\\?\C:\abc)"_s) == Path(std::string(R"(\\?\C:\abc)")));

QVERIFY(Path(uR"(\\nas01\drive)"_s) == Path(std::string(R"(\\nas01\drive)")));
QVERIFY(Path(uR"(\\nas01\drive\xxx)"_s) == Path(std::string(R"(\\nas01\drive\xxx)")));
QVERIFY(Path(uR"(\\nas01\drive\xxx\\)"_s) == Path(std::string(R"(\\nas01\drive\xxx)")));
#endif
}

Expand Down Expand Up @@ -109,11 +113,31 @@ private slots:
QCOMPARE(Path(u"<"_s).isValid(), false);
QCOMPARE(Path(u">"_s).isValid(), false);
QCOMPARE(Path(u"|"_s).isValid(), false);

QCOMPARE(Path(uR"(\\nas01\drive)"_s).isValid(), true);
QCOMPARE(Path(uR"(\\nas01\drive\xxx)"_s).isValid(), true);
QCOMPARE(Path(uR"(\\nas01\drive\xxx\\)"_s).isValid(), true);
#else
QCOMPARE(Path(u"\0"_s).isValid(), false);
#endif
}

#ifdef Q_OS_WIN
void testIsUNCPath() const
{
QCOMPARE(Path(uR"(\\)"_s).isUNCPath, false);
QCOMPARE(Path(uR"(\\\)"_s).isUNCPath, false);

QCOMPARE(Path(uR"(\\nas01\drive)"_s).isUNCPath, true);
QCOMPARE(Path(uR"(\\nas01\drive\)"_s).isUNCPath, true);
QCOMPARE(Path(uR"(\\nas01\drive\xxx)"_s).isUNCPath, true);
QCOMPARE(Path(uR"(\\nas01\drive\xxx\)"_s).isUNCPath, true);

QCOMPARE(Path(uR"(\\C:\\drive\xxx)"_s).isUNCPath, false);
QCOMPARE(Path(uR"(\\nas01\drive\?)"_s).isUNCPath, false);
QCOMPARE(Path(uR"(\\nas01\?\xxx)"_s).isUNCPath, false);
}
#endif
void testIsEmpty() const
{
QCOMPARE(Path().isEmpty(), true);
Expand Down Expand Up @@ -247,6 +271,10 @@ private slots:
QCOMPARE(Path(uR"(c:\)"_s).rootItem(), Path(uR"(c:/)"_s));
QCOMPARE(Path(uR"(c:\a)"_s).rootItem(), Path(uR"(c:\)"_s));
QCOMPARE(Path(uR"(c:\a\b)"_s).rootItem(), Path(uR"(c:\)"_s));

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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why \\nas01\drive is root item?

Copy link
Contributor Author

@DoubleSpicy DoubleSpicy Jan 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@xavier2k6 xavier2k6 Jan 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why \\nas01\drive is root item?

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

#else
QCOMPARE(Path(uR"(\a)"_s).rootItem(), Path(uR"(\a)"_s));
QCOMPARE(Path(uR"(\\a)"_s).rootItem(), Path(uR"(\\a)"_s));
Expand Down Expand Up @@ -280,6 +308,10 @@ private slots:
QCOMPARE(Path(uR"(c:\)"_s).parentPath(), Path());
QCOMPARE(Path(uR"(c:\a)"_s).parentPath(), Path(uR"(c:\)"_s));
QCOMPARE(Path(uR"(c:\a\b)"_s).parentPath(), Path(uR"(c:\a)"_s));

QCOMPARE(Path(uR"(\\nas01\drive)"_s).parentPath(), Path(uR"(\\nas01\drive)"_s));
QCOMPARE(Path(uR"(\\nas01\drive\xxx)"_s).parentPath(), Path(uR"(\\nas01\drive)"_s));
QCOMPARE(Path(uR"(\\nas01\drive\xxx\yyy)"_s).parentPath(), Path(uR"(\\nas01\drive\xxx)"_s));
#else
QCOMPARE(Path(uR"(\a)"_s).parentPath(), Path());
QCOMPARE(Path(uR"(\\a)"_s).parentPath(), Path());
Expand Down
Loading