From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57559) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zlh3I-0004Ix-Ie for qemu-devel@nongnu.org; Mon, 12 Oct 2015 13:38:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zlh3F-0004zk-8n for qemu-devel@nongnu.org; Mon, 12 Oct 2015 13:38:24 -0400 Received: from e39.co.us.ibm.com ([32.97.110.160]:49726) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zlh3F-0004zT-2n for qemu-devel@nongnu.org; Mon, 12 Oct 2015 13:38:21 -0400 Received: from localhost by e39.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 12 Oct 2015 11:38:19 -0600 Received: from b03cxnp08027.gho.boulder.ibm.com (b03cxnp08027.gho.boulder.ibm.com [9.17.130.19]) by d03dlp01.boulder.ibm.com (Postfix) with ESMTP id D738F1FF0042 for ; Mon, 12 Oct 2015 11:26:29 -0600 (MDT) Received: from d03av03.boulder.ibm.com (d03av03.boulder.ibm.com [9.17.195.169]) by b03cxnp08027.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t9CHcHBE4915586 for ; Mon, 12 Oct 2015 10:38:17 -0700 Received: from d03av03.boulder.ibm.com (localhost [127.0.0.1]) by d03av03.boulder.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t9CHcHI1028948 for ; Mon, 12 Oct 2015 11:38:17 -0600 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <1444215574-11895-3-git-send-email-den@openvz.org> References: <1444215574-11895-1-git-send-email-den@openvz.org> <1444215574-11895-3-git-send-email-den@openvz.org> Message-ID: <20151012173812.10003.73590@loki> Date: Mon, 12 Oct 2015 12:38:12 -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-07 05:59:34) > 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. > = > 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 | 52 ++++++++++++++++++++++++++++++++++++++++++++++= +----- > 2 files changed, 53 insertions(+), 5 deletions(-) > = > diff --git a/qga/commands-win32.c b/qga/commands-win32.c > index 3374678..3274417 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 > @@ -158,6 +159,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)); > + > 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..f19aed5 100644 > --- a/util/oslib-win32.c > +++ b/util/oslib-win32.c > @@ -121,17 +121,59 @@ 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; > + > + 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 */ > + unsigned long opt =3D (unsigned long)nonblocking; > + if (!nonblocking) { > + WSAEventSelect(fd, NULL, 0); > + } > ioctlsocket(fd, FIONBIO, &opt); > } > = > +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_set_fd_nonblocking(fd, true); > qemu_fd_register(fd); This still potentially calls qemu_fd_register() on a non-socket FD as noted in prior review. I think the fix is trivial, but if you can change and re-test that would be ideal. Since guest-file* already supports reading/writing to w32 pipes I can pick this up during soft-freeze if it comes to that. Since we're still taking the approach of generalizing qemu_set_nonblock() for win32 I'd really prefer to get Stefan's Ack/Reviewed-by before applying. > } > = > -- = > 2.1.4 >=20