Skip to content
Closed
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
7e7f368
Fix wrong drive format on Unix
MojtabaTajik Sep 4, 2024
9fe1985
Merge branch 'main' into FixWrongDriveFormatOnUnix
MojtabaTajik Sep 4, 2024
fec07ce
Add support for reading filesystem type from /proc/self/mountinfo on …
MojtabaTajik Sep 5, 2024
080825d
Fix whitespaces and replace tabs with 4 spaces
MojtabaTajik Sep 5, 2024
1171819
Remove pre-processor directives indention
MojtabaTajik Sep 5, 2024
d43dd7b
Fix whitespaces
MojtabaTajik Sep 5, 2024
3542fd8
Update src/libraries/Common/src/Interop/Unix/System.Native/Interop.Mo…
MojtabaTajik Sep 5, 2024
1607833
fix indention
MojtabaTajik Sep 5, 2024
2fd5836
Update src/libraries/Common/src/Interop/Unix/System.Native/Interop.Mo…
MojtabaTajik Sep 6, 2024
49ac57e
Update src/libraries/Common/src/Interop/Unix/System.Native/Interop.Mo…
MojtabaTajik Sep 6, 2024
eafc91b
Use actual type instead of var
MojtabaTajik Sep 6, 2024
4a17349
Update src/libraries/Common/src/Interop/Unix/System.Native/Interop.Mo…
MojtabaTajik Sep 6, 2024
3505c12
Update src/libraries/Common/src/Interop/Unix/System.Native/Interop.Mo…
MojtabaTajik Sep 6, 2024
c8c0a84
Simplify checks
MojtabaTajik Sep 6, 2024
eba6e78
add new line
MojtabaTajik Sep 6, 2024
5da14ac
fix spaces
MojtabaTajik Sep 6, 2024
ab1e2f4
Revert "fix spaces"
MojtabaTajik Sep 6, 2024
d52fe86
Revert "add new line"
MojtabaTajik Sep 6, 2024
4bbceac
Revert "Simplify checks"
MojtabaTajik Sep 6, 2024
459d1a6
Revert "Update src/libraries/Common/src/Interop/Unix/System.Native/In…
MojtabaTajik Sep 6, 2024
b5e2873
Update src/libraries/Common/src/Interop/Unix/System.Native/Interop.Mo…
MojtabaTajik Sep 6, 2024
542c244
Skip to mount point
MojtabaTajik Sep 6, 2024
171c068
Update comment
MojtabaTajik Sep 6, 2024
1ea7097
Use MemoryExtensions.Equals
MojtabaTajik Sep 6, 2024
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
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,65 @@ 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)
{
Copy link
Member

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

public void ParseValidStatFiles_Success(
.

#if TARGET_LINUX
try
{
using StreamReader reader = new("/proc/self/mountinfo");

string rawLine;
while ((rawLine = reader.ReadLine()) is not null)
{
ReadOnlySpan<char> line = rawLine.AsSpan();
MemoryExtensions.SpanSplitEnumerator<char> fields = line.Split(' ');

// Skip fields we don't care about (Fields 1-4)
fields.MoveNext(); // Skip Mount ID
Copy link
Member

@tmds tmds Sep 16, 2024

Choose a reason for hiding this comment

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

nit: I find the Skip in this comment misleading. The call to MoveNext puts us at the Mount ID, not past it, right?

Maybe you can drop the Skip.

fields.MoveNext(); // Mount ID
...

fields.MoveNext(); // Skip Parent ID
fields.MoveNext(); // Skip Major:Minor
fields.MoveNext(); // Skip Root
fields.MoveNext(); // Skip to MountPoint field
Copy link
Member

Choose a reason for hiding this comment

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

This PR should include additional changes so we use the MountPoint field as the Name/RootDirectory for DriveInfo.

Copy link
Member

Choose a reason for hiding this comment

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

That is: for the instances returned by DriveInfo.GetDrives.


if (!MemoryExtensions.Equals(line[fields.Current], name, StringComparison.Ordinal)) continue;

// Skip to the separator which is end of optional fields (Field 8)
while (fields.MoveNext() && !MemoryExtensions.Equals(line[fields.Current], "-", StringComparison.Ordinal));

fields.MoveNext();
format = line[fields.Current].ToString();

fields.MoveNext();
type = GetDriveType(line[fields.Current].ToString());
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we be passing format to GetDriveType instead of reading the next field?

man proc has this example line:

36 35 98:0 /mnt1 /mnt2 rw,noatime master:1 - ext3 /dev/root rw,errors=continue


Copy link
Member

Choose a reason for hiding this comment

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

There doesn't seem to be a matching closing } for the while loop. If CI doesn't complain, then TARGET_LINUX may not be set.

return 0;
}
Copy link
Member

Choose a reason for hiding this comment

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

If we went over the lines of mountinfo and didn't find a match, there's no need to do the PInvoke. We can just return immediately.

catch { /* ignored */ }
Copy link
Member

Choose a reason for hiding this comment

The 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 if (File.Exists("/proc/self/mountinfo")) to handle the case where proc is unavailable.

Copy link
Member

Choose a reason for hiding this comment

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

I see this was discussed in #107365 (comment).

Personally, I think the File.Exists should be fine, but I defer to the maintainers' preferences.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

The 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 /proc/self/mountinfo

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.

Copy link
Member

Choose a reason for hiding this comment

The 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
Comment on lines +78 to +110
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#if TARGET_LINUX
try
{
using StreamReader reader = new("/proc/self/mountinfo");
string rawLine;
while ((rawLine = reader.ReadLine()) is not null)
{
ReadOnlySpan<char> line = rawLine.AsSpan();
MemoryExtensions.SpanSplitEnumerator<char> fields = line.Split(' ');
// Skip fields we don't care about (Fields 1-4)
fields.MoveNext(); // Skip Mount ID
fields.MoveNext(); // Skip Parent ID
fields.MoveNext(); // Skip Major:Minor
fields.MoveNext(); // Skip Root
fields.MoveNext(); // Skip to MountPoint field
if (!MemoryExtensions.Equals(line[fields.Current], name, StringComparison.Ordinal)) continue;
// Skip to the separator which is end of optional fields (Field 8)
while (fields.MoveNext() && !MemoryExtensions.Equals(line[fields.Current], "-", StringComparison.Ordinal));
fields.MoveNext();
format = line[fields.Current].ToString();
fields.MoveNext();
type = GetDriveType(line[fields.Current].ToString());
return 0;
}
catch { /* ignored */ }
#endif
if (OperatingSystem.IsLinux())
{
try
{
using StreamReader reader = new("/proc/self/mountinfo");
string? rawLine;
while ((rawLine = reader.ReadLine()) is not null)
{
ReadOnlySpan<char> line = rawLine.AsSpan();
MemoryExtensions.SpanSplitEnumerator<char> fields = line.Split(' ');
// Skip fields we don't care about (Fields 1-4)
fields.MoveNext(); // Skip Mount ID
fields.MoveNext(); // Skip Parent ID
fields.MoveNext(); // Skip Major:Minor
fields.MoveNext(); // Skip Root
fields.MoveNext(); // Skip to MountPoint field
if (!MemoryExtensions.Equals(line[fields.Current], name, StringComparison.Ordinal)) continue;
// Skip to the separator which is end of optional fields (Field 8)
while (fields.MoveNext() && !MemoryExtensions.Equals(line[fields.Current], "-", StringComparison.Ordinal));
fields.MoveNext();
format = line[fields.Current].ToString();
fields.MoveNext();
type = GetDriveType(line[fields.Current].ToString());
return 0;
}
}
catch { /* ignored */ }
}

I just noticed that this wasn't building on Linux because only CoreLib has TARGET_LINUX type of constants defined. Also, there is a syntax error try and while blocks have only one closing }.

OperatingSystem.IsLinux() block will be optimized out on non-Linux by the JIT (since it returns a constant bool).

Copy link
Member

@tmds tmds Sep 24, 2024

Choose a reason for hiding this comment

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

I had noticed the syntax error and based on that assumed TARGET_LINUX must not be set (#107365 (comment)).

if (OperatingSystem.IsLinux())

Yes, this is the way!


byte* formatBuffer = stackalloc byte[MountPointFormatBufferSizeInBytes]; // format names should be small
long numericFormat;
int result = GetFormatInfoForMountPoint(name, formatBuffer, MountPointFormatBufferSizeInBytes, &numericFormat);
Expand Down