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

[mgrandi]

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]

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]

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]

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]

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]

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]

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]

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]

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]

@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]

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]

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]

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]

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]

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]

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

[zware]

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]

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]

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]

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]

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]

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]

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

[mgrandi]

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

[python-dev]

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