address code review feedback

dotnet · Jun 11, 2021 · c87354c · c87354c
1 parent 05e65eb
commit c87354c
Show file tree

Hide file tree

Showing 5 changed files with 66 additions and 73 deletions.
diff --git a/src/libraries/System.IO.FileSystem/tests/RandomAccess/SectorAlignedMemory.Windows.cs b/src/libraries/System.IO.FileSystem/tests/RandomAccess/SectorAlignedMemory.Windows.cs
@@ -22,11 +22,6 @@ private unsafe SectorAlignedMemory(void* memory, int length)
             this.length = length;
         }
 
-        ~SectorAlignedMemory()
-        {
-            Dispose(false);
-        }
-
         public static unsafe SectorAlignedMemory<T> Allocate(int length)
         {
             void* memory = VirtualAlloc(

diff --git a/...ibraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs b/...ibraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs
@@ -66,7 +66,7 @@ private static IntPtr NtCreateFile(string fullPath, FileMode mode, FileAccess ac
             }
             else
             {
-                var vsb = new ValueStringBuilder(stackalloc char[1024]);
+                var vsb = new ValueStringBuilder(stackalloc char[256]);
                 vsb.Append(MandatoryNtPrefix);
 
                 if (fullPath.StartsWith(@"\\?\", StringComparison.Ordinal)) // NtCreateFile does not support "\\?\" prefix, only "\??\"

diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Unix.cs b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Unix.cs
@@ -39,17 +39,17 @@ private static unsafe long ReadScatterAtOffset(SafeFileHandle handle, IReadOnlyL
             MemoryHandle[] handles = new MemoryHandle[buffers.Count];
             Span<Interop.Sys.IOVector> vectors = buffers.Count <= IovStackThreshold ? stackalloc Interop.Sys.IOVector[IovStackThreshold] : new Interop.Sys.IOVector[buffers.Count];
 
-            for (int i = 0; i < buffers.Count; i++)
-            {
-                Memory<byte> buffer = buffers[i];
-                MemoryHandle memoryHandle = buffer.Pin();
-                vectors[i] = new Interop.Sys.IOVector { Base = (byte*)memoryHandle.Pointer, Count = (UIntPtr)buffer.Length };
-                handles[i] = memoryHandle;
-            }
-
             long result;
             try
             {
+                for (int i = 0; i < buffers.Count; i++)
+                {
+                    Memory<byte> buffer = buffers[i];
+                    MemoryHandle memoryHandle = buffer.Pin();
+                    vectors[i] = new Interop.Sys.IOVector { Base = (byte*)memoryHandle.Pointer, Count = (UIntPtr)buffer.Length };
+                    handles[i] = memoryHandle;
+                }
+
                 fixed (Interop.Sys.IOVector* pinnedVectors = &MemoryMarshal.GetReference(vectors))
                 {
                     result = Interop.Sys.PReadV(handle, pinnedVectors, buffers.Count, fileOffset);
@@ -79,7 +79,7 @@ private static ValueTask<int> ReadAtOffsetAsync(SafeFileHandle handle, Memory<by
         private static ValueTask<long> ReadScatterAtOffsetAsync(SafeFileHandle handle, IReadOnlyList<Memory<byte>> buffers,
             long fileOffset, CancellationToken cancellationToken)
         {
-            return new ValueTask<long>(Task.Factory.StartNew(state =>
+            return new ValueTask<long>(Task.Factory.StartNew(static state =>
             {
                 var args = ((SafeFileHandle handle, IReadOnlyList<Memory<byte>> buffers, long fileOffset))state!;
                 return ReadScatterAtOffset(args.handle, args.buffers, args.fileOffset);
@@ -101,17 +101,17 @@ private static unsafe long WriteGatherAtOffset(SafeFileHandle handle, IReadOnlyL
             MemoryHandle[] handles = new MemoryHandle[buffers.Count];
             Span<Interop.Sys.IOVector> vectors = buffers.Count <= IovStackThreshold ? stackalloc Interop.Sys.IOVector[IovStackThreshold] : new Interop.Sys.IOVector[buffers.Count ];
 
-            for (int i = 0; i < buffers.Count; i++)
-            {
-                ReadOnlyMemory<byte> buffer = buffers[i];
-                MemoryHandle memoryHandle = buffer.Pin();
-                vectors[i] = new Interop.Sys.IOVector { Base = (byte*)memoryHandle.Pointer, Count = (UIntPtr)buffer.Length };
-                handles[i] = memoryHandle;
-            }
-
             long result;
             try
             {
+                for (int i = 0; i < buffers.Count; i++)
+                {
+                    ReadOnlyMemory<byte> buffer = buffers[i];
+                    MemoryHandle memoryHandle = buffer.Pin();
+                    vectors[i] = new Interop.Sys.IOVector { Base = (byte*)memoryHandle.Pointer, Count = (UIntPtr)buffer.Length };
+                    handles[i] = memoryHandle;
+                }
+
                 fixed (Interop.Sys.IOVector* pinnedVectors = &MemoryMarshal.GetReference(vectors))
                 {
                     result = Interop.Sys.PWriteV(handle, pinnedVectors, buffers.Count, fileOffset);
@@ -131,7 +131,7 @@ private static unsafe long WriteGatherAtOffset(SafeFileHandle handle, IReadOnlyL
         private static ValueTask<int> WriteAtOffsetAsync(SafeFileHandle handle, ReadOnlyMemory<byte> buffer, long fileOffset,
             CancellationToken cancellationToken)
         {
-            return new ValueTask<int>(Task.Factory.StartNew(state =>
+            return new ValueTask<int>(Task.Factory.StartNew(static state =>
             {
                 var args = ((SafeFileHandle handle, ReadOnlyMemory<byte> buffer, long fileOffset))state!;
                 return WriteAtOffset(args.handle, args.buffer.Span, args.fileOffset);
@@ -141,7 +141,7 @@ private static ValueTask<int> WriteAtOffsetAsync(SafeFileHandle handle, ReadOnly
         private static ValueTask<long> WriteGatherAtOffsetAsync(SafeFileHandle handle, IReadOnlyList<ReadOnlyMemory<byte>> buffers,
             long fileOffset, CancellationToken cancellationToken)
         {
-            return new ValueTask<long>(Task.Factory.StartNew(state =>
+            return new ValueTask<long>(Task.Factory.StartNew(static state =>
             {
                 var args = ((SafeFileHandle handle, IReadOnlyList<ReadOnlyMemory<byte>> buffers, long fileOffset))state!;
                 return WriteGatherAtOffset(args.handle, args.buffers, args.fileOffset);

diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs
@@ -326,10 +326,27 @@ private static async ValueTask<long> ReadScatterAtOffsetSingleSyscallAsync(SafeF
                 return await ReadAtOffsetAsync(handle, buffers[0], fileOffset, cancellationToken).ConfigureAwait(false);
             }
 
-            (MemoryHandle pinnedSegments, MemoryHandle[] memoryHandles) = PrepareMemorySegments(buffers);
+            // "The array must contain enough elements to store nNumberOfBytesToWrite bytes of data, and one element for the terminating NULL. "
+            long[] fileSegments = new long[buffers.Count + 1];
+            fileSegments[buffers.Count] = 0;
+
+            MemoryHandle[] memoryHandles = new MemoryHandle[buffers.Count];
+            MemoryHandle pinnedSegments = fileSegments.AsMemory().Pin();
 
             try
             {
+                for (int i = 0; i < buffers.Count; i++)
+                {
+                    Memory<byte> buffer = buffers[i];
+                    MemoryHandle memoryHandle = buffer.Pin();
+                    memoryHandles[i] = memoryHandle;
+
+                    unsafe // async method can't be unsafe
+                    {
+                        fileSegments[i] = new IntPtr(memoryHandle.Pointer).ToInt64();
+                    }
+                }
+
                 return await ReadFileScatterAsync(handle, pinnedSegments, totalBytes, fileOffset, cancellationToken).ConfigureAwait(false);
             }
             finally
@@ -458,10 +475,27 @@ private static async ValueTask<long> WriteGatherAtOffsetSingleSyscallAsync(SafeF
                 return await WriteAtOffsetAsync(handle, buffers[0], fileOffset, cancellationToken).ConfigureAwait(false);
             }
 
-            (MemoryHandle pinnedSegments, MemoryHandle[] memoryHandles) = PrepareMemorySegments(buffers);
+            // "The array must contain enough elements to store nNumberOfBytesToWrite bytes of data, and one element for the terminating NULL. "
+            long[] fileSegments = new long[buffers.Count + 1];
+            fileSegments[buffers.Count] = 0;
+
+            MemoryHandle[] memoryHandles = new MemoryHandle[buffers.Count];
+            MemoryHandle pinnedSegments = fileSegments.AsMemory().Pin();
 
             try
             {
+                for (int i = 0; i < buffers.Count; i++)
+                {
+                    ReadOnlyMemory<byte> buffer = buffers[i];
+                    MemoryHandle memoryHandle = buffer.Pin();
+                    memoryHandles[i] = memoryHandle;
+
+                    unsafe // async method can't be unsafe
+                    {
+                        fileSegments[i] = new IntPtr(memoryHandle.Pointer).ToInt64();
+                    }
+                }
+
                 return await WriteFileGatherAsync(handle, pinnedSegments, totalBytes, fileOffset, cancellationToken).ConfigureAwait(false);
             }
             finally
@@ -515,42 +549,6 @@ private static unsafe ValueTask<int> WriteFileGatherAsync(SafeFileHandle handle,
             return new ValueTask<int>(vts, vts.Version);
         }
 
-        private static unsafe (MemoryHandle pinnedSegments, MemoryHandle[] memoryHandles) PrepareMemorySegments(IReadOnlyList<Memory<byte>> buffers)
-        {
-            // "The array must contain enough elements to store nNumberOfBytesToWrite bytes of data, and one element for the terminating NULL. "
-            long[] fileSegments = new long[buffers.Count + 1];
-            fileSegments[buffers.Count] = 0;
-
-            MemoryHandle[] memoryHandles = new MemoryHandle[buffers.Count];
-            for (int i = 0; i < buffers.Count; i++)
-            {
-                Memory<byte> buffer = buffers[i];
-                MemoryHandle memoryHandle = buffer.Pin();
-                memoryHandles[i] = memoryHandle;
-                fileSegments[i] = new IntPtr(memoryHandle.Pointer).ToInt64();
-            }
-
-            return (fileSegments.AsMemory().Pin(), memoryHandles);
-        }
-
-        private static unsafe (MemoryHandle pinnedSegments, MemoryHandle[] memoryHandles) PrepareMemorySegments(IReadOnlyList<ReadOnlyMemory<byte>> buffers)
-        {
-            // "The array must contain enough elements to store nNumberOfBytesToWrite bytes of data, and one element for the terminating NULL. "
-            long[] fileSegments = new long[buffers.Count + 1];
-            fileSegments[buffers.Count] = 0;
-
-            MemoryHandle[] memoryHandles = new MemoryHandle[buffers.Count];
-            for (int i = 0; i < buffers.Count; i++)
-            {
-                ReadOnlyMemory<byte> buffer = buffers[i];
-                MemoryHandle memoryHandle = buffer.Pin();
-                memoryHandles[i] = memoryHandle;
-                fileSegments[i] = new IntPtr(memoryHandle.Pointer).ToInt64();
-            }
-
-            return (fileSegments.AsMemory().Pin(), memoryHandles);
-        }
-
         private static NativeOverlapped GetNativeOverlapped(long fileOffset)
         {
             NativeOverlapped nativeOverlapped = default;

diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.cs b/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.cs
@@ -92,13 +92,13 @@ public static long Read(SafeFileHandle handle, IReadOnlyList<Memory<byte>> buffe
         /// <remarks>Position of the file is not advanced.</remarks>
         public static ValueTask<int> ReadAsync(SafeFileHandle handle, Memory<byte> buffer, long fileOffset, CancellationToken cancellationToken = default)
         {
+            ValidateInput(handle, fileOffset, mustBeAsync: OperatingSystem.IsWindows());
+
             if (cancellationToken.IsCancellationRequested)
             {
                 return ValueTask.FromCanceled<int>(cancellationToken);
             }
 
-            ValidateInput(handle, fileOffset, mustBeAsync: OperatingSystem.IsWindows());
-
             return ReadAtOffsetAsync(handle, buffer, fileOffset, cancellationToken);
         }
 
@@ -121,14 +121,14 @@ public static ValueTask<int> ReadAsync(SafeFileHandle handle, Memory<byte> buffe
         /// <remarks>Position of the file is not advanced.</remarks>
         public static ValueTask<long> ReadAsync(SafeFileHandle handle, IReadOnlyList<Memory<byte>> buffers, long fileOffset, CancellationToken cancellationToken = default)
         {
+            ValidateInput(handle, fileOffset, mustBeAsync: OperatingSystem.IsWindows());
+            ValidateBuffers(buffers);
+
             if (cancellationToken.IsCancellationRequested)
             {
                 return ValueTask.FromCanceled<long>(cancellationToken);
             }
 
-            ValidateInput(handle, fileOffset, mustBeAsync: OperatingSystem.IsWindows());
-            ValidateBuffers(buffers);
-
             return ReadScatterAtOffsetAsync(handle, buffers, fileOffset, cancellationToken);
         }
 
@@ -198,13 +198,13 @@ public static long Write(SafeFileHandle handle, IReadOnlyList<ReadOnlyMemory<byt
         /// <remarks>Position of the file is not advanced.</remarks>
         public static ValueTask<int> WriteAsync(SafeFileHandle handle, ReadOnlyMemory<byte> buffer, long fileOffset, CancellationToken cancellationToken = default)
         {
+            ValidateInput(handle, fileOffset, mustBeAsync: OperatingSystem.IsWindows());
+
             if (cancellationToken.IsCancellationRequested)
             {
                 return ValueTask.FromCanceled<int>(cancellationToken);
             }
 
-            ValidateInput(handle, fileOffset, mustBeAsync: OperatingSystem.IsWindows());
-
             return WriteAtOffsetAsync(handle, buffer, fileOffset, cancellationToken);
         }
 
@@ -227,14 +227,14 @@ public static ValueTask<int> WriteAsync(SafeFileHandle handle, ReadOnlyMemory<by
         /// <remarks>Position of the file is not advanced.</remarks>
         public static ValueTask<long> WriteAsync(SafeFileHandle handle, IReadOnlyList<ReadOnlyMemory<byte>> buffers, long fileOffset, CancellationToken cancellationToken = default)
         {
+            ValidateInput(handle, fileOffset, mustBeAsync: OperatingSystem.IsWindows());
+            ValidateBuffers(buffers);
+
             if (cancellationToken.IsCancellationRequested)
             {
                 return ValueTask.FromCanceled<long>(cancellationToken);
             }
 
-            ValidateInput(handle, fileOffset, mustBeAsync: OperatingSystem.IsWindows());
-            ValidateBuffers(buffers);
-
             return WriteGatherAtOffsetAsync(handle, buffers, fileOffset, cancellationToken);
         }