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.

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

Address some System.Formats.Tar TODOs (infra and syscalls) #69107

Merged
merged 22 commits into from
May 17, 2022
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
c178925
Fix Design time build errors by removing src project as dependency of…
carlossanlop May 4, 2022
9f020bd
Add Browser to target platform identifiers. Ensure Browser can consum…
carlossanlop May 4, 2022
c4d444b
Remove nullable enable from csproj since it's now default
carlossanlop May 5, 2022
354e40a
Use FileStream constructor with FileMode.CreateNew to detect and thro…
carlossanlop May 9, 2022
d04ec6b
No error checking on syscalls that do not set errno.
carlossanlop May 9, 2022
9cb26f1
Add RDev field in FileStatus and retrieve it with stat/lstat so devma…
carlossanlop May 10, 2022
d94536a
Simplify some File.Open calls with direct FileStream constructor call…
carlossanlop May 10, 2022
cdbbddf
Implement and consume p/invokes to retrieve uname from uid and gname …
carlossanlop May 10, 2022
e343a3b
size_t variables should not be checked for negative values
carlossanlop May 11, 2022
1af51df
FileStream calls simplified to File.OpenRead
carlossanlop May 11, 2022
d845d56
Remove check for RDev > 0
carlossanlop May 11, 2022
94d2cf0
Use dictionary to preserve repeated unames and gnames mapped to uids …
carlossanlop May 11, 2022
90cc58c
Missing documentation in pal_uid.h new PALEXPORT methods.
carlossanlop May 11, 2022
fe9a927
Adjust syscalls to thread-safe ones. Start with stackalloc, then use …
carlossanlop May 12, 2022
a8f5cf6
Make dicts readonly and non-nullable, use TryGetValue
carlossanlop May 12, 2022
498c084
Merge branch 'main' into TarImprovements
carlossanlop May 13, 2022
7b4e7b5
Reuse 'GetNameFromUid' from pal_networking.c, move to pal_uid.c, use …
carlossanlop May 13, 2022
a9090b4
Remove unnecessary comment
carlossanlop May 13, 2022
7c01996
Put TargetFrameworks back in the first position of the PropertyGroup.
carlossanlop May 13, 2022
8de50ad
Address eerhardt suggestions
carlossanlop May 13, 2022
31ab76b
Update src/libraries/System.Formats.Tar/tests/TarWriter/TarWriter.Wri…
carlossanlop May 17, 2022
67fe812
Clarify in pal_uid.h methods comments that new memory is returned
carlossanlop May 17, 2022
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 @@ -23,6 +23,6 @@ internal static int CreateCharacterDevice(string pathName, uint mode, uint major
private static partial int MkNod(string pathName, uint mode, uint major, uint minor);

[LibraryImport(Libraries.SystemNative, EntryPoint = "SystemNative_GetDeviceIdentifiers", SetLastError = true)]
internal static unsafe partial int GetDeviceIdentifiers(ulong dev, uint* majorNumber, uint* minorNumber);
internal static unsafe partial void GetDeviceIdentifiers(ulong dev, uint* majorNumber, uint* minorNumber);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Runtime.InteropServices;
using System.Buffers;
using System.Text;
using System;
using System.Collections.Generic;
using System.Reflection;

internal static partial class Interop
{
internal static partial class Sys
{
/// <summary>
/// Gets the group name associated to the specified group ID.
/// </summary>
/// <param name="gid">The group ID.</param>
/// <returns>On success, return a string with the group name. On failure, throws an IOException.</returns>
internal static string GetGroupName(uint gid) => GetGroupNameInternal(gid) ?? throw GetIOException(GetLastErrorInfo());

/// <summary>
/// Gets the user name associated to the specified user ID.
/// </summary>
/// <param name="uid">The user ID.</param>
/// <returns>On success, return a string with the user name. On failure, throws an IOException.</returns>
internal static string GetUserName(uint uid) => GetUserNameInternal(uid) ?? throw GetIOException(GetLastErrorInfo());

[LibraryImport(Libraries.SystemNative, EntryPoint = "SystemNative_GetGroupName", StringMarshalling = StringMarshalling.Utf8, SetLastError = true)]
private static unsafe partial string? GetGroupNameInternal(uint uid);

[LibraryImport(Libraries.SystemNative, EntryPoint = "SystemNative_GetUserName", StringMarshalling = StringMarshalling.Utf8, SetLastError = true)]
private static unsafe partial string? GetUserNameInternal(uint uid);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ internal struct FileStatus
internal long BirthTime;
internal long BirthTimeNsec;
internal long Dev;
internal long RDev;
internal long Ino;
internal uint UserFlags;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFrameworks>$(NetCoreAppCurrent)-windows;$(NetCoreAppCurrent)-Unix;$(NetCoreAppCurrent)</TargetFrameworks>
<TargetFrameworks>$(NetCoreAppCurrent)-windows;$(NetCoreAppCurrent)-Unix;$(NetCoreAppCurrent)-Browser;$(NetCoreAppCurrent)</TargetFrameworks>
carlossanlop marked this conversation as resolved.
Show resolved Hide resolved
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
</PropertyGroup>

Expand Down Expand Up @@ -57,10 +57,14 @@
<Compile Include="$(CommonPath)Interop\Unix\Interop.Errors.cs" Link="Common\Interop\Unix\System.Native\Interop.Errors.cs" />
<Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.DeviceFiles.cs" Link="Common\Interop\Unix\System.Native\Interop.DeviceFiles.cs" />
<Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.FChMod.cs" Link="Common\Interop\Unix\System.Native\Interop.FChMod.cs" />
<Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.GroupNameUserName.cs" Link="Common\Interop\Unix\System.Native\Interop.GroupNameUserName.cs" />
<Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.Link.cs" Link="Common\Interop\Unix\System.Native\Interop.Link.cs" />
<Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.MkFifo.cs" Link="Common\Interop\Unix\System.Native\Interop.MkFifo.cs" />
<Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.Permissions.cs" Link="Common\Interop\Unix\Interop.Permissions.cs" />
<Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.Stat.cs" Link="Common\Interop\Unix\Interop.Stat.cs" />
</ItemGroup>
<!-- Unix and Browser specific files -->
<ItemGroup Condition="'$(TargetPlatformIdentifier)' == 'Unix' or '$(TargetPlatformIdentifier)' == 'Browser'">
<Compile Include="$(CommonPath)System\IO\Archiving.Utils.Unix.cs" Link="Common\System\IO\Archiving.Utils.Unix.cs" />
</ItemGroup>
<ItemGroup>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -431,7 +431,7 @@ private void ExtractAsRegularFile(string destinationFileName)
{
Debug.Assert(!Path.Exists(destinationFileName));

FileStreamOptions fileStreamOptions = new FileStreamOptions()
FileStreamOptions fileStreamOptions = new()
{
Access = FileAccess.Write,
Mode = FileMode.CreateNew,
Expand Down
18 changes: 3 additions & 15 deletions src/libraries/System.Formats.Tar/src/System/Formats/Tar/TarFile.cs
Original file line number Diff line number Diff line change
Expand Up @@ -72,12 +72,8 @@ public static void CreateFromDirectory(string sourceDirectoryName, string destin
throw new DirectoryNotFoundException(string.Format(SR.IO_PathNotFound_Path, sourceDirectoryName));
}

if (Path.Exists(destinationFileName))
{
throw new IOException(string.Format(SR.IO_FileExists_Name, destinationFileName));
}

using FileStream fs = File.Create(destinationFileName, bufferSize: 0x1000, FileOptions.None);
// Throws if the destination file exists
using FileStream fs = new(destinationFileName, FileMode.CreateNew, FileAccess.Write);

CreateFromDirectoryInternal(sourceDirectoryName, fs, includeBaseDirectory, leaveOpen: false);
}
Expand Down Expand Up @@ -170,15 +166,7 @@ public static void ExtractToDirectory(string sourceFileName, string destinationD
throw new DirectoryNotFoundException(string.Format(SR.IO_PathNotFound_Path, destinationDirectoryName));
}

FileStreamOptions fileStreamOptions = new()
{
Access = FileAccess.Read,
BufferSize = 0x1000,
Mode = FileMode.Open,
Share = FileShare.Read
};

using FileStream archive = File.Open(sourceFileName, fileStreamOptions);
using FileStream archive = File.OpenRead(sourceFileName);

ExtractToDirectoryInternal(archive, destinationDirectoryName, overwriteFiles, leaveOpen: false);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections.Generic;
using System.Diagnostics;
using System.IO;

Expand All @@ -9,6 +10,9 @@ namespace System.Formats.Tar
// Unix specific methods for the TarWriter class.
public sealed partial class TarWriter : IDisposable
{
private readonly Dictionary<uint, string> _userIdentifiers = new Dictionary<uint, string>();
private readonly Dictionary<uint, string> _groupIdentifiers = new Dictionary<uint, string>();

// Unix specific implementation of the method that reads an entry from disk and writes it into the archive stream.
partial void ReadFileFromDiskAndWriteToArchiveStreamAsEntry(string fullPath, string entryName)
{
Expand Down Expand Up @@ -41,13 +45,13 @@ partial void ReadFileFromDiskAndWriteToArchiveStreamAsEntry(string fullPath, str
_ => throw new FormatException(string.Format(SR.TarInvalidFormat, Format)),
};

if ((entryType is TarEntryType.BlockDevice or TarEntryType.CharacterDevice) && status.Dev > 0)
if (entryType is TarEntryType.BlockDevice or TarEntryType.CharacterDevice)
{
uint major;
uint minor;
unsafe
{
Interop.CheckIo(Interop.Sys.GetDeviceIdentifiers((ulong)status.Dev, &major, &minor));
Interop.Sys.GetDeviceIdentifiers((ulong)status.RDev, &major, &minor);
}

entry._header._devMajor = (int)major;
Expand All @@ -60,12 +64,23 @@ partial void ReadFileFromDiskAndWriteToArchiveStreamAsEntry(string fullPath, str

entry._header._mode = (status.Mode & 4095); // First 12 bits

entry.Uid = (int)status.Uid;
entry.Gid = (int)status.Gid;
// Uid and UName
entry._header._uid = (int)status.Uid;
if (!_userIdentifiers.TryGetValue(status.Uid, out string? uName))
{
uName = Interop.Sys.GetUserName(status.Uid);
_userIdentifiers.Add(status.Uid, uName);
carlossanlop marked this conversation as resolved.
Show resolved Hide resolved
}
entry._header._uName = uName;

// TODO: Add these p/invokes https://github.com/dotnet/runtime/issues/68230
entry._header._uName = "";// Interop.Sys.GetUName();
entry._header._gName = "";// Interop.Sys.GetGName();
// Gid and GName
entry._header._gid = (int)status.Gid;
if (!_groupIdentifiers.TryGetValue(status.Gid, out string? gName))
{
gName = Interop.Sys.GetGroupName(status.Gid);
_groupIdentifiers.Add(status.Gid, gName);
}
entry._header._gName = gName;

if (entry.EntryType == TarEntryType.SymbolicLink)
{
Expand All @@ -74,16 +89,8 @@ partial void ReadFileFromDiskAndWriteToArchiveStreamAsEntry(string fullPath, str

if (entry.EntryType is TarEntryType.RegularFile or TarEntryType.V7RegularFile)
{
FileStreamOptions options = new()
{
Mode = FileMode.Open,
Access = FileAccess.Read,
Share = FileShare.Read,
Options = FileOptions.None
};

Debug.Assert(entry._header._dataStream == null);
entry._header._dataStream = File.Open(fullPath, options);
entry._header._dataStream = File.OpenRead(fullPath);
}

WriteEntry(entry);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
<Compile Include="$(CommonPath)Interop\Unix\Interop.IOErrors.cs" Link="Common\Interop\Unix\Interop.IOErrors.cs" />
<Compile Include="$(CommonPath)Interop\Unix\Interop.Libraries.cs" Link="Common\Interop\Unix\Interop.Libraries.cs" />
<Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.DeviceFiles.cs" Link="Common\Interop\Unix\System.Native\Interop.DeviceFiles.cs" />
<Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.GroupNameUserName.cs" Link="Common\Interop\Unix\System.Native\Interop.GroupNameUserName.cs" />
<Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.Link.cs" Link="Common\Interop\Unix\System.Native\Interop.Link.cs" />
<Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.MkFifo.cs" Link="Common\Interop\Unix\System.Native\Interop.MkFifo.cs" />
<Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.Stat.cs" Link="Common\Interop\Unix\System.Native\Interop.Stat.cs" />
Expand All @@ -66,7 +67,4 @@
<ItemGroup>
<PackageReference Include="System.Formats.Tar.TestData" Version="$(SystemFormatsTarTestDataVersion)" />
</ItemGroup>
<ItemGroup>
<ProjectReference Include="..\src\System.Formats.Tar.csproj" />
</ItemGroup>
</Project>
Original file line number Diff line number Diff line change
Expand Up @@ -100,10 +100,7 @@ public void Extract_Archive_File_OverwriteFalse()

string filePath = Path.Join(destination.Path, "file.txt");

using (StreamWriter writer = File.CreateText(filePath))
{
writer.WriteLine("My existence should cause an exception");
}
File.Create(filePath).Dispose();

Assert.Throws<IOException>(() => TarFile.ExtractToDirectory(sourceArchiveFileName, destination.Path, overwriteFiles: false));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -713,13 +713,8 @@ private void Verify_Archive_BlockDevice(PosixTarEntry blockDevice, IReadOnlyDict
Assert.True(blockDevice.ModificationTime > DateTimeOffset.UnixEpoch);
Assert.Equal(expectedFileName, blockDevice.Name);
Assert.Equal(AssetUid, blockDevice.Uid);

// TODO: Figure out why the numbers don't match https://github.com/dotnet/runtime/issues/68230
// Assert.Equal(AssetBlockDeviceMajor, blockDevice.DeviceMajor);
// Assert.Equal(AssetBlockDeviceMinor, blockDevice.DeviceMinor);
// Remove these two temporary checks when the above is fixed
Assert.True(blockDevice.DeviceMajor > 0);
Assert.True(blockDevice.DeviceMinor > 0);
Assert.Equal(AssetBlockDeviceMajor, blockDevice.DeviceMajor);
Assert.Equal(AssetBlockDeviceMinor, blockDevice.DeviceMinor);
Assert.Equal(AssetGName, blockDevice.GroupName);
Assert.Equal(AssetUName, blockDevice.UserName);

Expand Down Expand Up @@ -749,13 +744,8 @@ private void Verify_Archive_CharacterDevice(PosixTarEntry characterDevice, IRead
Assert.True(characterDevice.ModificationTime > DateTimeOffset.UnixEpoch);
Assert.Equal(expectedFileName, characterDevice.Name);
Assert.Equal(AssetUid, characterDevice.Uid);

// TODO: Figure out why the numbers don't match https://github.com/dotnet/runtime/issues/68230
//Assert.Equal(AssetBlockDeviceMajor, characterDevice.DeviceMajor);
//Assert.Equal(AssetBlockDeviceMinor, characterDevice.DeviceMinor);
// Remove these two temporary checks when the above is fixed
Assert.True(characterDevice.DeviceMajor > 0);
Assert.True(characterDevice.DeviceMinor > 0);
Assert.Equal(AssetCharacterDeviceMajor, characterDevice.DeviceMajor);
Assert.Equal(AssetCharacterDeviceMinor, characterDevice.DeviceMinor);
Assert.Equal(AssetGName, characterDevice.GroupName);
Assert.Equal(AssetUName, characterDevice.UserName);

Expand Down
9 changes: 1 addition & 8 deletions src/libraries/System.Formats.Tar/tests/TarTestsBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -99,15 +99,8 @@ protected static string GetTarFilePath(CompressionMethod compressionMethod, Test
protected static MemoryStream GetTarMemoryStream(CompressionMethod compressionMethod, TestTarFormat format, string testCaseName)
{
string path = GetTarFilePath(compressionMethod, format, testCaseName);
FileStreamOptions options = new()
{
Access = FileAccess.Read,
Mode = FileMode.Open,
Share = FileShare.Read

};
MemoryStream ms = new();
using (FileStream fs = new FileStream(path, options))
using (FileStream fs = File.OpenRead(path))
{
fs.CopyTo(ms);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,10 +91,8 @@ public void Add_BlockDevice(TarFormat format)

VerifyPlatformSpecificMetadata(blockDevicePath, entry);

// TODO: Fix how these values are collected, the numbers don't match even though https://github.com/dotnet/runtime/issues/68230
// they come from stat's dev and from the major/minor syscalls
// Assert.Equal(TestBlockDeviceMajor, entry.DeviceMajor);
// Assert.Equal(TestBlockDeviceMinor, entry.DeviceMinor);
Assert.Equal(TestBlockDeviceMajor, entry.DeviceMajor);
Assert.Equal(TestBlockDeviceMinor, entry.DeviceMinor);

Assert.Null(reader.GetNextEntry());
}
Expand Down Expand Up @@ -138,10 +136,8 @@ public void Add_CharacterDevice(TarFormat format)

VerifyPlatformSpecificMetadata(characterDevicePath, entry);

// TODO: Fix how these values are collected, the numbers don't match even though https://github.com/dotnet/runtime/issues/68230
// they come from stat's dev and from the major/minor syscalls
// Assert.Equal(TestCharacterDeviceMajor, entry.DeviceMajor);
// Assert.Equal(TestCharacterDeviceMinor, entry.DeviceMinor);
Assert.Equal(TestCharacterDeviceMajor, entry.DeviceMajor);
Assert.Equal(TestCharacterDeviceMinor, entry.DeviceMinor);

Assert.Null(reader.GetNextEntry());
}
Expand All @@ -161,8 +157,11 @@ partial void VerifyPlatformSpecificMetadata(string filePath, TarEntry entry)

if (entry is PosixTarEntry posix)
{
Assert.Equal(DefaultGName, posix.GroupName);
Assert.Equal(DefaultUName, posix.UserName);
string gname = Interop.Sys.GetGroupName(status.Gid);
string uname = Interop.Sys.GetUserName(status.Uid);

Assert.Equal(gname, posix.GroupName);
Assert.Equal(uname, posix.UserName);

if (entry.EntryType is not TarEntryType.BlockDevice and not TarEntryType.CharacterDevice)
{
Expand Down
2 changes: 2 additions & 0 deletions src/native/libs/System.Native/entrypoints.c
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,8 @@ static const Entry s_sysNative[] =
DllImportEntry(SystemNative_GetEnv)
DllImportEntry(SystemNative_GetEnviron)
DllImportEntry(SystemNative_FreeEnviron)
DllImportEntry(SystemNative_GetGroupName)
DllImportEntry(SystemNative_GetUserName)
};

EXTERN_C const void* SystemResolveDllImport(const char* name);
Expand Down
9 changes: 2 additions & 7 deletions src/native/libs/System.Native/pal_io.c
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ c_static_assert(PAL_IN_ISDIR == IN_ISDIR);
static void ConvertFileStatus(const struct stat_* src, FileStatus* dst)
{
dst->Dev = (int64_t)src->st_dev;
dst->RDev = (int64_t)src->st_rdev;
dst->Ino = (int64_t)src->st_ino;
dst->Flags = FILESTATUS_FLAGS_NONE;
dst->Mode = (int32_t)src->st_mode;
Expand Down Expand Up @@ -770,23 +771,17 @@ int32_t SystemNative_SymLink(const char* target, const char* linkPath)
return result;
}

int32_t SystemNative_GetDeviceIdentifiers(uint64_t dev, uint32_t* majorNumber, uint32_t* minorNumber)
void SystemNative_GetDeviceIdentifiers(uint64_t dev, uint32_t* majorNumber, uint32_t* minorNumber)
{
dev_t castedDev = (dev_t)dev;
*majorNumber = (uint32_t)major(castedDev);
*minorNumber = (uint32_t)minor(castedDev);
return ConvertErrorPlatformToPal(errno);
}

int32_t SystemNative_MkNod(const char* pathName, uint32_t mode, uint32_t major, uint32_t minor)
{
dev_t dev = (dev_t)makedev(major, minor);

if (errno > 0)
{
return -1;
}

int32_t result;
while ((result = mknod(pathName, (mode_t)mode, dev)) < 0 && errno == EINTR);
return result;
Expand Down
4 changes: 2 additions & 2 deletions src/native/libs/System.Native/pal_io.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ typedef struct
int64_t BirthTime; // time the file was created
int64_t BirthTimeNsec; // nanosecond part
int64_t Dev; // ID of the device containing the file
int64_t RDev; // ID of the device if it is a special file
int64_t Ino; // inode number of the file
uint32_t UserFlags; // user defined flags
} FileStatus;
Expand Down Expand Up @@ -543,9 +544,8 @@ PALEXPORT int32_t SystemNative_SymLink(const char* target, const char* linkPath)

/**
* Given a device ID, extracts the major and minor and components and returns them.
* Return 0 on success; otherwise, returns -1 and errno is set.
*/
PALEXPORT int32_t SystemNative_GetDeviceIdentifiers(uint64_t dev, uint32_t* majorNumber, uint32_t* minorNumber);
PALEXPORT void SystemNative_GetDeviceIdentifiers(uint64_t dev, uint32_t* majorNumber, uint32_t* minorNumber);

/**
* Creates a special or ordinary file.
Expand Down
Loading