From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34430) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZyxLB-0008SX-JY for qemu-devel@nongnu.org; Wed, 18 Nov 2015 02:39:42 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZyxL6-00047V-Id for qemu-devel@nongnu.org; Wed, 18 Nov 2015 02:39:41 -0500 Received: from smtp.mail.uni-mannheim.de ([134.155.96.80]:46879) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZyxL6-00046o-7l for qemu-devel@nongnu.org; Wed, 18 Nov 2015 02:39:36 -0500 References: <1447787363-15216-1-git-send-email-Andrew.Baumann@microsoft.com> <1447787363-15216-3-git-send-email-Andrew.Baumann@microsoft.com> From: Stefan Weil Message-ID: <564C2B39.7080500@weilnetz.de> Date: Wed, 18 Nov 2015 08:39:37 +0100 MIME-Version: 1.0 In-Reply-To: <1447787363-15216-3-git-send-email-Andrew.Baumann@microsoft.com> Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2/2] tap-win32: disable broken async write path List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andrew Baumann , qemu-devel@nongnu.org Cc: Jason Wang 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 > --- > 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