GH Issue #15652 SqlFileStream Implementation on Windows by keeratsingh · Pull Request #29879 · dotnet/corefx

keeratsingh · 2018-05-23T22:43:25Z

This Pull Request ports the SqlFileStream #15652 capability only on Windows.
SqlClient throws SqlFileStream is not supported on this platform exception for Non-Windows Platforms.

Client	Server	Feasible	Comments
Windows	Windows	Yes	Implemented by this PR.
Windows	Linux	No	SQL Server on Linux does not support FileStream.
Linux	Windows	No	Currently there is no way to open a file over SMB and pass the Extended Attributes.
Linux	Linux	No	SQL Server on Linux does not support FileStream.

keeratsingh · 2018-05-23T22:45:27Z

    {
        // https://msdn.microsoft.com/en-us/library/bb432380.aspx
        // https://msdn.microsoft.com/en-us/library/windows/hardware/ff566424.aspx
+#pragma warning disable BCL0015 // Invalid Pinvoke call


Suppressing warnings as per Jan's suggestion #29169

Please remove the pragma- shouldn't need it if we aren't including this on UAP.

In reply to: 190421828 [](ancestors = 190421828)

keeratsingh · 2018-05-23T22:46:03Z

    internal partial class NtDll
    {
        // https://msdn.microsoft.com/en-us/library/windows/desktop/ms680600(v=vs.85).aspx
+		#pragma warning disable BCL0015 // Invalid Pinvoke call


Suppressing warnings as per Jan's suggestion #29169

You should not need this if you're only including the files in the non UAP builds. For example:

https://github.com/dotnet/corefx/blob/master/src/System.IO.FileSystem/src/System.IO.FileSystem.csproj#L9

https://github.com/dotnet/corefx/blob/master/src/System.IO.FileSystem/src/System.IO.FileSystem.csproj#L187-L216

@JeremyKuhne
Adding the condition '$(UWPCompatible)' != 'true' as per the suggestion throws an error TypesMustExist : Type 'System.Data.SqlTypes.SqlFileStream' does not exist in the implementation but it does exist in the contract.

keeratsingh · 2018-05-23T22:48:06Z

+        {
+                return ( CreateFile(eaBuffer: new Microsoft.Win32.SafeHandles.SafeFileHandle(IntPtr.Zero, true), eaLength: 0, path, rootDirectory,createDisposition, desiredAccess, shareAccess, fileAttributes, createOptions, objectAttributes) );
+        }
+        internal unsafe static (int status, IntPtr handle) CreateFile(


Had to create a separate CreateFile Method as SqlFileStream requires the Extended Attributes buffer to be supplied and currently there is currently no way to do so other than NTCreateFile #29379

stephentoub · 2018-05-24T13:19:14Z

+            return path;
+        }
+
+        static private void DemandAccessPermission


All of this CAS-related stuff can be deleted.

Is CAS-related stuff no longer valid/required ?

Right, it's all nops on core.

stephentoub · 2018-05-24T13:20:37Z

@@ -0,0 +1,69 @@
+// Licensed to the .NET Foundation under one or more agreements.


This file looks identical to the .Uap.cs one. You could have a single SqlFileStream.Unsupported.cs file that's included in both the UAP and Unix builds.

That said, why can't it be supported? There's no way to make the majority of the functionality work on Unix or in UAP?

I will create a singe file SqlFileStream.Unsupported.cs
As to why can't it be supported answered in the comments below.

stephentoub · 2018-05-24T13:21:46Z

+                try
+                {
+
+                    (int status, IntPtr handle) = Interop.NtDll.CreateFile( 


Why do we need to use Win32 interop for this? i.e. why can't this just use FileStream?

As per reasons mentioned in #29379, since it is not a real UNC path, the bogus path only has meaning in the context of this transaction, hence we cannot use FileStream directly.

stephentoub · 2018-05-24T13:22:26Z

+                bool bRevertAssert = false;
+                try
+                {
+                    SecurityPermission sp = new SecurityPermission(SecurityPermissionFlag.UnmanagedCode);


All of the CAS-related code can be deleted.

stephentoub · 2018-05-24T13:24:04Z

+                        byte[] transactionContext = reader.GetSqlBytes(1).Buffer;
+
+                        // Create the SqlFileStream  
+                        using (Stream fileStream = new SqlFileStream(path, transactionContext, FileAccess.Read, FileOptions.SequentialScan, allocationSize: 0))


I see, it's Windows-only because SQL Server hands back a UNC path to the file to be opened?
cc: @JeremyKuhne

Explained in the comment above and more information in comments #15652
Also, this is Windows-only because SQL Server on Linux doesn't support FileStream currently.
In case of Linux Client and Windows SQL Server, as per discussions with Robert Dorr, currently there is no way to open a file over SMB and pass the Extended Attributes on Linux.

JeremyKuhne · 2018-05-24T20:03:36Z

+			/// <summary>
+            /// The offset of the next FILE_FULL_EA_INFORMATION-type entry. This member is zero if no other entries follow this one.
+            /// </summary>
+            internal UInt32 nextEntryOffset;


nit: we use the C# keywords for types (uint, byte, byte, ushort, byte) #Resolved

JeremyKuhne · 2018-05-24T20:06:52Z

            public unsafe OBJECT_ATTRIBUTES(UNICODE_STRING* objectName, ObjectAttributes attributes, IntPtr rootDirectory)
            {
-                Length = (uint)sizeof(OBJECT_ATTRIBUTES);
+                Length = (uint)Marshal.SizeOf(typeof(OBJECT_ATTRIBUTES));


Don't change this to Marshal.SizeOf- it is much more expensive. #Resolved

JeremyKuhne · 2018-05-24T20:15:04Z

+			/// <summary>
+            /// Specifies the size, in bytes, of this structure.
+            /// </summary>
+            internal UInt32 length;


Also, please match the casing of the docs. (Length, etc.)

JeremyKuhne · 2018-05-24T20:16:06Z

+
+        public enum Access
+        {
+			/// <summary>


You've got tabs in here, please use spaces for indenting

JeremyKuhne · 2018-05-24T20:17:48Z

+    internal partial class Kernel32
+    {
+		/// <summary>
+        /// <a href="https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/buffer-descriptions-for-i-o-control-codes">RequiredAccess</a>.


Why this link?

Some more useful ones:

https://docs.microsoft.com/en-us/previous-versions/windows/hardware/design/dn614603
https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/defining-i-o-control-codes

Thank you for catching this, must have mixed up the links. I have updated the link with the second link, since the first link throws error '404 Content not found'

JeremyKuhne · 2018-05-24T20:28:01Z

+		/// Indicates the type of access that a caller must request when opening the file object that represents the device (see IRP_MJ_CREATE).
+        /// </summary>
+
+        public enum Access


You can make this a [Flags] to represent that you can OR the values. The other option (given that Any is 0), is to define FILE_READWRITE_ACCESS with a comment noting that it isn't actually defined in Windows, but we're including it for convenience.

I have added the [Flags] attribute to the enum.

JeremyKuhne · 2018-05-24T20:29:11Z

+			/// <summary>
+            /// The I/O manager sends the IRP for any caller that has a handle to the file object that represents the target device object.
+			/// </summary>
+            FILE_ANY_ACCESS,


Should call out that this value should be avoided. (https://docs.microsoft.com/en-us/previous-versions/windows/hardware/design/dn614603) Also, consider actually putting in the defined values explicitly (i.e. 0x00, 0x01, etc.)

The link above throws error 404 Content not found, could you kindly confirm if the link is correct.

JeremyKuhne · 2018-06-28T00:19:55Z

+        /// <a href="https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/buffer-descriptions-for-i-o-control-codes">TransferType</a>.
+        /// Indicates how the system will pass data between the caller of DeviceIoControl (or IoBuildDeviceIoControlRequest) and the driver that handles the IRP.
+        /// </summary>
+        public enum Method


It would be better to name this TransferType to match what it is used for. Or perhaps IoControlTransferType.

JeremyKuhne · 2018-06-28T00:23:06Z

+        }
+
+        internal unsafe static (int status, IntPtr handle) CreateFile(
+            void* eaBuffer,


Put new overload parameters at the end (eaBuffer & eaLength);

JeremyKuhne · 2018-06-28T00:30:37Z

+            string formatPath = string.Empty;
+            string uniqueId = string.Empty;
+
+            formatPath = System.IO.PathInternal.IsDeviceUNC(path.AsSpan()) ? @"\??" : @"\??\UNC\{0}\{1}";


nit: there is an implicit conversion to span from string, you shouldn't need .AsSpan(). Additionally you're not building it right if it is a device path already. If it is \\.\UNC\Server\Share\SomePath you need to change \\. to \??, not trim off \\. You also still need to add your uniqueId.

JeremyKuhne

Interop code is much better! Left some additional comments, some of which need to be addressed.

keeratsingh · 2018-06-28T21:16:20Z

@dotnet-bot Test NETFX x86 Release Build

JeremyKuhne · 2018-06-30T00:20:39Z

-                    out IntPtr handle,
-                    desiredAccess,
-                    ref attributes,
+                    FileHandle: out IntPtr handle,


nit: These aren't necessary, in fact you're missing one for statusBlock. The only reason I added them later was to be explicit about what null was (AllocationSize:). In earlier versions of C# you had to name all of the arguments after named one. This really should only have just that one explicit- it helps when reading the code when explicitly calling out what arguments are when passing things like null, true, false, and other unnamed constant values. (So you don't have to look at the called method to understand what the value is.)

Changed code to only one explicit AllocationSize as the other arguments are self descriptive.

JeremyKuhne · 2018-06-30T00:21:54Z

+            byte access )
+        {
+
+            // <a href="https://docs.microsoft.com/en-us/windows-hardware/drivers/develop/how-to-select-and-configure-the-device-fundamental-tests">MaxFunctionCode</a> method.


Tags like this (e.g. <a> have no meaning in a regular comment. The URL will still get highlighted, however.

Removed tag <a>

JeremyKuhne · 2018-06-30T00:22:47Z

+using Microsoft.Win32.SafeHandles;
+using System;
+using System.Runtime.InteropServices;
+using System.Runtime.Versioning;


Why the Versioning using?

Must have been carried over, removing the using System.Runtime.Versioning;

JeremyKuhne · 2018-06-30T00:24:37Z

+        #endregion
+
+    }
+


nit: empty lines

JeremyKuhne · 2018-06-30T00:24:49Z

+
+            return tempTable;
+        }
+        private static void ExecuteNonQueryCommand(string cmdText, SqlConnection conn)


nit: no separating line

JeremyKuhne · 2018-06-30T00:26:57Z

            // shimming method. (Or making a change that doesn't impact normalization, such as adding a filename to a
            // normalized base path.)
-            if (IsPartiallyQualified(path) || IsDevice(path))
+            if (IsPartiallyQualified(path.AsSpan()) || IsDevice(path.AsSpan()))


This shouldn't be necessary. Can you undo this, please?

Undid the changes.

JeremyKuhne · 2018-06-30T00:27:58Z

+            ObjectAttributes objectAttributes = ObjectAttributes.OBJ_CASE_INSENSITIVE,
+            void* eaBuffer = null,
+            uint eaLength = 0
+           )


nit: please keep the trailing paren on the line above.

JeremyKuhne · 2018-06-30T00:28:31Z

+            ushort deviceType,
+            ushort function,
+            byte method,
+            byte access )


nit: extra space

JeremyKuhne · 2018-06-30T00:33:09Z

+        };
+
+        public SqlFileStream
+            (


I don't see this sort of indenting for parameter parenthesis in the other files in this assembly. I know the original source files were like this, but it would be worth following the CoreFX styles and keep the parens next to the arguments. There aren't many arguments here so they can go on one line. Also, unless there is a conflict, you can remove System.IO from the type names.

Updated to normal styling as requested.

JeremyKuhne · 2018-06-30T00:34:22Z

+    public sealed partial class SqlFileStream : System.IO.Stream
+    {
+        public SqlFileStream
+            (


Commented elsewhere. Can you please pull the arguments into our normal styling? Also it looks like you have more usings than needed in this file.

Updated to normal styling as requested.

JeremyKuhne

Looks pretty good- there is some more style polish that I'l like to see. Otherwise looks great.

keeratsingh · 2018-07-03T16:35:16Z

@dotnet-bot Test Windows x64 Debug Build

keeratsingh · 2018-07-03T17:31:44Z

@dotnet-bot Test Linux x64 Release Build

JeremyKuhne · 2018-07-05T23:12:05Z

Thanks for the follow-up @keeratsingh!

keithnicholson · 2018-07-06T21:58:55Z

Wonderful! Thank you! Looking forward to the future release.

…efx#29879) * Initial port of SqlFileStream * Added SqlFileStreamTest * Cleanup code * Updated file paths after catchup merge. * Added newline at the end of Strings.resx * Fixed cosmetic changes * Replaced null SafeHandle call * Added headers to SqlFileStreamTest.cs * Addressed review feedback by Stephen and Jeremy * Fixed filename UnSupported.cs to Unsupported.cs to match case in Linux builds. * Addressed review comments * Made SqlFileStream netcoreapp and windows specific. * Update EaName to include Null Terminator * Fixed cosmetic issues * Addressed Dan's review feedback. Removed initially added file Interop.GetFullPathName.cs as is it no longer being used. * Addressed Jeremy's review comments. * Addressed Jeremy's review comments about style polish. Commit migrated from dotnet/corefx@2f7844a

Keerat Singh added 8 commits May 22, 2018 16:01

Initial port of SqlFileStream

3ef90b5

Added SqlFileStreamTest

5175975

Cleanup code

b371223

Merge branch 'master' into gh_15652_SqlFileStream

84afcba

Updated file paths after catchup merge.

23ab078

Added newline at the end of Strings.resx

52e7ff1

Fixed cosmetic changes

c5bd002

Replaced null SafeHandle call

56ba861

keeratsingh requested review from AfsanehR-zz, corivera, saurabh500 and stephentoub May 23, 2018 22:43

keeratsingh commented May 23, 2018

View reviewed changes

keeratsingh requested a review from geleems May 23, 2018 22:49

Added headers to SqlFileStreamTest.cs

51fedd2

keeratsingh changed the title ~~GH Issue #15652 SqlFileStream Implementaion~~ GH Issue #15652 SqlFileStream Implementation May 23, 2018

stephentoub reviewed May 24, 2018

View reviewed changes

JeremyKuhne reviewed May 24, 2018

View reviewed changes

JeremyKuhne reviewed Jun 28, 2018

View reviewed changes

JeremyKuhne suggested changes Jun 28, 2018

View reviewed changes

Addressed Jeremy's review comments.

cff06a1

JeremyKuhne reviewed Jun 30, 2018

View reviewed changes

JeremyKuhne suggested changes Jun 30, 2018

View reviewed changes

Addressed Jeremy's review comments about style polish.

b513d0d

JeremyKuhne approved these changes Jul 5, 2018

View reviewed changes

JeremyKuhne merged commit 2f7844a into dotnet:master Jul 5, 2018

keeratsingh changed the title ~~GH Issue #15652 SqlFileStream Implementation~~ GH Issue #15652 SqlFileStream Implementation on Windows Jul 6, 2018

keeratsingh added this to the 3.0 milestone Jul 6, 2018

keeratsingh deleted the gh_15652_SqlFileStream branch July 11, 2018 22:45

keeratsingh mentioned this pull request Jan 31, 2020

Fix the way code is handling passing in the extended attribute information dotnet/runtime#26589

Closed

qmfrederik mentioned this pull request Jul 7, 2020

src\libraries\Common\src\Interop\Windows\Kernel32\Interop.DeviceIoControl.cs appears to be a dead file dotnet/runtime#38861

Closed

		@@ -0,0 +1,69 @@
		// Licensed to the .NET Foundation under one or more agreements.

Conversation

keeratsingh commented May 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub May 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeremyKuhne May 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeremyKuhne May 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeremyKuhne left a comment

Choose a reason for hiding this comment

Uh oh!

keeratsingh commented Jun 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub May 24, 2018 •

edited

Loading

JeremyKuhne May 24, 2018 •

edited

Loading

JeremyKuhne May 24, 2018 •

edited

Loading