[PATCH 1 of 2] run-tests: implement proper process killing for Windows

[Pierre-Yves David]

On Mon, Jun 18, 2012 at 12:08:48PM +0200, Adrian Buehlmann wrote:
> On 2012-06-18 11:19, Pierre-Yves David wrote:
> > On Mon, Jun 18, 2012 at 12:35:17AM +0200, Adrian Buehlmann wrote:
> >> +if os.name == 'nt':
> >> +    import ctypes
> >> +    def killprocess(pid):
> >> +        vlog('# Killing daemon process %d' % pid)
> >> +        PROCESS_TERMINATE = 1
> >> +        k = ctypes.windll.kernel32
> >> +        handle = k.OpenProcess(PROCESS_TERMINATE, False, pid)
> > 
> > You this is the single use of PROCESS_TERMINATE. Can't you just use 1 here ?
> 
> We could, but I think that would make the code less readable.
> 
> PROCESS_TERMINATE is a name for a constant used in the Windows API
> 
> http://msdn.microsoft.com/en-us/library/windows/desktop/ms686714(v=vs.85).aspx
> 
> It's a process access right on Windows. In C++ code, we would use a
> define from a Windows header file. With Python's ctypes foreign function
> library, we usually define such things pretty close to the place where
> we need them. See mercurial/win32.py for comparison.

I suspected that but I was unsure.

> Using that name helps to search for such constants, also when
> google-ing. I think it's better than a magic number in the code.

+1

> Speedwise, it doesn't matter that much, it's testbed code. Though I
> could certainly move the definition out of killprocess().

We don't care at all about the performance impact of this here.

> Perhaps this would be a bit more pythonic:
> 
> if os.name == 'nt':
>     import ctypes
> 
>     _PROCESS_TERMINATE = 1
> 
>     def killprocess(pid):
>         vlog('# Killing daemon process %d' % pid)
>         k = ctypes.windll.kernel32
>         handle = k.OpenProcess(_PROCESS_TERMINATE, False, pid)
>         k.TerminateProcess(handle, -1)
>         k.CloseHandle(handle)

I would drop the frontal "_" and add a comment 

# Windows API symbolic constant definition


-- 
Pierre-Yves David

http://www.logilab.fr/

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20120618/913891ab/attachment.pgp>