From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38824) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZD0fg-0007Ms-3J for qemu-devel@nongnu.org; Wed, 08 Jul 2015 21:30:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZD0fc-0004hh-9E for qemu-devel@nongnu.org; Wed, 08 Jul 2015 21:30:39 -0400 Received: from e34.co.us.ibm.com ([32.97.110.152]:47998) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZD0fc-0004hQ-1G for qemu-devel@nongnu.org; Wed, 08 Jul 2015 21:30:36 -0400 Received: from /spool/local by e34.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 8 Jul 2015 19:30:35 -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 4328B19D8026 for ; Wed, 8 Jul 2015 19:21:33 -0600 (MDT) Received: from d03av02.boulder.ibm.com (d03av02.boulder.ibm.com [9.17.195.168]) by b03cxnp08027.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t691UXtm39714868 for ; Wed, 8 Jul 2015 18:30:33 -0700 Received: from d03av02.boulder.ibm.com (localhost [127.0.0.1]) by d03av02.boulder.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t691UX2e014025 for ; Wed, 8 Jul 2015 19:30:33 -0600 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <559DA6DE.2050407@openvz.org> References: <1435659923-2211-1-git-send-email-den@openvz.org> <1435659923-2211-2-git-send-email-den@openvz.org> <20150708211634.23206.41928@loki> <559DA6DE.2050407@openvz.org> Message-ID: <20150709000938.23206.13696@loki> Date: Wed, 08 Jul 2015 19:09:38 -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-07-08 17:40:30) > On 09/07/15 00:16, Michael Roth wrote: > > 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 spec= ific > >> 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 = *mode, 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 c= har *mode, > >> Error **errp) > >> { > >> @@ -417,10 +397,7 @@ int64_t qmp_guest_file_open(const char *path, boo= l has_mode, const char *mode, > >> /* set fd non-blocking to avoid common use cases (like reading f= rom 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, stru= ct 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, bec= ause > >> + * 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/. > > > at my opinion you are a little bit wrong here. First of all, from > Linux point of view, O_NONBLOCK for ordinary file is noop, i.e. > there is no difference in behavior with and without this flag > for such descriptors. Yes, I think you're right... I'd forgotten that the reason we use it in guest-file-open was for dealing with pipes/chardevs/sockets rather than anything to do with files. > = > This means that the write will take some time and will finish > without indefinite timeout. Sockets and pipes are different, > they can hang in read/write forever and thus the flag has > some value to avoid this. > = > The same applies for Windows platform too. Setting nonblock > here we want to avoid infinite hang and we do not want to > implement asynchronous IO, which is implemented using > overlapped API. > = > There are only 2 primitives in QGA which subjects to the rules > above - sockets and pipes. Thus this common helper makes > sense. If we will need more primitives - we will add proper > distinction into these calls. There's still an important invariant here in that O_NONBLOCK ensures EAGAIN in cases where you might otherwise wait indefinitely. That covers named/anonymous pipes, sockets, files, chardevs, any fd. To take the socket-only w32 implementation and only tack on pipes seems somewhat arbitrary in comparison, but I do understand your logic from a "FDs that make sense to use it with" standpoint. I think you'll need to Cc: our w32 maintainer Stefan Weil for any such change in oslib-win32.c though. Personally I think set_pipe_nonblock() in qga/commands-win32.c is clearer, and it seems unlikely we'll ever end up with a 2nd w32 user of it so we don't stand to gain much from a generalized function. > = > Den >=20