From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55115) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zp2pV-0001S7-HO for qemu-devel@nongnu.org; Wed, 21 Oct 2015 19:30:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zp2pR-0004zW-DS for qemu-devel@nongnu.org; Wed, 21 Oct 2015 19:30:01 -0400 Received: from e18.ny.us.ibm.com ([129.33.205.208]:55999) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zp2pR-0004zQ-97 for qemu-devel@nongnu.org; Wed, 21 Oct 2015 19:29:57 -0400 Received: from localhost by e18.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 21 Oct 2015 19:29:56 -0400 Received: from b01cxnp22033.gho.pok.ibm.com (b01cxnp22033.gho.pok.ibm.com [9.57.198.23]) by d01dlp03.pok.ibm.com (Postfix) with ESMTP id 8420AC90042 for ; Wed, 21 Oct 2015 19:18:04 -0400 (EDT) Received: from d01av03.pok.ibm.com (d01av03.pok.ibm.com [9.56.224.217]) by b01cxnp22033.gho.pok.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t9LNTrO358851518 for ; Wed, 21 Oct 2015 23:29:53 GMT Received: from d01av03.pok.ibm.com (localhost [127.0.0.1]) by d01av03.pok.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t9LNTrc3011108 for ; Wed, 21 Oct 2015 19:29:53 -0400 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <1444913471-15132-3-git-send-email-den@openvz.org> References: <1444913471-15132-1-git-send-email-den@openvz.org> <1444913471-15132-3-git-send-email-den@openvz.org> Message-ID: <20151021232949.10420.46582@loki> Date: Wed, 21 Oct 2015 18:29:49 -0500 Subject: Re: [Qemu-devel] [PATCH 2/2] qga: set file descriptors in qmp_guest_file_open non-blocking on Win32 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Denis V. Lunev" Cc: Olga Krishtal , Stefan Weil , qemu-devel@nongnu.org Quoting Denis V. Lunev (2015-10-15 07:51:11) > From: Olga Krishtal > = > Set fd non-blocking to avoid common use cases (like reading from a > named pipe) from hanging the agent. This was missed in the original > code. > = > Restore compatibility with Posix implementation. > = > The patch adds Win32 specific implementation of qemu_set_nonblock. > = > On Windows OS there is a separate API for changing flags of file, pipes > and sockets. Portable way to change file descriptor flags requires > to detect file descriptor type and proper actions depending of that > type. The patch adds wrapper qemu_set_fd_nonblocking into Windows specific > code to handle this stuff properly. > = > The only problem is that qemu_set_nonblock is void but this should not > be a big deal. > = > Despite the fact that qemu_fd_register() is used only once here and now > it is difficult to drop this function as it uses AioContext internals > which we do not want to involve here. > = > Signed-off-by: Olga Krishtal > Signed-off-by: Denis V. Lunev > Reviewed-by: Yuri Pudgorodskiy > CC: Michael Roth > CC: Stefan Weil > --- > qga/commands-win32.c | 6 ++++++ > util/oslib-win32.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++= ------ > 2 files changed, 57 insertions(+), 6 deletions(-) > = > diff --git a/qga/commands-win32.c b/qga/commands-win32.c > index 41bdd3f..745bddf 100644 > --- a/qga/commands-win32.c > +++ b/qga/commands-win32.c > @@ -34,6 +34,7 @@ > #include "qapi/qmp/qerror.h" > #include "qemu/queue.h" > #include "qemu/host-utils.h" > +#include "qemu/sockets.h" > = > #ifndef SHTDN_REASON_FLAG_PLANNED > #define SHTDN_REASON_FLAG_PLANNED 0x80000000 > @@ -156,6 +157,11 @@ int64_t qmp_guest_file_open(const char *path, bool h= as_mode, > return -1; > } > = > + /* set fd non-blocking to avoid common use cases (like reading from a > + * named pipe) from hanging the agent > + */ > + qemu_set_nonblock(fileno(fh)); This doesn't seem to build for me: CC qga/channel-win32.o /home/mdroth/w/qemu3.git/qga/commands-win32.c: In function =E2=80=98qmp_guest_file_open=E2=80=99: /home/mdroth/w/qemu3.git/qga/commands-win32.c:165: warning: dereferencing =E2=80=98void *=E2=80=99 pointer /home/mdroth/w/qemu3.git/qga/commands-win32.c:165: error: request for member =E2=80=98_file=E2=80=99 in something not a structure or union That's with ubuntu 14.04 and running: ../qemu3.git/configure --cross-prefix=3Di586-mingw32msvc- --target-list=3Dx86_64-softmmu --extra-cflags=3D-Wall && make -j4 fileno() expects a FILE *, but fh is actually a HANDLE. Is there an environment where this is not the case? I think what you want is _open_osfhandle(). Note that it looks like you then might need to track the resulting fd and use _close() in place of CloseHandle() in guest-file-close: https://msdn.microsoft.com/en-us/library/bdts1c9x.aspx That also make me a bit concerned that read/write operations on the HANDLE would then need to be replaced with read()/write() etc... the documentation doesn't seem very clear on this. I'm starting to think it best we just introduce a qemu_set_nonblock_w32_handle(HANDLE ...) to do this, and leave the existing qemu_set_nonblock() as it was before (winsock FDs only). If people want to risk whatever wierd stuff might come about by switching between HANDLE/fd they can use _open_osfhandle()/_get_osfhandle() prior to calling qemu_set_nonblock*(). But since we started off with a HANDLE in the first place, there's no reason to us to switch to fd, then back to HANDLE. > + > fd =3D guest_file_handle_add(fh, errp); > if (fd < 0) { > CloseHandle(&fh); > diff --git a/util/oslib-win32.c b/util/oslib-win32.c > index 08f5a9c..e1580c8 100644 > --- a/util/oslib-win32.c > +++ b/util/oslib-win32.c > @@ -121,18 +121,63 @@ struct tm *localtime_r(const time_t *timep, struct = tm *result) > } > #endif /* CONFIG_LOCALTIME_R */ > = > -void qemu_set_block(int fd) > +static void qemu_set_fd_nonblocking(int fd, bool nonblocking) > { > - unsigned long opt =3D 0; > - WSAEventSelect(fd, NULL, 0); > + HANDLE handle; > + DWORD file_type, pipe_state; > + unsigned long opt =3D (unsigned long)nonblocking; > + > + handle =3D (HANDLE)_get_osfhandle(fd); > + if (handle =3D=3D INVALID_HANDLE_VALUE) { > + return; > + } > + > + file_type =3D GetFileType(handle); > + if (file_type !=3D FILE_TYPE_PIPE) { > + return; > + } > + > + /* If file_type =3D=3D FILE_TYPE_PIPE, according to msdn > + * the specified file is socket or named pipe */ > + if (GetNamedPipeHandleState(handle, &pipe_state, NULL, > + NULL, NULL, NULL, 0)) { > + /* The fd is named pipe fd */ > + if (!nonblocking =3D=3D !(pipe_state & PIPE_NOWAIT)) { > + /* In this case we do not need perform any operation, because > + * nonblocking =3D true and PIPE_NOWAIT is already set or > + * nonblocking =3D false and PIPE_NOWAIT is not set */ > + return; > + } > + > + if (nonblocking) { > + pipe_state |=3D PIPE_NOWAIT; > + } else { > + pipe_state &=3D ~PIPE_NOWAIT; > + } > + > + SetNamedPipeHandleState(handle, &pipe_state, NULL, NULL); > + return; > + } > + > + /* The fd is socket fd */ > + if (!nonblocking) { > + WSAEventSelect(fd, NULL, 0); > + } > ioctlsocket(fd, FIONBIO, &opt); > + if (nonblocking) { > + qemu_fd_register(fd); > + } > + return; > +} > + > +void qemu_set_block(int fd) > +{ > + qemu_set_fd_nonblocking(fd, false); > } > = > void qemu_set_nonblock(int fd) > { > - unsigned long opt =3D 1; > - ioctlsocket(fd, FIONBIO, &opt); > - qemu_fd_register(fd); > + qemu_set_fd_nonblocking(fd, true); > } > = > int socket_set_fast_reuse(int fd) > -- = > 2.1.4 >=20