Accept raw REGSAM access masks in open/create key helpers#641
Conversation
benhillis
force-pushed
the
reg-regsam-access
branch
from
3e8f57d to
5bf5590
Compare
| static constexpr DWORD map_key_access(::wil::reg::key_access access) WI_NOEXCEPT | ||
| { | ||
| switch (access) | ||
| { | ||
| case ::wil::reg::key_access::read: | ||
| return KEY_READ; | ||
| case ::wil::reg::key_access::write: | ||
| return KEY_WRITE; | ||
| case ::wil::reg::key_access::readwrite: | ||
| return KEY_ALL_ACCESS; | ||
| case ::wil::reg::key_access::read64: | ||
| return KEY_READ | KEY_WOW64_64KEY; | ||
| case ::wil::reg::key_access::write64: | ||
| return KEY_WRITE | KEY_WOW64_64KEY; | ||
| case ::wil::reg::key_access::readwrite64: | ||
| return KEY_ALL_ACCESS | KEY_WOW64_64KEY; | ||
| } | ||
| FAIL_FAST(); | ||
| RESULT_NORETURN_RESULT(0); | ||
| } |
There was a problem hiding this comment.
Without thinking about it much... I wonder if it's time to just change key_access so its values just represent these values literally and then define enum flag operations for the type... That would allow us to keep using the key_access type and get rid of this new access_mask and just static_cast when we want the values.
There was a problem hiding this comment.
@keith-horton for his opinion since it seems @citelao is now at a different company
There was a problem hiding this comment.
I'm personally good with Ben's change. we had long conversations about this years ago and I don't recall the reasoning specifically why we landed on this model. I'm personally good with the current model.
There was a problem hiding this comment.
The main thing is that this is like the fourth or fifth PR to expand the set of allowed values (albeit this one does so without touching the enum, but the intent is the same). I think we have enough data to suggest that this is not the correct long-term design - it would be far more beneficial to just allow consumers to say key_access{ FLAG1 | FLAG2... } than have to issue a PR and wait for it to get merged/released.
There was a problem hiding this comment.
ok. that suggestion makes sense to me. @benhillis, can you make the change that Duncan suggested?
There was a problem hiding this comment.
Hi! Yes, the original intent of the enum was to give people a type-safe way of specifying these flags, since I like to add as much type-safety as possible, and I assumed a small list of modes (read, readwrite, etc) would be helpful. The fallback was (at one point, I think) to static_cast an int into a ::wil::reg::key_access.
Since that abstraction is causing problems now, and people need the full flexibility, I have no issue with the proposed change from my end.
Hope that helps!
There was a problem hiding this comment.
Hey @citelao! Looks like you found something new-and-improved after leaving Microsoft :)
This change looks good.
| static constexpr DWORD map_key_access(::wil::reg::key_access access) WI_NOEXCEPT | ||
| { | ||
| switch (access) | ||
| { | ||
| case ::wil::reg::key_access::read: | ||
| return KEY_READ; | ||
| case ::wil::reg::key_access::write: | ||
| return KEY_WRITE; | ||
| case ::wil::reg::key_access::readwrite: | ||
| return KEY_ALL_ACCESS; | ||
| case ::wil::reg::key_access::read64: | ||
| return KEY_READ | KEY_WOW64_64KEY; | ||
| case ::wil::reg::key_access::write64: | ||
| return KEY_WRITE | KEY_WOW64_64KEY; | ||
| case ::wil::reg::key_access::readwrite64: | ||
| return KEY_ALL_ACCESS | KEY_WOW64_64KEY; | ||
| } | ||
| FAIL_FAST(); | ||
| RESULT_NORETURN_RESULT(0); | ||
| } |
There was a problem hiding this comment.
I'm personally good with Ben's change. we had long conversations about this years ago and I don't recall the reasoning specifically why we landed on this model. I'm personally good with the current model.
Redefine wil::reg::key_access as a DWORD-backed flag enum whose values are
the REGSAM masks they represent (read = KEY_READ, read64 = KEY_READ |
KEY_WOW64_64KEY, etc.) and add DEFINE_ENUM_FLAG_OPERATORS. Callers who need
an access right not covered by a named value can now pass a raw mask
directly, e.g. key_access{KEY_QUERY_VALUE | KEY_SET_VALUE}, without a new
overload or enum value. Existing named key_access call sites are unchanged.
get_access_flags now just returns the mask value. Adds RegistryTests
coverage for raw-mask open/create and the flag operators.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
benhillis
force-pushed
the
reg-regsam-access
branch
from
0caa334 to
ad9d67f
Compare
4 participants
Adds an
access_masktype accepted by the open/create key helpers, implicitly constructible fromkey_accessor a raw REGSAM, so callers can pass custom access rights. Includes Catch2[registry]tests (and compile-time static_asserts) passing in normal & noexcept configs. Part of enabling WSL to drop its in-house registry helper in favor of wil::reg.