[PATCH 1 of 5] port win32.py to using the Python ctypes library
Adrian Buehlmann
adrian at cadifra.com
Sun Feb 13 07:07:46 CST 2011
On 2011-02-13 12:39, Patrick Mézard wrote:
> Le 08/02/11 17:52, Adrian Buehlmann a écrit :
>> # HG changeset patch
>> # User Adrian Buehlmann <adrian at cadifra.com>
>> # Date 1297121623 -3600
>> # Node ID 3ba4920422e9abcdc8b8634c5c12c68051286a8c
>> # Parent 69e69b131458023d21ec40aa48fc5299e43ce69b
>> port win32.py to using the Python ctypes library
>>
>> The pywin32 package is no longer needed.
>>
>> ctypes is now required for running Mercurial on Windows.
>>
>> ctypes is included in Python since version 2.5. For Python 2.4, ctypes is
>> available as an extra installer package for Windows.
>>
>> Also moved spawndetached() from windows.py to win32.py and fixed it, using
>> ctypes as well. spawndetached was defunct with Python 2.6.6 because Python
>> removed their undocumented subprocess.CreateProcess. This fixes
>> 'hg serve -d' on Windows.
>
> [...]
>
>> diff --git a/mercurial/win32.py b/mercurial/win32.py
>> --- a/mercurial/win32.py
>> +++ b/mercurial/win32.py
>> @@ -5,73 +5,174 @@
>> # This software may be used and distributed according to the terms of the
>> # GNU General Public License version 2 or any later version.
>>
>> -"""Utility functions that use win32 API.
>> +import osutil, encoding
>> +import ctypes, errno, os, struct, subprocess
>>
>> -Mark Hammond's win32all package allows better functionality on
>> -Windows. This module overrides definitions in util.py. If not
>> -available, import of this module will fail, and generic code will be
>> -used.
>> -"""
>> +_kernel32 = ctypes.windll.kernel32
>>
>> -import win32api
>> +_BOOL = ctypes.c_long
>> +_WORD = ctypes.c_ushort
>> +_DWORD = ctypes.c_ulong
>> +_LPCSTR = _LPSTR = ctypes.c_char_p
>> +_HANDLE = ctypes.c_void_p
>> +_HWND = _HANDLE
>
> [ ... skipping types definitions ...]
>
>> +def _raiseoserror(name):
>> + err = ctypes.WinError()
>> + raise OSError(err.errno, '%s: %s' % (name, err.strerror))
>> +
>> +def _getfileinfo(name):
>> + fh = _kernel32.CreateFileA(name,
>> + 0, _FILE_SHARE_READ | _FILE_SHARE_WRITE | _FILE_SHARE_DELETE,
>> + None, _OPEN_EXISTING, 0, None)
>
> (You changed the sharing mode here. Which is fine but could have gone in a separate patch imho.)
Changing the same code location more than once in the same series seemed like overkill to me, but well. Sorry for sneaking in a bugfix.
>> + if fh == _INVALID_HANDLE_VALUE:
>> + _raiseoserror(name)
>> + try:
>> + fi = _BY_HANDLE_FILE_INFORMATION()
>> + if not _kernel32.GetFileInformationByHandle(fh, ctypes.byref(fi)):
>> + _raiseoserror(name)
>> + return fi
>> + finally:
>> + if fh != _INVALID_HANDLE_VALUE:
>
> Isn't this always True?
>
>> + _kernel32.CloseHandle(fh)
>>
>> def os_link(src, dst):
>> - try:
>> - win32file.CreateHardLink(dst, src)
>> - except pywintypes.error:
>> - raise OSError(errno.EINVAL, 'target implements hardlinks improperly')
>> - except NotImplementedError: # Another fake error win Win98
>> - raise OSError(errno.EINVAL, 'Hardlinking not supported')
>> + if not _kernel32.CreateHardLinkA(dst, src, None):
>> + _raiseoserror(src)
>
> [...]
>
>> def lookup_reg(key, valname=None, scope=None):
>> ''' Look up a key/value name in the Windows registry.
>> @@ -82,101 +183,168 @@ def lookup_reg(key, valname=None, scope=
>> a sequence of scopes to look up in order. Default (CURRENT_USER,
>> LOCAL_MACHINE).
>> '''
>> - try:
>> - from _winreg import HKEY_CURRENT_USER, HKEY_LOCAL_MACHINE, \
>> - QueryValueEx, OpenKey
>> - except ImportError:
>> - return None
>> -
>> + adv = ctypes.windll.advapi32
>> + byref = ctypes.byref
>> if scope is None:
>> - scope = (HKEY_CURRENT_USER, HKEY_LOCAL_MACHINE)
>> + scope = (_HKEY_CURRENT_USER, _HKEY_LOCAL_MACHINE)
>> elif not isinstance(scope, (list, tuple)):
>> scope = (scope,)
>> for s in scope:
>> + kh = _HANDLE()
>> + res = adv.RegOpenKeyExA(s, key, 0, _KEY_READ, ctypes.byref(kh))
>> + if res != _ERROR_SUCCESS:
>> + continue
>> try:
>> - val = QueryValueEx(OpenKey(s, key), valname)[0]
>> - # never let a Unicode string escape into the wild
>> - return encoding.tolocal(val.encode('UTF-8'))
>> - except EnvironmentError:
>> - pass
>> + size = _DWORD(600)
>> + type = _DWORD()
>> + buf = ctypes.create_string_buffer(size.value + 1)
>> + res = adv.RegGetValueA(kh.value, None, valname, _RRF_RT_ANY,
>> + byref(type), byref(buf), byref(size))
>> + if res != _ERROR_SUCCESS:
>> + continue
>> + if type.value == _REG_SZ:
>> + # never let a Unicode string escape into the wild
>> + return encoding.tolocal(buf.value.encode('UTF-8'))
>> + elif type.value == _REG_DWORD:
>> + fmt = '<L'
>> + s = ctypes.string_at(byref(buf), struct.calcsize(fmt))
>> + return struct.unpack(fmt, s)[0]
>> + finally:
>> + if kh.value:
>
> Do you think RegOpenKeyExA can succeed while returning a NULL key HANDLE?
The 'if' check is unneeded.
Calling RegCloseKey with a NULL handle is harmless anyway:
Python 2.6.6 (r266:84297, Aug 24 2010, 18:13:38) [MSC v.1500 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import ctypes
>>> ctypes.windll.advapi32.RegCloseKey(0)
6
(6 is ERROR_INVALID_HANDLE)
>> + adv.RegCloseKey(kh.value)
>>
>> def system_rcpath_win32():
>> '''return default os-specific hgrc search path'''
>> - filename = win32api.GetModuleFileName(0)
>> + rcpath = []
>> + size = 600
>> + buf = ctypes.create_string_buffer(size + 1)
>> + len = _kernel32.GetModuleFileNameA(None, ctypes.byref(buf), size)
>> + if not len:
>
> This should check the case where buf is too small, testing if len == size should be good enough.
>
> http://msdn.microsoft.com/en-us/library/ms683197(VS.85).aspx
Sounds a bit difficult to me to put an exe in a path location with a path length
>600 on a file system if the OS' maximum path length is 260.
Can an exe be started from a path longer than 260?
After all, the buffer has a size of 600...
But adding a check for truncation is certainly an improvement and won't harm.
FWIW, GetModuleFileNameA will never write more than size chars, so it's not like
we are talking about a potential buffer overrun case here (truncation at worst).
(although it's quite beyond comprehension why Microsoft returns success on
GetModuleFileNameA if the target buffer is too small)
>> + return rcpath
>> + filename = buf.value
>> # Use mercurial.ini found in directory with hg.exe
>> progrc = os.path.join(os.path.dirname(filename), 'mercurial.ini')
>> if os.path.isfile(progrc):
>> - return [progrc]
>> + rcpath.append(progrc)
>> + return rcpath
>> # Use hgrc.d found in directory with hg.exe
>> progrcd = os.path.join(os.path.dirname(filename), 'hgrc.d')
>> if os.path.isdir(progrcd):
>> - rcpath = []
>> for f, kind in osutil.listdir(progrcd):
>> if f.endswith('.rc'):
>> rcpath.append(os.path.join(progrcd, f))
>> return rcpath
>> # else look for a system rcpath in the registry
>> - try:
>> - value = win32api.RegQueryValue(
>> - win32con.HKEY_LOCAL_MACHINE, 'SOFTWARE\\Mercurial')
>> - rcpath = []
>> - for p in value.split(os.pathsep):
>> - if p.lower().endswith('mercurial.ini'):
>> - rcpath.append(p)
>> - elif os.path.isdir(p):
>> - for f, kind in osutil.listdir(p):
>> - if f.endswith('.rc'):
>> - rcpath.append(os.path.join(p, f))
>> + value = lookup_reg('SOFTWARE\\Mercurial', None, _HKEY_LOCAL_MACHINE)
>> + if not isinstance(value, str) or not value:
>> return rcpath
>> - except pywintypes.error:
>> - return []
>> + value = value.replace('/', os.sep)
>> + for p in value.split(os.pathsep):
>> + if p.lower().endswith('mercurial.ini'):
>> + rcpath.append(p)
>> + elif os.path.isdir(p):
>> + for f, kind in osutil.listdir(p):
>> + if f.endswith('.rc'):
>> + rcpath.append(os.path.join(p, f))
>> + return rcpath
>>
>> def user_rcpath_win32():
>> '''return os-specific hgrc search path to the user dir'''
>> userdir = os.path.expanduser('~')
>> - if sys.getwindowsversion()[3] != 2 and userdir == '~':
>> - # We are on win < nt: fetch the APPDATA directory location and use
>> - # the parent directory as the user home dir.
>> - appdir = shell.SHGetPathFromIDList(
>> - shell.SHGetSpecialFolderLocation(0, shellcon.CSIDL_APPDATA))
>> - userdir = os.path.dirname(appdir)
>> return [os.path.join(userdir, 'mercurial.ini'),
>> os.path.join(userdir, '.hgrc')]
>>
>> def getuser():
>> '''return name of current user'''
>> - return win32api.GetUserName()
>> + adv = ctypes.windll.advapi32
>> + size = _DWORD(300)
>> + buf = ctypes.create_string_buffer(size.value + 1)
>> + if not adv.GetUserNameA(ctypes.byref(buf), ctypes.byref(size)):
>> + raise ctypes.WinError()
>> + return buf.value
>> +
>> +_SIGNAL_HANDLER = ctypes.WINFUNCTYPE(_BOOL, _DWORD)
>> +_signal_handler = []
>>
>> def set_signal_handler_win32():
>> - """Register a termination handler for console events including
>> + '''Register a termination handler for console events including
>> CTRL+C. python signal handlers do not work well with socket
>> operations.
>> - """
>> + '''
>> def handler(event):
>> - win32process.ExitProcess(1)
>> - win32api.SetConsoleCtrlHandler(handler)
>> + _kernel32.ExitProcess(1)
>> +
>> + if _signal_handler:
>> + return # already registered
>
> (This behaviour change could have gone in another patch too, unless the ctypes version does behave differently than the pywin32 one.)
What behavior change?
>> + h = _SIGNAL_HANDLER(handler)
>> + _signal_handler.append(h) # needed to prevent garbage collection
>> + if not _kernel32.SetConsoleCtrlHandler(h, True):
>> + raise ctypes.WinError()
>
> [...]
>
>> diff --git a/mercurial/windows.py b/mercurial/windows.py
>> --- a/mercurial/windows.py
>> +++ b/mercurial/windows.py
>> @@ -71,7 +71,7 @@ def _is_win_9x():
>> return 'command' in os.environ.get('comspec', '')
>>
>> def openhardlinks():
>> - return not _is_win_9x() and "win32api" in globals()
>> + return not _is_win_9x()
>>
>> def system_rcpath():
>> try:
>> @@ -106,10 +106,6 @@ def sshargs(sshcmd, host, user, port):
>> args = user and ("%s@%s" % (user, host)) or host
>> return port and ("%s %s %s" % (args, pflag, port)) or args
>>
>> -def testpid(pid):
>> - '''return False if pid dead, True if running or not known'''
>> - return True
>> -
>> def set_flags(f, l, x):
>> pass
>>
>> @@ -211,7 +207,7 @@ def find_exe(command):
>> def set_signal_handler():
>> try:
>> set_signal_handler_win32()
>> - except NameError:
>> + except WindowsError:
>> pass
>
> Silencing the lack of set_signal_handler_win32() when pywin32 was missing was a bit rough but we all know that hg without pywin32 was not fully functional. But why do you want to silence errors from set_signal_handler_win32()?
I'm fine with removing the try..except.
> [...]
>
> Except for the GetModuleFileNameA error handling and my last remark, everything else look fine.
More information about the Mercurial-devel
mailing list