Re: [Qemu-devel] [PATCH 2/2] tap-win32: disable broken async write path

[Stefan Weil]

Am 17.11.2015 um 20:09 schrieb Andrew Baumann:
> The code under the TUN_ASYNCHRONOUS_WRITES path makes two incorrect
> assumptions about the behaviour of the WriteFile API for overlapped
> file handles. First, WriteFile does not update the
> lpNumberOfBytesWritten parameter when the overlapped parameter is
> non-NULL (the number of bytes written is known only when the operation
> completes). Second, the buffer shouldn't be touched (or freed) until
> the operation completes. This led to at least one bug where
> tap_win32_write returned zero bytes written, which in turn caused
> further writes ("receives") to be disabled for that device.
>
> This change disables the asynchronous write path, while keeping most
> of the code around in case someone sees value in resurrecting it. It
> also adds some conditional debug output, similar to the read path.
>
> Signed-off-by: Andrew Baumann <address@hidden>
> ---
> Although this version of the patch tries to be minimally invasive and
> keep the code around, I'm not sure if there is much value in fixing
> this -- I haven't checked, but would expect that the write to a tap
> device is just a fairly fast buffer copy. If you would prefer a patch
> that simply removes the async write support entirely, that should be
> easy to do.
>
>  net/tap-win32.c | 48 +++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 37 insertions(+), 11 deletions(-)
>
> diff --git a/net/tap-win32.c b/net/tap-win32.c
> index 5e5d6db..02a3b0d 100644
> --- a/net/tap-win32.c
> +++ b/net/tap-win32.c
> @@ -77,7 +77,12 @@
>  
>  //#define DEBUG_TAP_WIN32
>  
> -#define TUN_ASYNCHRONOUS_WRITES 1
> +/* FIXME: The asynch write path appears to be broken at
> + * present. WriteFile() ignores the lpNumberOfBytesWritten parameter
> + * for overlapped writes, with the result we return zero bytes sent,
> + * and after handling a single packet, receive is disabled for this
> + * interface. */
> +/* #define TUN_ASYNCHRONOUS_WRITES 1 */
>  
>  #define TUN_BUFFER_SIZE 1560
>  #define TUN_MAX_BUFFER_COUNT 32
> @@ -461,26 +466,47 @@ static int tap_win32_write(tap_win32_overlapped_t 
> *overlapped,
>      BOOL result;
>      DWORD error;
>  
> +#ifdef TUN_ASYNCHRONOUS_WRITES
>      result = GetOverlappedResult( overlapped->handle, 
> &overlapped->write_overlapped,
>                                    &write_size, FALSE);
>  
>      if (!result && GetLastError() == ERROR_IO_INCOMPLETE)
>          WaitForSingleObject(overlapped->write_event, INFINITE);
> +#endif
>  
>      result = WriteFile(overlapped->handle, buffer, size,
> -                       &write_size, &overlapped->write_overlapped);
> +                       NULL, &overlapped->write_overlapped);
> +
> +#ifdef TUN_ASYNCHRONOUS_WRITES
> +    /* FIXME: we can't sensibly set write_size here, without waiting
> +     * for the IO to complete! Moreover, we can't return zero,
> +     * because that will disable receive on this interface, and we
> +     * also can't assume it will succeed and return the full size,
> +     * because that will result in the buffer being reclaimed while
> +     * the IO is in progress. */
> +#error Async writes are broken. Please disable TUN_ASYNCHRONOUS_WRITES.
> +#else /* !TUN_ASYNCHRONOUS_WRITES */
> +    if (!result) {
> +        error = GetLastError();
> +        if (error == ERROR_IO_PENDING) {
> +            result = GetOverlappedResult(overlapped->handle,
> +                                         &overlapped->write_overlapped,
> +                                         &write_size, TRUE);
> +        }
> +    }
> +#endif
>  
>      if (!result) {
> -        switch (error = GetLastError())
> -        {
> -        case ERROR_IO_PENDING:
> -#ifndef TUN_ASYNCHRONOUS_WRITES
> -            WaitForSingleObject(overlapped->write_event, INFINITE);
> +#ifdef DEBUG_TAP_WIN32
> +        LPVOID msgbuf;

Does this also work ...

    char *msgbuf;


> +        error = GetLastError();
> +        
> FormatMessage(FORMAT_MESSAGE_ALLOCATE_BUFFER|FORMAT_MESSAGE_FROM_SYSTEM,
> +                      NULL, error, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
> +                      (LPTSTR)&msgbuf, 0, NULL);

... and remove the type cast here? If it works without compiler
warnings, I'd prefer that variant.

> +        fprintf(stderr, "Tap-Win32: Error WriteFile %d - %s\n", error, 
> msgbuf);
> +        LocalFree(msgbuf);
>  #endif
> -            break;
> -        default:
> -            return -1;
> -        }
> +        return 0;
>      }
>  
>      return write_size;

Otherwise, the patch looks good, but I currently cannot test it.

Stefan