From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59167) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZCwrB-0004FH-BH for qemu-devel@nongnu.org; Wed, 08 Jul 2015 17:26:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZCwr8-0005Dy-Kc for qemu-devel@nongnu.org; Wed, 08 Jul 2015 17:26:17 -0400 Received: from e38.co.us.ibm.com ([32.97.110.159]:48331) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZCwr8-0005Dr-Dq for qemu-devel@nongnu.org; Wed, 08 Jul 2015 17:26:14 -0400 Received: from /spool/local by e38.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 8 Jul 2015 15:26:13 -0600 Received: from b03cxnp08027.gho.boulder.ibm.com (b03cxnp08027.gho.boulder.ibm.com [9.17.130.19]) by d03dlp03.boulder.ibm.com (Postfix) with ESMTP id 11A9219D8026 for ; Wed, 8 Jul 2015 15:17:11 -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 t68LQA4R31064194 for ; Wed, 8 Jul 2015 14:26:11 -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 t68LQAgm021359 for ; Wed, 8 Jul 2015 15:26:10 -0600 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <1435659923-2211-2-git-send-email-den@openvz.org> References: <1435659923-2211-1-git-send-email-den@openvz.org> <1435659923-2211-2-git-send-email-den@openvz.org> Message-ID: <20150708211634.23206.41928@loki> Date: Wed, 08 Jul 2015 16:16:34 -0500 Subject: Re: [Qemu-devel] [PATCH 01/10] util, qga: drop guest_file_toggle_flags List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Denis V. Lunev" Cc: Olga Krishtal , qemu-devel@nongnu.org Quoting Denis V. Lunev (2015-06-30 05:25:14) > From: Olga Krishtal > = > guest_file_toggle_flags is a copy from semi-portable qemu_set_nonblock. > The latter is not working properly for Windows due to reduced Windows > Posix implementation. > = > 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 problem. > = > Signed-off-by: Olga Krishtal > Signed-off-by: Denis V. Lunev > CC: Eric Blake > CC: Michael Roth > --- > qga/commands-posix.c | 27 ++------------------------- > util/oslib-win32.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++= +----- > 2 files changed, 49 insertions(+), 30 deletions(-) > = > diff --git a/qga/commands-posix.c b/qga/commands-posix.c > index befd00b..40dbe25 100644 > --- a/qga/commands-posix.c > +++ b/qga/commands-posix.c > @@ -28,6 +28,7 @@ > #include "qapi/qmp/qerror.h" > #include "qemu/queue.h" > #include "qemu/host-utils.h" > +#include "qemu/sockets.h" > = > #ifndef CONFIG_HAS_ENVIRON > #ifdef __APPLE__ > @@ -376,27 +377,6 @@ safe_open_or_create(const char *path, const char *mo= de, Error **errp) > return NULL; > } > = > -static int guest_file_toggle_flags(int fd, int flags, bool set, Error **= err) > -{ > - int ret, old_flags; > - > - old_flags =3D fcntl(fd, F_GETFL); > - if (old_flags =3D=3D -1) { > - error_setg_errno(err, errno, QERR_QGA_COMMAND_FAILED, > - "failed to fetch filehandle flags"); > - return -1; > - } > - > - ret =3D fcntl(fd, F_SETFL, set ? (old_flags | flags) : (old_flags & = ~flags)); > - if (ret =3D=3D -1) { > - error_setg_errno(err, errno, QERR_QGA_COMMAND_FAILED, > - "failed to set filehandle flags"); > - return -1; > - } > - > - return ret; > -} > - > int64_t qmp_guest_file_open(const char *path, bool has_mode, const char = *mode, > Error **errp) > { > @@ -417,10 +397,7 @@ int64_t qmp_guest_file_open(const char *path, bool h= as_mode, const char *mode, > /* set fd non-blocking to avoid common use cases (like reading from a > * named pipe) from hanging the agent > */ > - if (guest_file_toggle_flags(fileno(fh), O_NONBLOCK, true, errp) < 0)= { > - fclose(fh); > - return -1; > - } > + qemu_set_nonblock(fileno(fh)); > = > handle =3D guest_file_handle_add(fh, errp); > if (handle < 0) { > diff --git a/util/oslib-win32.c b/util/oslib-win32.c > index 730a670..1a6ae72 100644 > --- a/util/oslib-win32.c > +++ b/util/oslib-win32.c > @@ -119,17 +119,59 @@ struct tm *localtime_r(const time_t *timep, struct = tm *result) > return p; > } > = > -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); We wouldn't want to pass a non-socket FD to qemu_fd_register(), so this should get moved to qemu_set_fd_nonblocking() at least. I think it's confusing to have a qemu_set_nonblock() that's limited to pipes and sockets. At least with the current code it's fairly obvious it's intended for sockets. It might be worthwhile if we could at least cover disk files, but I seem to recall that being non-trivial on w32 and generally requiring non-posix functions that support overlapped IO. So, if we're only extending this to support pipes it should probably just be a separate helper in qga/. > } > = > -- = > 2.1.4 >=20