Re: [Qemu-devel] [PATCH] Added cleanup for Win32 TAP interface

[Stefan Hajnoczi]

On Wed, Mar 13, 2013 at 04:23:52PM +0400, Pavel Dovgaluk wrote:
> Added cleanup for Win32 TAP interface.
> 
> Signed-off-by: Pavel Dovgalyuk<address@hidden>
> ---
>  net/tap-win32.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/net/tap-win32.c b/net/tap-win32.c
> index 91e9e84..1c1176c 100644
> --- a/net/tap-win32.c
> +++ b/net/tap-win32.c
> @@ -99,6 +99,7 @@ typedef struct tap_win32_overlapped {
>      HANDLE output_queue_semaphore;
>      HANDLE free_list_semaphore;
>      HANDLE tap_semaphore;
> +    HANDLE hThread;

Please follow QEMU coding style: "thread" or "hthread" or "h_thread" but
not "hThread".

>      CRITICAL_SECTION output_queue_cs;
>      CRITICAL_SECTION free_list_cs;
>      OVERLAPPED read_overlapped;
> @@ -625,7 +626,7 @@ static int tap_win32_open(tap_win32_overlapped_t 
> **phandle,
>  
>      *phandle = &tap_overlapped;
>  
> -    CreateThread(NULL, 0, tap_win32_thread_entry,
> +    tap_overlapped.hThread = CreateThread(NULL, 0, tap_win32_thread_entry,
>                   (LPVOID)&tap_overlapped, 0, &idThread);
>      return 0;
>  }
> @@ -643,9 +644,8 @@ static void tap_cleanup(NetClientState *nc)
>  
>      qemu_del_wait_object(s->handle->tap_semaphore, NULL, NULL);
>  
> -    /* FIXME: need to kill thread and close file handle:
> -       tap_win32_close(s);
> -    */
> +    TerminateThread(s->handle->hThread, 0);
> +    CloseHandle(s->handle->handle);

http://msdn.microsoft.com/en-us/library/windows/desktop/ms686717(v=vs.85).aspx

"Windows Server 2003 and Windows XP:  The target thread's initial stack
is not freed, causing a resource leak."

"TerminateThread is a dangerous function that should only be used in the
most extreme cases. You should call TerminateThread only if you know
exactly what the target thread is doing, and you control all of the code
that the target thread could possibly be running at the time of the
termination."

Please terminate tap_win32_thread_entry() cleanly instead of using
TerminateThread().  Typically this means signalling that the thread
should stop and then waiting for the thread to exit by itself.

Stefan