msg232442 - (view) |
Author: Mark Grandi (markgrandi) * |
Date: 2014-12-10 20:13 |
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
|
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) *  |
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) *  |
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: https://msdn.microsoft.com/en-us/library/windows/desktop/aa383751%28v=vs.85%29.aspx
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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)
https://hg.python.org/cpython/rev/ac2a2534f793
|
msg266288 - (view) |
Author: Steve Dower (steve.dower) *  |
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:
winreg
------
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 '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
|
msg266385 - (view) |
Author: Steve Dower (steve.dower) *  |
Date: 2016-05-25 18:25 |
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.
|
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
https://hg.python.org/cpython/rev/ed4eec682199
|
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.
https://hg.python.org/cpython/rev/5306f27c53aa
|
|
Date |
User |
Action |
Args |
2022-04-11 14:58:11 | admin | set | github: 67215 |
2016-06-09 13:17:24 | python-dev | set | messages:
+ msg268023 |
2016-05-25 18:40:42 | zach.ware | set | status: open -> closed resolution: fixed |
2016-05-25 18:29:58 | markgrandi | set | status: closed -> open resolution: fixed -> (no value) messages:
+ msg266387
|
2016-05-25 18:26:29 | python-dev | set | status: open -> closed resolution: fixed messages:
+ msg266386
stage: commit review -> resolved |
2016-05-25 18:25:38 | steve.dower | set | messages:
+ msg266385 |
2016-05-25 18:12:51 | markgrandi | set | messages:
+ msg266383 |
2016-05-25 07:40:51 | eryksun | set | nosy:
+ eryksun messages:
+ msg266317
|
2016-05-24 22:44:11 | steve.dower | set | messages:
+ msg266288 stage: commit review |
2016-05-24 22:43:27 | python-dev | set | nosy:
+ python-dev messages:
+ msg266287
|
2016-05-24 15:03:00 | zach.ware | set | messages:
+ msg266253 |
2016-05-24 14:52:07 | hakril | set | messages:
+ msg266252 |
2016-05-24 13:18:41 | steve.dower | set | messages:
+ msg266247 versions:
- Python 3.5 |
2016-05-24 11:17:13 | paul.moore | set | messages:
+ msg266243 |
2016-05-24 10:50:04 | hakril | set | messages:
+ msg266242 |
2016-05-24 10:01:09 | paul.moore | set | messages:
+ msg266241 |
2016-05-24 09:01:02 | hakril | set | messages:
+ msg266239 |
2016-05-23 19:12:21 | markgrandi | set | messages:
+ msg266175 versions:
+ Python 3.5 |
2016-05-23 19:02:23 | steve.dower | set | messages:
+ msg266174 versions:
- Python 3.5 |
2016-05-23 18:30:56 | hakril | set | messages:
+ msg266171 |
2016-05-23 17:39:07 | markgrandi | set | messages:
+ msg266169 |
2016-05-23 17:31:21 | paul.moore | set | messages:
+ msg266168 |
2016-05-23 16:36:17 | steve.dower | set | nosy:
+ paul.moore
messages:
+ msg266163 versions:
+ Python 3.6 |
2016-05-23 11:15:45 | hakril | set | files:
+ winreg_qword_patch_with_test.patch
messages:
+ msg266137 versions:
+ Python 3.5, - Python 3.4 |
2016-05-23 10:53:39 | hakril | set | nosy:
+ hakril
|
2015-01-22 23:24:09 | markgrandi | set | files:
+ winreg_add_qword_constants_part2_markgrandi.patch
messages:
+ msg234526 |
2014-12-10 20:13:53 | markgrandi | create | |