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 <andrew.baum...@microsoft.com>
> ---
> 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
