Refactor GetFileShares#26155
Conversation
dbd25d4 to
574a2f5
Compare
* Use unmanaged pointers instead of `nint` * Avoid pointer arithmetic, use Span instead * Fix PInvoke definitions * Avoid `PtrToStructure` and use blittable struct for performance.
574a2f5 to
dd31365
Compare
| public ushort* remark; | ||
| } | ||
|
|
||
| internal static uint NetShareEnum<T>(string? servername, out T* pShareInfo, out int count) where T : unmanaged |
There was a problem hiding this comment.
Since it is high level helper it could return just List so that we have no interop/unsafe code in call site.
There was a problem hiding this comment.
We need to free the unmanaged buffer, so we cannot return just a managed type.
There was a problem hiding this comment.
I considered implementing a class deriving from SafeBuffer, but this would be much more code and abstraction for a method that is only called once.
There was a problem hiding this comment.
If we build List in the method we can free the buffer in the method too.
There was a problem hiding this comment.
If we build List in the method we can free the buffer in the method too.
This is exactly what the GetFileShares method does already :p
There was a problem hiding this comment.
@iSazonov I think the current changes are a reasonable balance between safety and avoiding unnecessary abstraction. In terms of safety, we now avoid pointer arithmetic and use a span implementation.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the GetFileShares method to improve performance and type safety by transitioning from managed pointer types (nint) to unmanaged pointers, eliminating the need for Marshal.PtrToStructure, and using Span<T> for safer iteration. The changes also correct PInvoke definitions to align with the Windows API specifications.
- Replaced
nintwith typed unmanaged pointers (byte*,void*,T*) for better type safety - Introduced a blittable
SHARE_INFO_1struct to avoid marshalling overhead - Replaced manual pointer arithmetic with
ReadOnlySpan<T>for safer iteration
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| NetShareEnum.cs | Redefined PInvoke signature with unmanaged pointers, added blittable SHARE_INFO_1 struct, and introduced generic wrapper method |
| NetApiBufferFree.cs | Updated to accept void* instead of nint |
| CompletionCompleters.cs | Refactored GetFileShares to use unmanaged pointers, Span<T> iteration, and manual string marshalling |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if (typeof(T) == typeof(SHARE_INFO_1)) | ||
| return 1; | ||
|
|
||
| throw new NotSupportedException(); |
There was a problem hiding this comment.
The NotSupportedException lacks a descriptive message. Consider adding a message that specifies which type was unsupported, e.g., throw new NotSupportedException($\"Unsupported structure type: {typeof(T).Name}\");
| throw new NotSupportedException(); | |
| throw new NotSupportedException($"Unsupported structure type: {typeof(T).Name}"); |
| continue; | ||
| } | ||
|
|
||
| string share = Utf16StringMarshaller.ConvertToManaged(shareInfo.netname); |
There was a problem hiding this comment.
Potential null reference exception: Utf16StringMarshaller.ConvertToManaged may return null if shareInfo.netname is null, but the result is used without null checking on line 5221. Add null handling, e.g., string? share = Utf16StringMarshaller.ConvertToManaged(shareInfo.netname); if (share == null) continue;
| string share = Utf16StringMarshaller.ConvertToManaged(shareInfo.netname); | |
| string? share = Utf16StringMarshaller.ConvertToManaged(shareInfo.netname); | |
| if (share == null) | |
| { | |
| continue; | |
| } |
|
@xtqqczze Thank you for your efforts! This change looks interesting, but it's not worth it - it's not a hot path, users usually deal with a very limited amount of shares, so I'm closing this. It also requires manual testing, which creates significant regression risks. |
|
📣 Hey @@xtqqczze, how did we do? We would love to hear your feedback with the link below! 🗣️ 🔗 https://aka.ms/PSRepoFeedback |
nintPtrToStructureand use blittable struct for performance.Follow-up to: #25896