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

Winreg module doesn't support REG_QWORD, small DWORD doc update #67215

Closed
mgrandi mannequin opened this issue Dec 10, 2014 · 24 comments
Closed

Winreg module doesn't support REG_QWORD, small DWORD doc update #67215

mgrandi mannequin opened this issue Dec 10, 2014 · 24 comments

Comments

@mgrandi
Copy link
Mannequin

mgrandi mannequin commented Dec 10, 2014

BPO 23026
Nosy @pfmoore, @tjguk, @zware, @mgrandi, @eryksun, @zooba, @https://github.com/hakril
Files
  • winreg_add_qword_constants_part1_markgrandi.patch
  • winreg_add_qword_constants_part2_markgrandi.patch: updated patch, doc changes + code changes
  • winreg_qword_patch_with_test.patch
  • 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 = None
    closed_at = <Date 2016-05-25.18:40:42.251>
    created_at = <Date 2014-12-10.20:13:53.879>
    labels = ['OS-windows']
    title = "Winreg module doesn't support REG_QWORD, small DWORD doc update"
    updated_at = <Date 2016-06-09.13:17:24.746>
    user = 'https://github.com/mgrandi'

    bugs.python.org fields:

    activity = <Date 2016-06-09.13:17:24.746>
    actor = 'python-dev'
    assignee = 'none'
    closed = True
    closed_date = <Date 2016-05-25.18:40:42.251>
    closer = 'zach.ware'
    components = ['Windows']
    creation = <Date 2014-12-10.20:13:53.879>
    creator = 'markgrandi'
    dependencies = []
    files = ['37411', '37825', '42954']
    hgrepos = []
    issue_num = 23026
    keywords = ['patch']
    message_count = 24.0
    messages = ['232442', '234526', '266137', '266163', '266168', '266169', '266171', '266174', '266175', '266239', '266241', '266242', '266243', '266247', '266252', '266253', '266287', '266288', '266317', '266383', '266385', '266386', '266387', '268023']
    nosy_count = 8.0
    nosy_names = ['paul.moore', 'tim.golden', 'python-dev', 'zach.ware', 'markgrandi', 'eryksun', 'steve.dower', 'hakril']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue23026'
    versions = ['Python 3.6']

    @mgrandi
    Copy link
    Mannequin Author

    mgrandi mannequin commented Dec 10, 2014

    The winreg module has constants for the different Windows registry types: https://docs.python.org/3/library/winreg.html?highlight=winreg#value-types

    It also links to the Microsoft documentation on the registry types, and I noticed that python doesn't have constants for 2 of the types (really just 1), REG_QWORD and REG_QWORD_LITTLE_ENDIAN (same as REG_QWORD).

    So I added the constants in winreg.c, which I think is right, as its the same format as the other ones, and I edited the documentation inside of winreg.c, which is the same changes i made to the docs in winreg.rst, except it doesn't have the markdown/rst/whatever formatting markers. I also made notes on REG_DWORD_LITTLE_ENDIAN that it is equivalent to REG_DWORD.

    I also tested to make sure that the documentation builds fine, but I haven't tested building python with the winreg.c changes, as I don't have access to a windows computer that can compile it at the moment.

    Some concerns:

    1: I have no idea if REG_QWORD is defined on 32 bit windows, I would assume it would be, but I can't check at the moment.

    2: There should probably be some switch statement entries in the Reg2Py() and Py2Reg() functions that deal with QWORDs, and dealing with if the python build is 32 bit vs 64 bit. I'm not comfortable with the python C internals to do that myself.

    My initial patch is attached, with the documentation + changes to winreg.c that just add the constants to the module

    @mgrandi mgrandi mannequin added the OS-windows label Dec 10, 2014
    @mgrandi
    Copy link
    Mannequin Author

    mgrandi mannequin commented Jan 22, 2015

    I Attempted to modify Reg2Py and Py2Reg to handle the QWORDs, this is assuming that the only thing that needs to change is just using PyLong_AsUnsignedLongLong()/FromUnsignedLongLong() instead of just UnsignedLong()

    the patch has the same changes as the 'part1' patch + the Py2Reg and Reg2Py changes

    @httpsgithubcomhakril
    Copy link
    Mannequin

    httpsgithubcomhakril mannequin commented May 23, 2016

    I also stumbled on this issue when playing with the winreg module.
    I will try to make the discussion go forward by submitting a new patch.
    I added a test and tried everything (test included on 3.5) on my own computer (windows 8.1 64b)
    There are two points that might be 'sensitive' and I would like to have someone's opinion:
    * I am using DWORD64 as an unsigned 64bit value as QWORD is not defined in <windows.h>
    * I changed one line of the previous implementation for REG_DWORD:
    * PyLong_FromUnsignedLong(*(int *)retDataBuf); became PyLong_FromUnsignedLong(*(DWORD *)retDataBuf);
    * I don't think it is a problem, but better ask than break something

    @zooba
    Copy link
    Member

    zooba commented May 23, 2016

    Looks good to me. (In case someone else applies the patch, there are a few mixed tab/spaces that should be fixed.)

    I'm personally okay with adding this to 3.5, but technically I think it's a new feature and should be in 3.6 only. Anyone else have an opinion here?

    @pfmoore
    Copy link
    Member

    pfmoore commented May 23, 2016

    My instinct is that this should only go into 3.6. It's a remote possibility, but someone could otherwise write code that uses REG_DWORD64 and say that it "works on 3.5" because they tested on 3.5.x, only to have it fail on 3.5.1.

    But if someone with a better feel for the implications wants to say it's OK, I wouldn't object.

    @mgrandi
    Copy link
    Mannequin Author

    mgrandi mannequin commented May 23, 2016

    why do you say that QWORD is not present in windows.h?

    I found it here: https://msdn.microsoft.com/en-us/library/windows/desktop/aa383751%28v=vs.85%29.aspx

    typedef unsigned __int64 QWORD;

    @httpsgithubcomhakril
    Copy link
    Mannequin

    httpsgithubcomhakril mannequin commented May 23, 2016

    Because QWORD is not defined in any file included by CPython. The main file for basic sized type definition in Windows is:

    basetsd.h: Type definitions for the basic sized types.
    

    which contains the definition of DWORD64: typedef unsigned __int64 DWORD64, *PDWORD64;
    but no reference to QWORD. Furthemore the code did not compile with the use of QWORD.
    So, I am pretty sure it is not defined by the standard includes

    Other people complain in your link:

    > "Apparently, QWORD isn't defined in any of the windows header files for MSVC 2010."
    

    Finally, the only files where I could find a typedef for QWORD are:

    * Include\shared\Sspi.h (Security Support Provider Interface. Prototypes and structure definitions)
    * Include\um\WinDNS.h (Domain Name System (DNS). DNS definitions and DNS API.)
    * Include\um\wmcodecdsp.h (COM interface -> generated code)
    * Include\um\wmnetsourcecreator.h (COM interface -> generated code)
    * Include\um\wmnetsourcecreator.idl (MIDL Interface Definition File)
    * Include\um\wmsdkidl.h (COM interface -> generated code)
    * Include\um\wmsdkidl.idl (MIDL Interface Definition File)
    * Include\um\mfobjects.h (COM interface -> generated code)
    * Include\um\Mfobjects.idl (MIDL Interface Definition File)
    

    It seems to be in pretty obscure files that should not be included by CPython (In my opinion)

    @zooba
    Copy link
    Member

    zooba commented May 23, 2016

    IIRC, the preferred typedef is ULONGLONG, but it's not really a big deal. Avoiding new header files is a good enough reason.

    I think Paul's right about 3.6 only. We shouldn't encourage code that won't work downlevel within the same version, and there is a ctypes workaround available.

    @mgrandi
    Copy link
    Mannequin Author

    mgrandi mannequin commented May 23, 2016

    @hakril: ok, that makes sense, I'm not a windows/win32 programmer by any means so I was mostly going on the microsoft web docs

    also the patch by hakril includes the documentation from my patch, so that patch should be the one considered (as a FYI)

    @httpsgithubcomhakril
    Copy link
    Mannequin

    httpsgithubcomhakril mannequin commented May 24, 2016

    I would like to discuss the fact that this patch is a new feature.

    In the current state, CPython handles the REG_QWORD type as an unkown type and returns the raw registry value: a string of size 8.
    In my opinion this behavior does not match the Windows documentation (REG_QWORD: A 64-bit number)
    or CPython's own behavior regarding REG_DWORD that returns a Python int (more important).

    Is this enough to consider this a bugfix ? and therefore port it to 2.7 ?

    @pfmoore
    Copy link
    Member

    pfmoore commented May 24, 2016

    I'm not sure I follow your comment. In Python 3.5,

    >>> from winreg import REG_QWORD
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    ImportError: cannot import name 'REG_QWORD'

    So exposing REG_QWORD from the winreg module would be a change in behaviour, not a bug fix.

    @httpsgithubcomhakril
    Copy link
    Mannequin

    httpsgithubcomhakril mannequin commented May 24, 2016

    My comment was about the behavior of winreg.QueryValueEx. The current behavior is:

    >>> import winreg
    >>> tstkey = winreg.OpenKey(winreg.HKEY_CURRENT_USER, "TestKey")
    >>> winreg.QueryValueEx(tstkey, "MYDWORD") # a REG_DWORD value
    (16909060, 4)
    >>> winreg.QueryValueEx(tstkey, "MYQWORD") # a REG_QWORD value
    (b'\x08\x07\x06\x05\x04\x03\x02\x01', 11)

    With this patch the new behavior would be:

    >>> winreg.QueryValueEx(tstkey, "MYQWORD") # a REG_QWORD value
    (72623859790382856, 11)

    I agree that exposing REG_QWORD is a new feature.

    @pfmoore
    Copy link
    Member

    pfmoore commented May 24, 2016

    Hmm, OK. So code that currently needs to access QWORD values and decodes the bytes returned would be broken by this change. Equally it is possible (albeit not ideal) to get the data out with the current behaviour.

    I can't judge how likely it is that anyone has written code like this - personally, I'd prefer not to take the risk. But if someone else feels I'm being over-cautious, that's fine.

    @zooba
    Copy link
    Member

    zooba commented May 24, 2016

    Changing a return value type without notice certainly can't go into a maintenance release. I'd assumed an extra import too, but since it's transparent this is 3.6 only (definitely not a security fix, so 2.7 isn't up for discussion).

    @httpsgithubcomhakril
    Copy link
    Mannequin

    httpsgithubcomhakril mannequin commented May 24, 2016

    Stated as you did, it makes sens. Should the change of return type be acknowledged somewhere in the documentation ?

    @zware
    Copy link
    Member

    zware commented May 24, 2016

    Clement Rouault added the comment:

    Stated as you did, it makes sens. Should the change of return type be acknowledged somewhere in the documentation ?

    Yes, it should be mentioned in Doc/whatsnew/3.6.rst and probably also
    somewhere in Doc/library/winreg.rst

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 24, 2016

    New changeset ac2a2534f793 by Steve Dower in branch 'default':
    Issue bpo-23026: winreg.QueryValueEx() now return an integer for REG_QWORD type. (Patch by hakril)
    https://hg.python.org/cpython/rev/ac2a2534f793

    @zooba
    Copy link
    Member

    zooba commented May 24, 2016

    I'd appreciate at least one other person looking over my commit before I close this, just to confirm that the notes I added to the docs are fine.

    @eryksun
    Copy link
    Contributor

    eryksun commented May 25, 2016

    In Py2Reg, (*retDataBuf==NULL) needs spaces around the == operator.

    Should the changes to the winreg docs be noted as new in 3.6?

    Bikeshedding: in "What's New" you mention the change to QueryValueEx, but Reg2Py and Py2Reg affect EnumValue, QueryValueEx, and SetValueEx. Maybe change it to a more general description, like this:

    winreg
    \------
    
    Added the 64-bit integer type :data:`REG_QWORD <winreg.REG_QWORD>`.
    (Contributed by Clement Rouault in :issue:`23026`.)
    

    @mgrandi
    Copy link
    Mannequin Author

    mgrandi mannequin commented May 25, 2016

    in 'test_winreg.py', there should probably be a test that covers REG_QWORD_LITTLE_ENDIAN, even if they are essentially equivalent

    Also, in Py2Reg and Reg2Py, it seems that both are missing the 'case' clause for REG_QWORD_LITTLE_ENDIAN as well

    case REG_QWORD:
    case REG_QWORD_LITTLE_ENDIAN:
    <code here>

    And sure, i would have the documentation changes listed in 'Doc/whatsnew/3.6.rst'. And now that I think about it, the documentation in '/Doc/library/winreg.rst' should probably mention that REG_QWORD and REG_QWORD_LITTLE_ENDIAN are new as of python 3.6

    @zooba
    Copy link
    Member

    zooba commented May 25, 2016

    Thanks!

    I don't think we need to mention doc changes in whatsnew, but I've made the description more general and added the "New in version 3.6" notes to the winreg page.

    The formatting was presumably copied from the surrounding code, but I've fixed all instances of it.

    I can't actually add ``case REG_QWORD_LITTLE_ENDIAN:`` as it has the same value as REG_QWORD and won't compile.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 25, 2016

    New changeset ed4eec682199 by Steve Dower in branch 'default':
    Closes bpo-23026: Documentation improvements and code formatting
    https://hg.python.org/cpython/rev/ed4eec682199

    @python-dev python-dev mannequin closed this as completed May 25, 2016
    @mgrandi
    Copy link
    Mannequin Author

    mgrandi mannequin commented May 25, 2016

    Oh, I didn't realize that both REG_QWORD and REG_QWORD_LITTLE_ENDIAN had the same value, that works then.

    @mgrandi mgrandi mannequin reopened this May 25, 2016
    @zware zware closed this as completed May 25, 2016
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 9, 2016

    New changeset 5306f27c53aa by Serhiy Storchaka in branch 'default':
    Regenerate Argument Clinic code for issue bpo-23026.
    https://hg.python.org/cpython/rev/5306f27c53aa

    @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
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants