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

Jump to bottom

Add {Create|Delete}KeyEx to _winreg, doc and test updates #51596

Closed
briancurtin opened this issue Nov 18, 2009 · 13 comments
Closed

Add {Create|Delete}KeyEx to _winreg, doc and test updates #51596

briancurtin opened this issue Nov 18, 2009 · 13 comments
Assignees
@briancurtin
Labels
extension-modules C modules in the Modules dir OS-windows type-feature A feature request or enhancement

Comments

@briancurtin
Copy link
Member

BPO 7347
Nosy @ericvsmith, @briancurtin
Dependencies
  • bpo-7860: 32-bit Python on 64-bit Windows reports incorrect architecture
    		•
    Files
  • winreg_add_createkeyex.patch: patch against r76365
  • winreg_add_createkeyex_v2.patch: patch against r76432
  • issue7347.diff
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/briancurtin'
closed_at = <Date 2010-04-21.23:56:50.737>
created_at = <Date 2009-11-18.17:48:13.967>
labels = ['extension-modules', 'type-feature', 'OS-windows']
title = 'Add {Create|Delete}KeyEx to _winreg, doc and test updates'
updated_at = <Date 2010-04-21.23:56:50.736>
user = 'https://github.com/briancurtin'

    bugs.python.org fields:

    activity = <Date 2010-04-21.23:56:50.736>
actor = 'brian.curtin'
assignee = 'brian.curtin'
closed = True
closed_date = <Date 2010-04-21.23:56:50.737>
closer = 'brian.curtin'
components = ['Extension Modules', 'Windows']
creation = <Date 2009-11-18.17:48:13.967>
creator = 'brian.curtin'
dependencies = ['7860']
files = ['15360', '15370', '16558']
hgrepos = []
issue_num = 7347
keywords = ['patch', '64bit', 'needs review']
message_count = 13.0
messages = ['95434', '95436', '95563', '98251', '98952', '98967', '100286', '101137', '101364', '101374', '102029', '102206', '103926']
nosy_count = 3.0
nosy_names = ['ggenellina', 'eric.smith', 'brian.curtin']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue7347'
versions = ['Python 2.7', 'Python 3.2']
    @briancurtin
    Copy link
    Member Author

    The _winreg module could use the addition of the RegCreateKeyEx call, as
    evidenced by this thread on c.l.py:
    http://mail.python.org/pipermail/python-list/2009-November/614023.html
    This expanded API would benefit users trying to create keys with access
    masks (like KEY_WOW64_64KEY), which you can't set through _winreg.CreateKey.

    The patch includes the change to _winreg.c, along with doc and test
    changes. Comments/suggestions appreciated.

    @briancurtin briancurtin added extension-modules C modules in the Modules dir type-feature A feature request or enhancement labels Nov 18, 2009
    @briancurtin
    Copy link
    Member Author

    Updated patch - c&p mistake in a comment

    @briancurtin
    Copy link
    Member Author

    After looking at this more, I poked around and found a whole lot of
    things either missing or broken with _winreg.

    Changes:

    • documentation missing for *ReflectionKey, and updated a few
      incorrectly documented exceptions in the docstrings
    • QueryReflectionKey always returned False if the underlying API call
      succeeded (used return code from call instead of bool param)
    • addition of RegCreateKeyEx and RegDeleteKeyEx for 64-bit support
    • changed the test suite to cover much more of the _winreg API. There
      are now classes for local, remote, and 64-bit specific tests, with some
      tests in the 64-bit class which are specific to whether Python was
      compiled for 32 or 64-bit due to the differences in how _winreg acts.

    So far everything passes on XP 32-bit, Server 2003 32-bit, and Server
    2003 64-bit with a 32-bit Python. On Server 2003 64-bit with a 64-bit
    Python, I get one failure on test_create_open_delete_for_32bit about
    DeleteKeyEx -- not sure what the deal is, but I'm looking into it.

    There is an added file, delete_regkey.vbs, because 32-bit applications
    can create keys in the 64-bit space, but apparently they cannot be
    deleted with DeleteKey -- they'd need DeleteKeyEx but that's 64-bit
    only. It's used as a cleanup to make sure the key gets deleted in one case.

    @briancurtin briancurtin changed the title Patch - add RegCreateKeyEx to _winreg Add {Create|Delete}KeyEx to _winreg, doc and test updates Nov 20, 2009
    @briancurtin
    Copy link
    Member Author

    After looking into this again, the testing situations are much different than I had originally written.

    Some of the tests aren't really valid exercises. For one, the querying/enabling/disabling of reflection keys aren't good tests as they aren't tested against reflected keys. Windows 7 and Server 2008 R2 act differently in reflection situations when compared to XP/Vista/Server 2003, and I'll need to account for that.

    For now, forget about the current patches, I'm working on a more correct testing approach.

    @briancurtin briancurtin self-assigned this Jan 24, 2010
    @briancurtin
    Copy link
    Member Author

    This needs bpo-7860 for properly figuring out the machine architecture for some 32-bit Python on 64-bit Windows tests.

    @briancurtin
    Copy link
    Member Author

    Attached is what I believe is the complete patch. You'll need to apply the patch on bpo-7860 for proper test coverage of a 32-bit Python running on 64-bit Windows.

    Here's a summary of what's contained:

    1. Documented and tested the previously undocumented and untested *ReflectionKey functions
    2. Added, documented, and tested CreateKeyEx
    3. Added, documented, and tested DeleteKeyEx
    4. Fixed QueryReflectionKey to return the key state rather than the return value of the underlying Windows call.

    Overall the theme is to better our support of 64-bit Windows.

    The testing scenario becomes a bit more involved given the spread of Windows versions supported and their varying level of support of some of the APIs being exposed. I think the tests are documented well enough to explain what scenarios are available and which are being tested, but let me know if more detail is needed.

    I've tested this on the following machines with success:

    1. Windows XP SP2 32-bit
    2. Windows Server 2003 32-bit
    3. Windows Server 2003 64-bit with 32 and 64-bit Python
    4. Windows 7 64-bit with 32 and 64-bit Python

    @briancurtin
    Copy link
    Member Author

    I've uploaded the patch to http://codereview.appspot.com/223088

    @briancurtin
    Copy link
    Member Author

    Fixed a few a/an word changes and a few tab/space issues. Re-uploaded to Rietveld at http://codereview.appspot.com/580041

    @ggenellina
    Copy link
    Mannequin

    ggenellina mannequin commented Mar 20, 2010

    Why the *Ex names? Can't we just add additional arguments to the original names?

    The Python names do not necesarily have to match the API calls. Having QueryValue and QueryValueEx was a mistake in the first place, and I would prefer not continuing doing such things.

    @briancurtin
    Copy link
    Member Author

    RegDeleteKeyEx will only work on a Windows version of 5.2 or greater (Vista/XP x64), and XP is 5.1, so RegDeleteKeyEx can't be a simple drop-in under the "DeleteKey" name.

    CreateKeyEx is different though since it goes as far back as Win2k, and it could be put into CreateKey like you suggest. However, given the way the rest of the module is laid out with *Key and *KeyEx already, it seemed more consistent the way I had written it. I could change this if more people agree with your plan.

    @briancurtin
    Copy link
    Member Author

    Gabriel, besides the *Ex naming, do you see anything wrong with the rest of the patch? I'd like to try and get this into 2.7 before the upcoming beta.

    @briancurtin
    Copy link
    Member Author

    Committed to trunk in r79620. I'll do the forward port after 2.7b1.

    @briancurtin
    Copy link
    Member Author

    Ported to py3k in r80329.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees

    @briancurtin briancurtin

    Labels
    extension-modules C modules in the Modules dir OS-windows type-feature A feature request or enhancement
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    1 participant
    @briancurtin