Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2507263002:
Make nt_registry Create/OpenRegKey return a scoped object
DescriptionMake nt_registry Create/OpenRegKey return a scoped object
Attempts to make ownership of lifetime of HANDLE more clear.
Note that previously, INVALID_HANDLE_VALUE was used, but never actually
tested against -- null is the correct invalid handle value for the
registry functions.
R=pennymac@chromium.org
BUG=665635
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
Patch Set 1 #
Total comments: 48
Patch Set 2 : fixes #Patch Set 3 : . #Patch Set 4 : null #Patch Set 5 : fixes #
Total comments: 2
Messages
Total messages: 28 (9 generated)
Description was changed from ========== Make nt_registry Create/OpenRegKey return a scoped object R=pennymac@chromium.org BUG=665635 ========== to ========== Make nt_registry Create/OpenRegKey return a scoped object R=pennymac@chromium.org BUG=665635 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Description was changed from ========== Make nt_registry Create/OpenRegKey return a scoped object R=pennymac@chromium.org BUG=665635 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Make nt_registry Create/OpenRegKey return a scoped object Attempts to make ownership of lifetime of HANDLE more clear. R=pennymac@chromium.org BUG=665635 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Hey Penny, This is what I was thinking. I tried not to go overboard on magic stuff to keep to the basic/raw design you mentioned in the bug. But I think the usage, e.g. in the QueryRegValueXyzs is simpler and easier to get right since the caller doesn't have to worry about forgetting to close properly on all exit paths from a function that opens/creates the key. I don't feel that strongly about it though if you don't like it. :)
Scott, I think this looks great, small and more tidy - thanks for whipping it up! I think you'll have to take a quick look at chrome_elf/nt_registry/nt_registry_unittest.cc to adjust it for the changes. The tests currently run as part of chrome_elf_unittests suite. You can filter down to NtRegistryTest*. You expanded my c++ brain with the constructors. See my questions. **See comment in nt_registry.h: "Let's make this class generic ScopedHANDLE. Nothing about a Windows HANDLE is specific to registry. It would be easy in the future to re-use this for any HANDLE usage. Thanks! https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... File chrome_elf/nt_registry/nt_registry.cc (right): https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.cc:573: if (!g_initialized) Let's first add a check for is_valid(). I'd rather not do any of this if |handle_| is INVALID_HANDLE_VALUE or NULL. I definitely don't want to clash with any debug or safety measures related to calling NtClose on an invalid handle. https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.cc:675: // |key_path| can be null or empty, but it can't be longer than Above this, let's set a default error code (for if we want to bail out before call to NtOpenKey). if (error_code) *error_code = STATUS_UNSUCCESSFUL; https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.cc:684: NTSTATUS status = STATUS_UNSUCCESSFUL; Delete this initialization. https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.cc:687: HANDLE result_handle = INVALID_HANDLE_VALUE; Move initialization of HANDLE result_handle down to just above the call to g_nt_open_key_ex (line 704). https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.cc:704: status = g_nt_open_key_ex(&result_handle, access, &obj, 0); NTSTATUS status = ... https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.cc:706: if (error_code) Let's move this down a tad, so that we only set |error_code| on failure. if (!NT_SUCCESS(status)) { // See if caller wants the NTSTATUS. if (error_code) *error_code = status; return ScopedHANDLE(); } return ScopedHANDLE(result_handle); The above should replace line 705 to 712. https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.cc:718: return NT_SUCCESS(g_nt_delete_key(key)); Do my ocd a favour and add an empty line above this return. :D https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.cc:729: return DeleteRegKey(key.get()); Do my ocd a favour and add an empty line above this return. :D https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.cc:804: return QueryRegValueDWORD(key.get(), value_name, out_dword); Do my ocd a favour and add an empty line above this return. :D https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.cc:836: return QueryRegValueSZ(key.get(), value_name, out_sz); Do my ocd a favour and add an empty line above this return. :D https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.cc:883: return QueryRegValueMULTISZ(key.get(), value_name, out_multi_sz); Do my ocd a favour and add an empty line above this return. :D https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.cc:928: return SetRegValueDWORD(key.get(), value_name, value); Do my ocd a favour and add an empty line above this return. :D https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.cc:955: return SetRegValueSZ(key.get(), value_name, value); Do my ocd a favour and add an empty line above this return. :D https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.cc:986: reinterpret_cast<BYTE*>(builder.data()), I'm just curious why this got moved down... it wasn't over 80 chars. (I honestly prefer one argument per line, but that's not our coding standard.) https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.cc:1000: return SetRegValueMULTISZ(key.get(), value_name, values); Do my ocd a favour and add an empty line above this return. :D https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... File chrome_elf/nt_registry/nt_registry.h (right): https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.h:47: Please add a small class comment. " // ScopedHANDLE wraps a generic Windows HANDLE, allowing it to be // closed automatically (via NtClose) when the object goes out of scope. // // - Use is_valid() function to check validity of internal HANDLE. // - Use release() function to take ownership of the raw internal HANDLE. " https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.h:48: class ScopedRegKeyHANDLE { Let's make this class generic ScopedHANDLE. Nothing about a Windows HANDLE is specific to registry. It would be easy in the future to re-use this for any HANDLE usage. https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.h:51: explicit ScopedRegKeyHANDLE(HANDLE handle); Just for my own c++ learnin': why did you think to add explicit only to this constructor (and not the others... or not at all)? Is it because we want to force handling of the raw HANDLE passed in? Or is this just to stop a caller from doing cross-type assignment like "ScopedHANDLE a = HANDLE win_handle"? <-- Wouldn't the compiler scream? From what I understand, the two constructors below will behave as converting constructors - which lets one "copy initialize" with it? e.g. ScopedHANDLE b(HANDLE win_handle); ScopedHANDLE a = b; ^^ which will delete b during construction of a? https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.h:52: ScopedRegKeyHANDLE(ScopedRegKeyHANDLE&& rvalue) : handle_(rvalue.release()) {} Why did you have to release and then set in this constructor and not the ones below? I'm assuming it's because the ones below do implicit copying of the object contents (including |handle_|) before deleting the old, while implicit copying of a pointer to an object does not copy the contents?? Could I actually use this like: ScopedHANDLE b(HANDLE win_handle); ScopedHANDLE a = &b; // <--- seriously? https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.h:53: ScopedRegKeyHANDLE(const ScopedRegKeyHANDLE&) = delete; Did you need to add one with "const"? Is that just so this constructor can take both const or non-const argument? And we didn't add "const" in the above constructor because it's a pointer to an object, which would probably never be const? https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.h:58: bool is_valid() const { return handle_ != INVALID_HANDLE_VALUE; } I think I'd like is_valid to *also* check for NULL. Just to be extra careful. https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.h:59: HANDLE release(); Please add tiny comment like: " // Caller is responsible for closing the returned HANDLE if != INVALID_HANDLE_VALUE. " https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.h:73: // - Optional |out_handle|. If nullptr, function will close handle when done. Change this comment to be: // - On success, the returned object will hold the open handle to the // deepest subkey. https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.h:88: bool DeleteRegKey(HANDLE handle); I was fine with the name change, but then I realized that all the HANDLE arguments further down are called |key| as well. So let's change it back to "key" for consistency.
Thanks! (Sorry if I was confused or explaining things you already know for the scoped class... if we're still talking past each other feel free to ping and we can talk through it.) https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... File chrome_elf/nt_registry/nt_registry.cc (right): https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.cc:573: if (!g_initialized) On 2016/11/21 21:35:46, penny wrote: > Let's first add a check for is_valid(). I'd rather not do any of this if > |handle_| is INVALID_HANDLE_VALUE or NULL. > > I definitely don't want to clash with any debug or safety measures related to > calling NtClose on an invalid handle. Good catch! Done. https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.cc:675: // |key_path| can be null or empty, but it can't be longer than On 2016/11/21 21:35:47, penny wrote: > Above this, let's set a default error code (for if we want to bail out before > call to NtOpenKey). > > if (error_code) > *error_code = STATUS_UNSUCCESSFUL; Done. https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.cc:684: NTSTATUS status = STATUS_UNSUCCESSFUL; On 2016/11/21 21:35:46, penny wrote: > Delete this initialization. Done. https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.cc:687: HANDLE result_handle = INVALID_HANDLE_VALUE; On 2016/11/21 21:35:47, penny wrote: > Move initialization of HANDLE result_handle down to just above the call to > g_nt_open_key_ex (line 704). Done. https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.cc:704: status = g_nt_open_key_ex(&result_handle, access, &obj, 0); On 2016/11/21 21:35:47, penny wrote: > NTSTATUS status = ... Done. https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.cc:706: if (error_code) On 2016/11/21 21:35:46, penny wrote: > Let's move this down a tad, so that we only set |error_code| on failure. > > if (!NT_SUCCESS(status)) { > // See if caller wants the NTSTATUS. > if (error_code) > *error_code = status; > return ScopedHANDLE(); > } > > return ScopedHANDLE(result_handle); > > > The above should replace line 705 to 712. Done. https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.cc:718: return NT_SUCCESS(g_nt_delete_key(key)); On 2016/11/21 21:35:47, penny wrote: > Do my ocd a favour and add an empty line above this return. :D Ugly! If you insist. https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.cc:729: return DeleteRegKey(key.get()); On 2016/11/21 21:35:47, penny wrote: > Do my ocd a favour and add an empty line above this return. :D Done. https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.cc:804: return QueryRegValueDWORD(key.get(), value_name, out_dword); On 2016/11/21 21:35:47, penny wrote: > Do my ocd a favour and add an empty line above this return. :D Done. https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.cc:836: return QueryRegValueSZ(key.get(), value_name, out_sz); On 2016/11/21 21:35:47, penny wrote: > Do my ocd a favour and add an empty line above this return. :D Done. https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.cc:883: return QueryRegValueMULTISZ(key.get(), value_name, out_multi_sz); On 2016/11/21 21:35:47, penny wrote: > Do my ocd a favour and add an empty line above this return. :D Done. https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.cc:928: return SetRegValueDWORD(key.get(), value_name, value); On 2016/11/21 21:35:47, penny wrote: > Do my ocd a favour and add an empty line above this return. :D Done. https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.cc:955: return SetRegValueSZ(key.get(), value_name, value); On 2016/11/21 21:35:46, penny wrote: > Do my ocd a favour and add an empty line above this return. :D Done. https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.cc:986: reinterpret_cast<BYTE*>(builder.data()), On 2016/11/21 21:35:47, penny wrote: > I'm just curious why this got moved down... it wasn't over 80 chars. Forgot to reformat, done. > > (I honestly prefer one argument per line, but that's not our coding standard.) Agreed. (Crashpad dissented in this way and has a customized clang-format file. Shhhh!) https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.cc:1000: return SetRegValueMULTISZ(key.get(), value_name, values); On 2016/11/21 21:35:46, penny wrote: > Do my ocd a favour and add an empty line above this return. :D Done. https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... File chrome_elf/nt_registry/nt_registry.h (right): https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.h:47: On 2016/11/21 21:35:48, penny wrote: > Please add a small class comment. > > " > // ScopedHANDLE wraps a generic Windows HANDLE, allowing it to be > // closed automatically (via NtClose) when the object goes out of scope. > // > // - Use is_valid() function to check validity of internal HANDLE. > // - Use release() function to take ownership of the raw internal HANDLE. > " Done. https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.h:48: class ScopedRegKeyHANDLE { On 2016/11/21 21:35:47, penny wrote: > Let's make this class generic ScopedHANDLE. Nothing about a Windows HANDLE is > specific to registry. It would be easy in the future to re-use this for any > HANDLE usage. The INVALID_HANDLE_VALUE vs nullptr was why I didn't do that. I think it's better to keep File-y handles separate from kernel handles. How about ScopedFileHANDLE? https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.h:51: explicit ScopedRegKeyHANDLE(HANDLE handle); On 2016/11/21 21:35:47, penny wrote: > Just for my own c++ learnin': why did you think to add explicit only to this > constructor (and not the others... or not at all)? Is it because we want to > force handling of the raw HANDLE passed in? Or is this just to stop a caller > from doing cross-type assignment like "ScopedHANDLE a = HANDLE win_handle"? <-- > Wouldn't the compiler scream? Yeah, it's for this. Otherwise, any old HANDLE could magically convert into a ScopedHANDLE which we don't really want. This way it has to be explicit, either in a constructor statement, like one of these: ScopedHandle x(value); ScopedHandle x = ScopedHandle(SomeFunctionThatReturnsAHANDLE()); But really I'm just obeying https://google.github.io/styleguide/cppguide.html#Implicit_Conversions :). > > From what I understand, the two constructors below will behave as converting > constructors - which lets one "copy initialize" with it? I'd actually never thought of that! I think you can't really put explicit on copy/move ctors because they wouldn't be able to do their magic. > > e.g. > ScopedHANDLE b(HANDLE win_handle); > ScopedHANDLE a = b; > > ^^ which will delete b during construction of a? Yes, that's definitely why we don't want to allow operator=. https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.h:52: ScopedRegKeyHANDLE(ScopedRegKeyHANDLE&& rvalue) : handle_(rvalue.release()) {} On 2016/11/21 21:35:47, penny wrote: > Why did you have to release and then set in this constructor and not the ones > below? I'm assuming it's because the ones below do implicit copying of the > object contents (including |handle_|) before deleting the old, while implicit > copying of a pointer to an object does not copy the contents?? > > Could I actually use this like: > ScopedHANDLE b(HANDLE win_handle); > ScopedHANDLE a = &b; // <--- seriously? The ones below are =delete, so they're not implemented. This one is a move constructor, so the rvalue reference (&&) that's coming in is being moved into this object. So we don't want it to own the lifetime of the HANDLE it used to contain, so we're moving that value into the one that's being constructed here. https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.h:53: ScopedRegKeyHANDLE(const ScopedRegKeyHANDLE&) = delete; On 2016/11/21 21:35:47, penny wrote: > Did you need to add one with "const"? Is that just so this constructor can take > both const or non-const argument? And we didn't add "const" in the above > constructor because it's a pointer to an object, which would probably never be > const? You kind of lost me by this point. But the operator= is a typo. (Thanks!) That one should have a const. The idea is that the const ones can accept both const and non-const, so we only need the const. https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.h:58: bool is_valid() const { return handle_ != INVALID_HANDLE_VALUE; } On 2016/11/21 21:35:47, penny wrote: > I think I'd like is_valid to *also* check for NULL. Just to be extra careful. How about I assert for non-null? I'd prefer to keep file handle and kernel handle separate. https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.h:59: HANDLE release(); On 2016/11/21 21:35:47, penny wrote: > Please add tiny comment like: > > " > // Caller is responsible for closing the returned HANDLE if != > INVALID_HANDLE_VALUE. > " Done. Also made it private for now, since we don't need to expose it. https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.h:73: // - Optional |out_handle|. If nullptr, function will close handle when done. On 2016/11/21 21:35:47, penny wrote: > Change this comment to be: > > // - On success, the returned object will hold the open handle to the > // deepest subkey. Done. https://codereview.chromium.org/2507263002/diff/1/chrome_elf/nt_registry/nt_r... chrome_elf/nt_registry/nt_registry.h:88: bool DeleteRegKey(HANDLE handle); On 2016/11/21 21:35:47, penny wrote: > I was fine with the name change, but then I realized that all the HANDLE > arguments further down are called |key| as well. > > So let's change it back to "key" for consistency. Oops, sorry, unintentional. I was fiddling a bit more than this CL, but then walked it back and forgot to return this to what it was. Done.
ping
On 2017/01/02 22:37:16, scottmg wrote: > ping Sorry! Lost this on my todo list. As per our discussion just now, plan is to adjust this ScopedHandle class to default to null (and assert if invalid_handle_value in is_valid()). Also please add a good comment above the class definition about not using this for any APIs (CreateFile) that deal in invalid_handle_value. Thanks Scott!
Description was changed from ========== Make nt_registry Create/OpenRegKey return a scoped object Attempts to make ownership of lifetime of HANDLE more clear. R=pennymac@chromium.org BUG=665635 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Make nt_registry Create/OpenRegKey return a scoped object Attempts to make ownership of lifetime of HANDLE more clear. Note that previously, INVALID_HANDLE_VALUE was used, but never actually tested against -- null is the correct invalid handle value for the registry functions. R=pennymac@chromium.org BUG=665635 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
Updated description to mention invalid_handle_value vs null.
On 2017/01/26 21:29:18, scottmg wrote: > Updated description to mention invalid_handle_value vs null. So close Scott. - I don't think you've tried running the tests yet, or they'd fail to even compile. chrome_elf/nt_registry/nt_registry_unittest.cc needs to be updated. Lots of calls to nt::CreateRegKey() and nt::OpenRegKey(), which now return a ScopedHANDLE instead of bool. These tests run under "chrome_elf_unittests". - Small nit: can we move the implementation of is_valid() out of nt_registry.h and into .cc? Then you can remove your added "#include <assert.h>" from the .h file as well. Thanks!
Patchset #5 (id:80001) has been deleted
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/30 19:36:26, penny wrote: > On 2017/01/26 21:29:18, scottmg wrote: > > Updated description to mention invalid_handle_value vs null. > > So close Scott. > > - I don't think you've tried running the tests yet, or they'd fail to even > compile. > chrome_elf/nt_registry/nt_registry_unittest.cc needs to be updated. Lots of > calls to nt::CreateRegKey() and nt::OpenRegKey(), which now return a > ScopedHANDLE instead of bool. These tests run under "chrome_elf_unittests". (There's tests!) Done. > > - Small nit: can we move the implementation of is_valid() out of nt_registry.h > and into .cc? Then you can remove your added "#include <assert.h>" from the .h > file as well. Done. > > Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Irritating drive-by: why not RAII the ntregistry api itself? It seems like it'd be easier to use it correctly.
On 2017/02/07 08:27:57, grt (UTC plus 1) wrote: > Irritating drive-by: why not RAII the ntregistry api itself? It seems like it'd > be easier to use it correctly. LGTM Thanks Scott. grt, sorry what is RAII short for?
On 2017/02/08 21:19:43, penny wrote: > On 2017/02/07 08:27:57, grt (UTC plus 1) wrote: > > Irritating drive-by: why not RAII the ntregistry api itself? It seems like > it'd > > be easier to use it correctly. > > LGTM > > Thanks Scott. > > grt, sorry what is RAII short for? Resource Acquisition Is Initialization (i.e. what we're doing here by wrapping the acquisitions in a scoped thingy so that there's a clear owner (the ScopedHANDLE) with a destructor that tidies up. I'm not sure what level you were thinking though Greg. Are you thinking Query/Set etc. become methods on an NtRegistryHandle or something?
On 2017/02/08 21:23:32, scottmg wrote: > On 2017/02/08 21:19:43, penny wrote: > > On 2017/02/07 08:27:57, grt (UTC plus 1) wrote: > > > Irritating drive-by: why not RAII the ntregistry api itself? It seems like > > it'd > > > be easier to use it correctly. > > > > LGTM > > > > Thanks Scott. > > > > grt, sorry what is RAII short for? > > Resource Acquisition Is Initialization (i.e. what we're doing here by wrapping > the acquisitions in a scoped thingy so that there's a clear owner (the > ScopedHANDLE) with a destructor that tidies up. And apparently I need RAII to match my nested parentheses! > > I'm not sure what level you were thinking though Greg. Are you thinking > Query/Set etc. become methods on an NtRegistryHandle or something? Since you're here anyway Greg, I need your blessing and/or comments for chrome/install_static.
On 2017/02/08 21:23:32, scottmg wrote: > On 2017/02/08 21:19:43, penny wrote: > > On 2017/02/07 08:27:57, grt (UTC plus 1) wrote: > > > Irritating drive-by: why not RAII the ntregistry api itself? It seems like > > it'd > > > be easier to use it correctly. > > > > LGTM > > > > Thanks Scott. > > > > grt, sorry what is RAII short for? > > Resource Acquisition Is Initialization (i.e. what we're doing here by wrapping > the acquisitions in a scoped thingy so that there's a clear owner (the > ScopedHANDLE) with a destructor that tidies up. > > I'm not sure what level you were thinking though Greg. Are you thinking > Query/Set etc. become methods on an NtRegistryHandle or something? Yes. The API looks more cumbersome than needed right now, and has a built-in foot-gun since use of .get() is mandatory.
grt@chromium.org changed reviewers: + grt@chromium.org
https://codereview.chromium.org/2507263002/diff/100001/chrome/install_static/... File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2507263002/diff/100001/chrome/install_static/... chrome/install_static/install_util.cc:355: if (!key.is_valid()) wdyt of an explicit operator bool so this can be written as: if (!key) https://codereview.chromium.org/2507263002/diff/100001/chrome/install_static/... chrome/install_static/install_util.cc:358: return nt::SetRegValueDWORD(key.get(), kRegValueChromeStatsSample, i think that return key.SetRegValueDWORD(...); is quite a bit nicer. is there a reason not to go this route?
On 2017/02/08 21:44:02, grt (UTC plus 1) wrote: > https://codereview.chromium.org/2507263002/diff/100001/chrome/install_static/... > File chrome/install_static/install_util.cc (right): > > https://codereview.chromium.org/2507263002/diff/100001/chrome/install_static/... > chrome/install_static/install_util.cc:355: if (!key.is_valid()) > wdyt of an explicit operator bool so this can be written as: > if (!key) > > https://codereview.chromium.org/2507263002/diff/100001/chrome/install_static/... > chrome/install_static/install_util.cc:358: return > nt::SetRegValueDWORD(key.get(), kRegValueChromeStatsSample, > i think that > return key.SetRegValueDWORD(...); > is quite a bit nicer. is there a reason not to go this route? So, first off, I think Scott should land this CL as is - his original work (way back) had nothing to do with changing NtRegistry, and it was only a comment by a reviewer that prompted the addition of a scoped object class. The truth is, I only went along with adding the scoped class grudgingly. NtRegistry is not meant to be a replacement for base registry APIs. It's meant to be a raw shim over ntdll. Wrapping a HANDLE in a class already adds a sad layer of complexity where we had to decide whether to internally handle INVALID_HANDLE_VALUE vs NULL. Hidden assumptions inside a class. I feel dirty just writing this now. Almost wish we weren't doing this CL now Scott. :( So I really don't want to add more fluffy "object orientation" right this moment. I only wrote this tool with a vision for low-level "security-ish" tools, and definitely for places that don't deal with base. Having said that, if you have a different vision for using NtRegistry in another way Greg, I'm wide open to chatting about it outside of this review! (I would expect scoped, "classy", "objectified" use of NtRegistry to exist in a higher layer like base APIs - if base APIs for example wrapped NtRegistry instead of advapi32.)
On 2017/02/09 20:55:01, penny wrote: > On 2017/02/08 21:44:02, grt (UTC plus 1) wrote: > > > https://codereview.chromium.org/2507263002/diff/100001/chrome/install_static/... > > File chrome/install_static/install_util.cc (right): > > > > > https://codereview.chromium.org/2507263002/diff/100001/chrome/install_static/... > > chrome/install_static/install_util.cc:355: if (!key.is_valid()) > > wdyt of an explicit operator bool so this can be written as: > > if (!key) > > > > > https://codereview.chromium.org/2507263002/diff/100001/chrome/install_static/... > > chrome/install_static/install_util.cc:358: return > > nt::SetRegValueDWORD(key.get(), kRegValueChromeStatsSample, > > i think that > > return key.SetRegValueDWORD(...); > > is quite a bit nicer. is there a reason not to go this route? > > So, first off, I think Scott should land this CL as is - his original work (way > back) had nothing to do with changing NtRegistry, and it was only a comment by a > reviewer that prompted the addition of a scoped object class. > > The truth is, I only went along with adding the scoped class grudgingly. > NtRegistry is not meant to be a replacement for base registry APIs. It's meant > to be a raw shim over ntdll. Wrapping a HANDLE in a class already adds a sad > layer of complexity where we had to decide whether to internally handle > INVALID_HANDLE_VALUE vs NULL. Hidden assumptions inside a class. I feel dirty > just writing this now. Almost wish we weren't doing this CL now Scott. :( So > I really don't want to add more fluffy "object orientation" right this moment. > > I only wrote this tool with a vision for low-level "security-ish" tools, and > definitely for places that don't deal with base. > > Having said that, if you have a different vision for using NtRegistry in another > way Greg, I'm wide open to chatting about it outside of this review! > > (I would expect scoped, "classy", "objectified" use of NtRegistry to exist in a > higher layer like base APIs - if base APIs for example wrapped NtRegistry > instead of advapi32.) Yeah, the reason I did only this was primarily incrementalism. I think some of the confusion is that nt_registry.h is a combination of the lowest level, along with some wrapper-level. That is, there's the lowest level which are basically just GetProcAddress() helpers, but then there's also a lot of functions listed as "WRAPPER" which are higher level. For those higher level ones, I think there's a reasonable expectation that they should be more user-friendly and so RAII makes sense. More concretely, we could do something like: keep functions for Create, Open, Delete, Close, (raw) Query, (raw) Set. Those would work on raw HANDLEs. Then (perhaps in another header) we'd have the base-like wrappers that manage HANDLE lifetime, wrap a key up into a more abstract concept with various getters and setters for multisz and so on. But tbh, three months on, I've sort of lost the plot, and don't care too much whether we land this or not. :)
On 2017/02/09 21:30:20, scottmg wrote: > On 2017/02/09 20:55:01, penny wrote: > > On 2017/02/08 21:44:02, grt (UTC plus 1) wrote: > > > > > > https://codereview.chromium.org/2507263002/diff/100001/chrome/install_static/... > > > File chrome/install_static/install_util.cc (right): > > > > > > > > > https://codereview.chromium.org/2507263002/diff/100001/chrome/install_static/... > > > chrome/install_static/install_util.cc:355: if (!key.is_valid()) > > > wdyt of an explicit operator bool so this can be written as: > > > if (!key) > > > > > > > > > https://codereview.chromium.org/2507263002/diff/100001/chrome/install_static/... > > > chrome/install_static/install_util.cc:358: return > > > nt::SetRegValueDWORD(key.get(), kRegValueChromeStatsSample, > > > i think that > > > return key.SetRegValueDWORD(...); > > > is quite a bit nicer. is there a reason not to go this route? > > > > So, first off, I think Scott should land this CL as is - his original work > (way > > back) had nothing to do with changing NtRegistry, and it was only a comment by > a > > reviewer that prompted the addition of a scoped object class. > > > > The truth is, I only went along with adding the scoped class grudgingly. > > NtRegistry is not meant to be a replacement for base registry APIs. It's > meant > > to be a raw shim over ntdll. Wrapping a HANDLE in a class already adds a sad > > layer of complexity where we had to decide whether to internally handle > > INVALID_HANDLE_VALUE vs NULL. Hidden assumptions inside a class. I feel > dirty > > just writing this now. Almost wish we weren't doing this CL now Scott. :( > So > > I really don't want to add more fluffy "object orientation" right this moment. > > > > I only wrote this tool with a vision for low-level "security-ish" tools, and > > definitely for places that don't deal with base. > > > > Having said that, if you have a different vision for using NtRegistry in > another > > way Greg, I'm wide open to chatting about it outside of this review! > > > > (I would expect scoped, "classy", "objectified" use of NtRegistry to exist in > a > > higher layer like base APIs - if base APIs for example wrapped NtRegistry > > instead of advapi32.) > > Yeah, the reason I did only this was primarily incrementalism. Makes sense. > I think some of the confusion is that nt_registry.h is a combination of the > lowest level, along with some wrapper-level. That is, there's the lowest level > which are basically just GetProcAddress() helpers, but then there's also a lot > of functions listed as "WRAPPER" which are higher level. For those higher level > ones, I think there's a reasonable expectation that they should be more > user-friendly and so RAII makes sense. > > More concretely, we could do something like: keep functions for Create, Open, > Delete, Close, (raw) Query, (raw) Set. Those would work on raw HANDLEs. > > Then (perhaps in another header) we'd have the base-like wrappers that manage > HANDLE lifetime, wrap a key up into a more abstract concept with various getters > and setters for multisz and so on. Sounds fine to me. > But tbh, three months on, I've sort of lost the plot, and don't care too much > whether we land this or not. :) Heh. I won't block landing this, just pointing out that I think RAII is a good thing and should be used. Just because accessing the Nt registry funcs is low level isn't an argument that we shouldn't have a sane API for doing so.
On 2017/02/10 09:48:28, grt (AFK until Feb 20) wrote: > On 2017/02/09 21:30:20, scottmg wrote: > > On 2017/02/09 20:55:01, penny wrote: > > > On 2017/02/08 21:44:02, grt (UTC plus 1) wrote: > > > > > > > > > > https://codereview.chromium.org/2507263002/diff/100001/chrome/install_static/... > > > > File chrome/install_static/install_util.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2507263002/diff/100001/chrome/install_static/... > > > > chrome/install_static/install_util.cc:355: if (!key.is_valid()) > > > > wdyt of an explicit operator bool so this can be written as: > > > > if (!key) > > > > > > > > > > > > > > https://codereview.chromium.org/2507263002/diff/100001/chrome/install_static/... > > > > chrome/install_static/install_util.cc:358: return > > > > nt::SetRegValueDWORD(key.get(), kRegValueChromeStatsSample, > > > > i think that > > > > return key.SetRegValueDWORD(...); > > > > is quite a bit nicer. is there a reason not to go this route? > > > > > > So, first off, I think Scott should land this CL as is - his original work > > (way > > > back) had nothing to do with changing NtRegistry, and it was only a comment > by > > a > > > reviewer that prompted the addition of a scoped object class. > > > > > > The truth is, I only went along with adding the scoped class grudgingly. > > > NtRegistry is not meant to be a replacement for base registry APIs. It's > > meant > > > to be a raw shim over ntdll. Wrapping a HANDLE in a class already adds a > sad > > > layer of complexity where we had to decide whether to internally handle > > > INVALID_HANDLE_VALUE vs NULL. Hidden assumptions inside a class. I feel > > dirty > > > just writing this now. Almost wish we weren't doing this CL now Scott. :( > > So > > > I really don't want to add more fluffy "object orientation" right this > moment. > > > > > > I only wrote this tool with a vision for low-level "security-ish" tools, and > > > definitely for places that don't deal with base. > > > > > > Having said that, if you have a different vision for using NtRegistry in > > another > > > way Greg, I'm wide open to chatting about it outside of this review! > > > > > > (I would expect scoped, "classy", "objectified" use of NtRegistry to exist > in > > a > > > higher layer like base APIs - if base APIs for example wrapped NtRegistry > > > instead of advapi32.) > > > > Yeah, the reason I did only this was primarily incrementalism. > > Makes sense. > > > I think some of the confusion is that nt_registry.h is a combination of the > > lowest level, along with some wrapper-level. That is, there's the lowest level > > which are basically just GetProcAddress() helpers, but then there's also a lot > > of functions listed as "WRAPPER" which are higher level. For those higher > level > > ones, I think there's a reasonable expectation that they should be more > > user-friendly and so RAII makes sense. > > > > More concretely, we could do something like: keep functions for Create, Open, > > Delete, Close, (raw) Query, (raw) Set. Those would work on raw HANDLEs. > > > > Then (perhaps in another header) we'd have the base-like wrappers that manage > > HANDLE lifetime, wrap a key up into a more abstract concept with various > getters > > and setters for multisz and so on. > > Sounds fine to me. > > > But tbh, three months on, I've sort of lost the plot, and don't care too much > > whether we land this or not. :) > > Heh. I won't block landing this, just pointing out that I think RAII is a good > thing and should be used. Just because accessing the Nt registry funcs is low > level isn't an argument that we shouldn't have a sane API for doing so. Sorry for this dragging on. I've made a new ticket for myself: crbug/693215. Scott, feel free to discard this CL if you feel comfortable with that. Probably better to do it all at once to restructure NtRegistry a bit, as you've both recommended.
sgtm. thanks for taking this on. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
