Unify CreateFile pinvoke in SMA #18751

iSazonov · 2022-12-08T09:37:57Z

PR Summary

Contributes to #18553.

PR Context

PR Checklist

xtqqczze · 2022-12-10T13:11:58Z

src/System.Management.Automation/engine/Interop/Windows/FindFirstFile.cs

@@ -50,6 +52,14 @@ protected override bool ReleaseHandle()
        // We use 'FindFirstFileW' instead of 'FindFirstFileExW' because the latter doesn't work correctly with Unicode file names on FAT32.
        // See https://github.com/PowerShell/PowerShell/issues/16804
        [LibraryImport("api-ms-win-core-file-l1-1-0.dll", EntryPoint = "FindFirstFileW", SetLastError = true, StringMarshalling = StringMarshalling.Utf16)]


we can now remove EntryPoint parameter.

I would rather use explicit names everywhere as I have caught a problem with them several times already :-) Moreover LibraryImport source generator always adds this explicitly.

xtqqczze · 2022-12-10T13:11:58Z

src/System.Management.Automation/engine/Interop/Windows/CreateFile.cs

+
+        // WARNING: This method does not implicitly handle long paths. Use CreateFile.
+        [LibraryImport("api-ms-win-core-file-l1-1-0.dll", EntryPoint = "CreateFileW", SetLastError = true, StringMarshalling = StringMarshalling.Utf16)]
+        private static unsafe partial SafeFileHandle CreateFilePrivate(


Rename method to the entry point name.

I believe the intent of the name is that code doesn't call the C function directly

@SteveL-MSFT The private accessibility modifier should be sufficient.

We could have the one extern signature that returns nint and just have the outer methods wrap that in the specific safe handle types.

The code was copied from .Net as well tested. Different teams create pinvoke code in .Net so it can have different styles. We have several PRs at the same time so it's also hard for us to follow one style. It is better to make a style correction later.

SteveL-MSFT

Looks fine to me, but there's a good amount of changes that should get another opinion

jborean93 · 2022-12-12T20:05:13Z

src/System.Management.Automation/engine/Interop/Windows/CreateFile.cs

+
+        // dwFlagsAndAttributes
+        [Flags]
+        internal enum FileAttributes : uint


This is missing a few things from https://learn.microsoft.com/en-us/dotnet/api/system.io.fileattributes?view=net-7.0. Personally I recommend having FileAttributes as one parameter and another custom enum just for the flags and in the CreateFile call bitor them together like https://gist.github.com/jborean93/246daf83f3aaca631dbca5652c3cc91e#file-get-smbapplicationkey-ps1-L165-L184.

It is a copy from old code. Really we have no need all the values and I kept them "just in case". Later we could correct style to follow .Net doc names.

If we are trying to unify this, isn't it ideal to do it properly first then migrate the code.

Personally I recommend having FileAttributes as one parameter and another custom enum just for the flags and in the CreateFile call bitor them together

Since this is low-level internal code for P/Invoke, there's no particular reason to complicate matters by using the public FileAttributes Enum.

My preference is to stay as close a possible to the underlying API, see: https://github.com/dotnet/pinvoke/blob/main/src/Kernel32/Kernel32+CreateFileFlags.cs

jborean93 · 2022-12-12T20:05:13Z

src/System.Management.Automation/engine/Interop/Windows/CreateFile.cs

+            System = 0x00000004,
+            Archive = 0x00000020,
+            Encrypted = 0x00004000,
+            Write_Through = 0x80000000,


If you do keep this method, any reason why Write_Through is written like this?

If we are not going to use original naming convention, i.e. FILE_FLAG_WRITE_THROUGH, use C# convention.

It is a copy from old code. Really we have no need all the values and I kept them "just in case". Later we could correct style to follow .Net doc names.

Really, there is no need to follow the names from the public .NET API.

src/System.Management.Automation/engine/remoting/common/RemoteSessionNamedPipe.cs

jborean93 · 2022-12-13T04:09:32Z

src/System.Management.Automation/engine/Interop/Windows/CreateFile.cs

+            return CreateFilePrivate(lpFileName, (uint)dwDesiredAccess, dwShareMode, nint.Zero, dwCreationDisposition, dwFlagsAndAttributes, nint.Zero);
+        }
+
+        internal static unsafe nint CreateFileWithSafePipeHandle(


Why does this return nint and not SafePipeHandle? I assume the purpose of having a separate call is to wrap it in a specific handle.

I preferred to make minimal changes to this code so as not to break anything.
As you can see it needs a thorough revision so it could probably be replaced by .Net code and most importantly it compiles for Unix when it should not.
It takes more knowledge to get the job done. Perhaps you could take a look at this if you are interested. That would be great!

If that's the case I wouldn't have a CreateFileWithSafePipeHandle just a generic CreateFileW that returns the nint. In the future when we are ready to expose something that will return a SafePipeHandle then we can look at adding this new pipe specific call.

iSazonov · 2022-12-13T04:11:45Z

Since we do several PRs in parallel and copy code from both PowerShell and .Net, we already have a number of differences in style. This can be unified later if needed. I suppose it doesn't make sense to waste time adding unused constants or adding new enums because it's unlikely that this code will be used elsewhere. We aim to eliminate pinvokes in favor of .Net APIs.

jborean93 · 2022-12-13T04:15:40Z

We aim to eliminate pinvokes in favor of .Net APIs.

In this case dotnet do not seem like they are going to expose this in their own public API as per your recent issue dotnet/runtime#79335. My concern is we are moving code that can be improved in several ways and by not updating it to a better standard we run the risk of it staying like that forever. I don't want to stand in the way of this PR but my concern is we will move this across as is then in the future people might be reluctant to change it because that's how the code is. Granted that's not a big concern but why not just do things properly once rather than twice.

iSazonov · 2022-12-13T05:12:28Z

In this case dotnet do not seem like they are going to expose this in their own public API as per your recent issue dotnet/runtime#79335.

For this reason we will not be able to remove all pinvokes, but so where possible it should be done. And we need to keep working with the .Net team on new APIs.

My concern is we are moving code that can be improved in several ways and by not updating it to a better standard we run the risk of it staying like that forever. I don't want to stand in the way of this PR but my concern is we will move this across as is then in the future people might be reluctant to change it because that's how the code is. Granted that's not a big concern but why not just do things properly once rather than twice.

Migrating to LibraryImport helps us at least make the main code more readable and remove auxiliary code from it. I'm very happy that this happens. For example, the FileSystemProvider file will become almost 10% smaller!
As for the style of pinvokes themselves, it doesn't bother me. For example, there will always be a debate which is better - to use constant names from documentation, or enums, or to combine these together. I have only one strict preference - to follow as close as possible to the .Net implementation, but as we know they don't have the same style there either. Moreover, they are still working on LibraryImport in .Net 8, so there will most likely be improvements there for us to follow.
It is more important to make the code better. This, too, can be a matter of discussion. For example, there is no point in implementing SafeHandle and adding a lot of allocations on hot path if we can easily open and close the handle in one place.
It's worth discussing anyway. To be specific, please open an issue(s) with a proposal for discussion. It can be about a specific code or a general style with examples. I am willing to participate in the discussions and actively promote your PRs.

iSazonov · 2022-12-13T05:25:59Z

Looks fine to me, but there's a good amount of changes that should get another opinion

@SteveL-MSFT Do you ask about additional review? Who do you want to involve?

SteveL-MSFT · 2022-12-13T05:34:09Z

if @jborean93 approves, I think that would be sufficient

jborean93 · 2022-12-13T05:41:08Z

The only thing I think I really would like to see is the removal of the pipe specific extern signatures. Currently it is adding no benefit over a straight call to CreateFileW as it isn't wrapping SafePipeHandle and the extended path/period end checks should not apply to pipes. I think we should just have 2 methods in this file

CreateFileW that is the extern signature and returns nint
CreateFileWithSafeFileHandle that does the max path and trailing period check that calls CreateFileW and wraps the return with SafeFileHandle

The current code that calls CreateFileWithSafePipeHandle can just call CreateFileW until we are ready to make a CreateFileWithSafePipeHandle (or the equivalent for pipes).

The other stuff I've mentioned in the review is mostly cosmetic and I think are fine to evolve over time in separate PRs.

iSazonov · 2022-12-13T07:09:02Z

The only thing I think I really would like to see is the removal of the pipe specific extern signatures. Currently it is adding no benefit over a straight call to CreateFileW as it isn't wrapping SafePipeHandle and the extended path/period end checks should not apply to pipes. I think we should just have 2 methods in this file

I removed pipe path normalization since max pipe length is 256 https://learn.microsoft.com/en-us/windows/win32/ipc/pipe-names
I rename pipe helper (remove "Safe" from name) but keep the method since it hides two extra parameters in main code. I hope we remove the pinvoke at all in future.
@jborean93 Since you have great experience in this area could you please look and conclude if it is possible to get rid of this pinvoke at all with .Net code?

Pipe Names - Win32 apps
Each named pipe has a unique name that distinguishes it from other named pipes in the systems list of named objects. A pipe server specifies a name for the pipe when it calls the CreateNamedPipe function to create one or more instances of a named pipe.

jborean93 · 2022-12-13T07:13:52Z

At least for the pipes it should be possible to just use NamedPipeClientStream. I'm not aware of any limitations it has over the CreateFileW call but as you've mentioned in another PR migrating to this might take more invasive changes we wanted to hold off from the PInvoke update PRs.

pull-request-quantifier · 2022-12-15T03:58:55Z

This PR has 313 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Large
Size       : +142 -171
Percentile : 71.3%

Total files changed: 8

Change summary by file extension:
.cs : +142 -171

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detected.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 👌 👎 (Email)
Customize PullRequestQuantifier for this repository.

msftbot · 2022-12-20T22:58:01Z

🎉v7.4.0-preview.1 has been released which incorporates this pull request.🎉

Handy links:

Release Notes

iSazonov added the CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log label Dec 8, 2022

iSazonov requested review from jborean93 and SteveL-MSFT December 8, 2022 09:37

iSazonov self-assigned this Dec 8, 2022

pull-request-quantifier bot added the Large label Dec 8, 2022

iSazonov force-pushed the libraryimport-createfile branch 4 times, most recently from 8428bb9 to b102d42 Compare December 8, 2022 16:55

iSazonov added 3 commits December 8, 2022 22:03

WinIsSameFileSystemItem

6c350fc

WinGetInodeData

eb17edd

WinOpenReparsePoint

2c28236

iSazonov force-pushed the libraryimport-createfile branch 3 times, most recently from fdbca38 to 280cb23 Compare December 8, 2022 17:25

iSazonov marked this pull request as ready for review December 9, 2022 06:34

iSazonov requested review from PaulHigin and anmenaga as code owners December 9, 2022 06:34

iSazonov added 5 commits December 9, 2022 14:23

WinIsHardLink

68e77c4

FindFirstFile

21978a0

Remove unused code

5cfa704

RemoteSessionNamedPipe

0c40260

Fix build on Unix

06493e6

iSazonov force-pushed the libraryimport-createfile branch from f6032c3 to 06493e6 Compare December 9, 2022 09:24

xtqqczze reviewed Dec 10, 2022

View changes

SteveL-MSFT approved these changes Dec 12, 2022

View changes

jborean93 reviewed Dec 12, 2022

View changes

jborean93 reviewed Dec 13, 2022

View changes

Remove path normalization for pipes

ce8715d

xtqqczze mentioned this pull request Dec 13, 2022

Use System.IO.Pipes.PipeStream #18145

Open

iSazonov closed this Dec 15, 2022

iSazonov reopened this Dec 15, 2022

iSazonov merged commit 218c6ce into PowerShell:master Dec 15, 2022
42 checks passed

iSazonov deleted the libraryimport-createfile branch December 15, 2022 06:05

CarloToso pushed a commit to CarloToso/PowerShell that referenced this pull request Jan 15, 2023

Unify CreateFile pinvoke in SMA (PowerShell#18751)

0a07ef8

Unify CreateFile pinvoke in SMA #18751

Unify CreateFile pinvoke in SMA #18751

iSazonov commented Dec 8, 2022 •

edited

xtqqczze Dec 10, 2022

iSazonov Dec 13, 2022

xtqqczze Dec 10, 2022

SteveL-MSFT Dec 12, 2022

xtqqczze Dec 12, 2022

jborean93 Dec 12, 2022

iSazonov Dec 13, 2022

SteveL-MSFT left a comment

jborean93 Dec 12, 2022

iSazonov Dec 13, 2022 •

edited

jborean93 Dec 13, 2022

xtqqczze Dec 13, 2022

jborean93 Dec 12, 2022

xtqqczze Dec 12, 2022

iSazonov Dec 13, 2022

xtqqczze Dec 13, 2022 •

edited

jborean93 Dec 13, 2022

iSazonov Dec 13, 2022

jborean93 Dec 13, 2022

iSazonov commented Dec 13, 2022

jborean93 commented Dec 13, 2022

iSazonov commented Dec 13, 2022

iSazonov commented Dec 13, 2022

SteveL-MSFT commented Dec 13, 2022

jborean93 commented Dec 13, 2022 •

edited

iSazonov commented Dec 13, 2022 •

edited by unfurl-links bot

jborean93 commented Dec 13, 2022

pull-request-quantifier bot commented Dec 15, 2022

What can I do to optimize my changes

How to interpret the change counts in git diff output

msftbot bot commented Dec 20, 2022

Unify CreateFile pinvoke in SMA #18751

Unify CreateFile pinvoke in SMA #18751

Conversation

iSazonov commented Dec 8, 2022 • edited

PR Summary

PR Context

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveL-MSFT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iSazonov Dec 13, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xtqqczze Dec 13, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iSazonov commented Dec 13, 2022

jborean93 commented Dec 13, 2022

iSazonov commented Dec 13, 2022

iSazonov commented Dec 13, 2022

SteveL-MSFT commented Dec 13, 2022

jborean93 commented Dec 13, 2022 • edited

iSazonov commented Dec 13, 2022 • edited by unfurl-links bot

jborean93 commented Dec 13, 2022

pull-request-quantifier bot commented Dec 15, 2022

What can I do to optimize my changes

How to interpret the change counts in git diff output

msftbot bot commented Dec 20, 2022

iSazonov commented Dec 8, 2022 •

edited

iSazonov Dec 13, 2022 •

edited

xtqqczze Dec 13, 2022 •

edited

jborean93 commented Dec 13, 2022 •

edited

iSazonov commented Dec 13, 2022 •

edited by unfurl-links bot