-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Fix wrong drive format on Unix #107365
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.
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
Fix wrong drive format on Unix #107365
Changes from 10 commits
7e7f368
9fe1985
fec07ce
080825d
1171819
d43dd7b
3542fd8
1607833
2fd5836
49ac57e
eafc91b
4a17349
3505c12
c8c0a84
eba6e78
5da14ac
ab1e2f4
d52fe86
4bbceac
459d1a6
b5e2873
542c244
171c068
1ea7097
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,8 +50,68 @@ internal static int GetFormatInfoForMountPoint(string name, out DriveType type) | |
| return GetFormatInfoForMountPoint(name, out _, out type); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Retrieves format information for the specified mount point. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// This method uses a two-step approach to retrieve filesystem information: | ||
| /// 1. On Linux systems, it first attempts to read from `/proc/self/mountinfo`. | ||
| /// 2. If step 1 fails or on non-Linux systems, it falls back to a P/Invoke call. | ||
| /// | ||
| /// The `/proc/self/mountinfo` approach is preferred on Linux because | ||
| /// it's more reliable when procfs is available and functioning correctly. | ||
| /// | ||
| /// For other systems: | ||
| /// - SunOS and similar: procfs provides a binary interface, not suitable for this method. | ||
| /// - macOS: procfs is not available. | ||
| /// - FreeBSD: procfs is optional and not enabled by default. | ||
| /// | ||
| /// The method uses try/catch blocks to ensure robustness, even though `/proc/self/mountinfo` | ||
| /// should be specific to each process according to kernel documentation. | ||
| /// </remarks> | ||
| /// <param name="name">The mount point name to query.</param> | ||
| /// <param name="format">Output parameter for the filesystem format.</param> | ||
| /// <param name="type">Output parameter for the drive type.</param> | ||
| /// <returns>0 if successful, otherwise an error code.</returns> | ||
| private static unsafe int GetFormatInfoForMountPoint(string name, out string format, out DriveType type) | ||
| { | ||
| #if TARGET_LINUX | ||
| try | ||
| { | ||
| const string mountInfoFilePath = "/proc/self/mountinfo"; | ||
| var mountInfoFileContent = File.ReadAllLines(mountInfoFilePath); | ||
| foreach (var line in mountInfoFileContent) | ||
| { | ||
| var parser = new StringParser(line, ' '); | ||
MojtabaTajik marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| // Skip fields we don't care about (Fields 1-4) | ||
| parser.MoveNext(); // Skip Mount ID | ||
| parser.MoveNext(); // Skip Parent ID | ||
| parser.MoveNext(); // Skip Major:Minor | ||
| parser.MoveNext(); // Skip Root | ||
|
|
||
| // Get the mount point (Field 5) | ||
| string mountPoint = parser.MoveAndExtractNext(); | ||
|
|
||
| // Skip to the separator which is end of optional fields (Field 8) | ||
| while (parser.MoveAndExtractNext() != "-") | ||
| { | ||
| } | ||
|
|
||
| // Get filesystem type (Field 9) | ||
| string filesystemType = parser.MoveAndExtractNext(); | ||
|
|
||
| if (mountPoint.Equals(name, StringComparison.Ordinal)) | ||
MojtabaTajik marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| { | ||
| format = filesystemType; | ||
| type = GetDriveType(filesystemType); | ||
| return 0; | ||
| } | ||
| } | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we went over the lines of |
||
| catch { /* ignored */ } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the format is well documented, we shouldn't need a catch all. We can place this under
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see this was discussed in #107365 (comment). Personally, I think the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason why all/most procfs usage in libs are using try-catch pattern is because anything could go wrong (#62513). Also see #74316 (comment) and #52753 to read on unreliability issues attached with FileSystemInfo.Exists out in the wild.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are some reasons why we don't expect these to occur for Anyway, my main concern here is that a generic catch all masks failures in the parsing. Perhaps we can filter for IOException/UnauthorizedAccessException. These are the exceptions we might expect when not being able to read the procfs file.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BTW, this approach is similar to what has been used in networking info https://github.com/dotnet/runtime/pull/63696/files#diff-deb3e580b54bccfac2148f60c5d90c6e88b55b508948052e9cf2d1e5421f6ff5R21. Maybe adding a Debug.Fail in catch body for unexpected parsing issues during the development/testing is enough so implementation bugs don't go undetected? |
||
| #endif | ||
|
|
||
| byte* formatBuffer = stackalloc byte[MountPointFormatBufferSizeInBytes]; // format names should be small | ||
| long numericFormat; | ||
| int result = GetFormatInfoForMountPoint(name, formatBuffer, MountPointFormatBufferSizeInBytes, &numericFormat); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to have some parsing tests for this, similar to
runtime/src/libraries/Common/tests/Tests/Interop/procfsTests.cs
Line 36 in 0154a2f