Title: Winreg module doesn't support REG_QWORD, small DWORD doc update
Type: Stage: resolved
Components: Windows Versions: Python 3.6
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: eryksun, hakril, markgrandi, paul.moore, python-dev, steve.dower, tim.golden, zach.ware
Priority: normal Keywords: patch

Created on 2014-12-10 20:13 by markgrandi, last changed 2016-06-09 13:17 by python-dev. This issue is now closed.

File name Uploaded Description Edit
winreg_add_qword_constants_part1_markgrandi.patch markgrandi, 2014-12-10 20:13 review
winreg_add_qword_constants_part2_markgrandi.patch markgrandi, 2015-01-22 23:24 updated patch, doc changes + code changes review
winreg_qword_patch_with_test.patch hakril, 2016-05-23 11:15
Messages (24)
msg232442 - (view) Author: Mark Grandi (markgrandi) * Date: 2014-12-10 20:13
The winreg module has constants for the different Windows registry 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
msg234526 - (view) Author: Mark Grandi (markgrandi) * Date: 2015-01-22 23:24
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
msg266137 - (view) Author: Clement Rouault (hakril) * Date: 2016-05-23 11:15
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
msg266163 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2016-05-23 16:36
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?
msg266168 - (view) Author: Paul Moore (paul.moore) * (Python committer) Date: 2016-05-23 17:31
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.
msg266169 - (view) Author: Mark Grandi (markgrandi) * Date: 2016-05-23 17:39
why do you say that QWORD is not present in windows.h? 

I found it here:

typedef unsigned __int64 QWORD;
msg266171 - (view) Author: Clement Rouault (hakril) * Date: 2016-05-23 18:30
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)
msg266174 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2016-05-23 19:02
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.
msg266175 - (view) Author: Mark Grandi (markgrandi) * Date: 2016-05-23 19:12
@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)
msg266239 - (view) Author: Clement Rouault (hakril) * Date: 2016-05-24 09:01
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 ?
msg266241 - (view) Author: Paul Moore (paul.moore) * (Python committer) Date: 2016-05-24 10:01
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.
msg266242 - (view) Author: Clement Rouault (hakril) * Date: 2016-05-24 10:50
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.
msg266243 - (view) Author: Paul Moore (paul.moore) * (Python committer) Date: 2016-05-24 11:17
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.
msg266247 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2016-05-24 13:18
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).
msg266252 - (view) Author: Clement Rouault (hakril) * Date: 2016-05-24 14:52
Stated as you did, it makes sens. Should the change of return type be acknowledged somewhere in the documentation ?
msg266253 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2016-05-24 15:03
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
msg266287 - (view) Author: Roundup Robot (python-dev) Date: 2016-05-24 22:43
New changeset ac2a2534f793 by Steve Dower in branch 'default':
Issue #23026: winreg.QueryValueEx() now return an integer for REG_QWORD type. (Patch by hakril)
msg266288 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2016-05-24 22:44
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.
msg266317 - (view) Author: Eryk Sun (eryksun) * Date: 2016-05-25 07:40
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:


    Added the 64-bit integer type :data:`REG_QWORD <winreg.REG_QWORD>`.
    (Contributed by Clement Rouault in :issue:`23026`.)
msg266383 - (view) Author: Mark Grandi (markgrandi) * Date: 2016-05-25 18:12
in '', 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

<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
msg266385 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2016-05-25 18:25

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.
msg266386 - (view) Author: Roundup Robot (python-dev) Date: 2016-05-25 18:26
New changeset ed4eec682199 by Steve Dower in branch 'default':
Closes #23026: Documentation improvements and code formatting
msg266387 - (view) Author: Mark Grandi (markgrandi) * Date: 2016-05-25 18:29
Oh, I didn't realize that both REG_QWORD and REG_QWORD_LITTLE_ENDIAN had the same value, that works then.
msg268023 - (view) Author: Roundup Robot (python-dev) Date: 2016-06-09 13:17
New changeset 5306f27c53aa by Serhiy Storchaka in branch 'default':
Regenerate Argument Clinic code for issue #23026.
Date User Action Args
2016-06-09 13:17:24python-devsetmessages: + msg268023
2016-05-25 18:40:42zach.waresetstatus: open -> closed
resolution: fixed
2016-05-25 18:29:58markgrandisetstatus: closed -> open
resolution: fixed -> (no value)
messages: + msg266387
2016-05-25 18:26:29python-devsetstatus: open -> closed
resolution: fixed
messages: + msg266386

stage: commit review -> resolved
2016-05-25 18:25:38steve.dowersetmessages: + msg266385
2016-05-25 18:12:51markgrandisetmessages: + msg266383
2016-05-25 07:40:51eryksunsetnosy: + eryksun
messages: + msg266317
2016-05-24 22:44:11steve.dowersetmessages: + msg266288
stage: commit review
2016-05-24 22:43:27python-devsetnosy: + python-dev
messages: + msg266287
2016-05-24 15:03:00zach.waresetmessages: + msg266253
2016-05-24 14:52:07hakrilsetmessages: + msg266252
2016-05-24 13:18:41steve.dowersetmessages: + msg266247
versions: - Python 3.5
2016-05-24 11:17:13paul.mooresetmessages: + msg266243
2016-05-24 10:50:04hakrilsetmessages: + msg266242
2016-05-24 10:01:09paul.mooresetmessages: + msg266241
2016-05-24 09:01:02hakrilsetmessages: + msg266239
2016-05-23 19:12:21markgrandisetmessages: + msg266175
versions: + Python 3.5
2016-05-23 19:02:23steve.dowersetmessages: + msg266174
versions: - Python 3.5
2016-05-23 18:30:56hakrilsetmessages: + msg266171
2016-05-23 17:39:07markgrandisetmessages: + msg266169
2016-05-23 17:31:21paul.mooresetmessages: + msg266168
2016-05-23 16:36:17steve.dowersetnosy: + paul.moore

messages: + msg266163
versions: + Python 3.6
2016-05-23 11:15:45hakrilsetfiles: + winreg_qword_patch_with_test.patch

messages: + msg266137
versions: + Python 3.5, - Python 3.4
2016-05-23 10:53:39hakrilsetnosy: + hakril
2015-01-22 23:24:09markgrandisetfiles: + winreg_add_qword_constants_part2_markgrandi.patch

messages: + msg234526
2014-12-10 20:13:53markgrandicreate