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

Merge | NativeMethods/SafeNativeMethods #2997

Merged
merged 4 commits into from
Nov 13, 2024

Conversation

benrr101
Copy link
Contributor

Description: This is the last major change for the Interop namespace. There was a handful of interop calls tucked away in the native safe and unsafe classes as well as a super secret one in the SNI wrapper class. For this PR they were moved into the Interop namespace in the classes that correspond to the DLLs they were referencing.

One semi-important note: Everything in SafeNativeMethods was moved into a Kernel32Safe class - this was because the "Safe" methods were given an attribute that indicates that the security checks when calling across the managed/unmanaged boundary should not be performed. Rather than potentially change perf, I've elected to leave it as-is but call out when the calls are "safe".

Testing: For the most part, this PR just moves stuff around without changing functionality. It should be good if the tests pass.

@benrr101 benrr101 added the Common Project 🚮 Things that relate to the common project project label Nov 11, 2024
[ResourceExposure(ResourceScope.None)]
static internal extern IntPtr GetProcAddress(IntPtr HModule, [MarshalAs(UnmanagedType.LPStr), In] string funcName/*lpcstr*/);

internal static extern IntPtr GetProcAddress(IntPtr HModule, [MarshalAs(UnmanagedType.LPStr), In] string funcName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice change! Should we consider updating the remaining functions to align with this convention in future PRs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohhhhhh... yeah. I've got most of those staged up, not sure if it's ready yet.

@cheenamalhotra
Copy link
Member

cheenamalhotra commented Nov 13, 2024

Little concerned about why the Windows Enclave 'functional' tests are hung up.. any idea what's going on?

Edit: They seem to have passed now.

Copy link

codecov bot commented Nov 13, 2024

Codecov Report

Attention: Patch coverage is 10.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 72.47%. Comparing base (68c4e41) to head (72867e9).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
...etfx/src/Microsoft/Data/SqlClient/SqlConnection.cs 0.00% 5 Missing ⚠️
...t/netfx/src/Microsoft/Data/SqlClient/LocalDBAPI.cs 0.00% 2 Missing ⚠️
...SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2997      +/-   ##
==========================================
+ Coverage   72.45%   72.47%   +0.01%     
==========================================
  Files         288      288              
  Lines       59498    59493       -5     
==========================================
+ Hits        43111    43115       +4     
+ Misses      16387    16378       -9     
Flag Coverage Δ
addons 92.58% <ø> (ø)
netcore 75.41% <ø> (+0.04%) ⬆️
netfx 70.91% <10.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@benrr101 benrr101 merged commit 4052186 into dotnet:main Nov 13, 2024
76 checks passed
@benrr101 benrr101 deleted the interop/nativemethods branch November 13, 2024 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Common Project 🚮 Things that relate to the common project project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants