[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