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

MapViewOfFile should return a handle type #570

Closed
wants to merge 1 commit into from

Conversation

mikebattista
Copy link
Collaborator

@mikebattista mikebattista commented Jul 7, 2021

Fixes #561

@AArnott AArnott changed the title Fixed #561. MapViewOfFile should return a handle type Jul 9, 2021
@AArnott
Copy link
Member

AArnott commented Jul 9, 2021

@mikebattista tip: Use a descriptive title for PRs and make sure you mention the issue # in the PR description. In my list of PRs to review, "Fixes #561" is not a great title, particularly since the 561 in that isn't clickable, making it hard to find out what it actually fixes.

Comment on lines +255 to +256
UnmapViewOfFile2::BaseAddress=MemoryMappedViewHandle
UnmapViewOfFileEx::BaseAddress=MemoryMappedViewHandle
Copy link
Member

Choose a reason for hiding this comment

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

I wonder what the complications are from having two extra methods that release the handle. The RAIIRelease attribute will only point at the first one. So if the second and third would only be called manually, where would the caller have obtained the handle? I wonder if we should leave them accepting IntPtr instead of the new handle struct if that's the type that callers are likely to have.
I've never used any of these APIs, so it would be great to get the opinion of someone familiar with the area.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're supposed to use the UnmapViewOfFile that matches the MapViewOfFile API you called.

This one is interesting which is why I submitted a PR for review instead of just submitting this.

Copy link
Member

Choose a reason for hiding this comment

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

Then I suppose we may need 3 handle types. And as the 2 and Ex methods take more than one parameter, they'll require special recognition by the projection so they know what to pass as the other argument(s).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually looks like the docs say you can mix and match a bit: https://docs.microsoft.com/en-us/windows/win32/api/memoryapi/nf-memoryapi-unmapviewoffile#parameters. If you need the extra flags depending on the scenario, you can unmap with -Ex.

What does .NET do here? Do they just call the base unmap function regardless of how the handle was acquired?

Copy link
Member

Choose a reason for hiding this comment

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

.NET only exposes one SafeHandle AFAIK, and it uses the simply-named release function. But that's to say nothing of which method was used to obtain the handle in the first place, as .NET doesn't expose it.

@riverar
Copy link
Collaborator

riverar commented Nov 6, 2021

This PR is a bit stale, any updates?

@kennykerr kennykerr closed this Mar 24, 2022
@mikebattista mikebattista deleted the user/mikebattista/#561 branch November 16, 2022 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MapViewOfFile should generate an overload returning SafeMemoryMappedViewHandle
4 participants