From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52024) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZCy1P-0002xI-AR for qemu-devel@nongnu.org; Wed, 08 Jul 2015 18:40:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZCy1M-0004Hh-2G for qemu-devel@nongnu.org; Wed, 08 Jul 2015 18:40:55 -0400 Received: from relay.parallels.com ([195.214.232.42]:48298) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZCy1L-0004Fe-Ls for qemu-devel@nongnu.org; Wed, 08 Jul 2015 18:40:51 -0400 Message-ID: <559DA6DE.2050407@openvz.org> Date: Thu, 9 Jul 2015 01:40:30 +0300 From: "Denis V. Lunev" MIME-Version: 1.0 References: <1435659923-2211-1-git-send-email-den@openvz.org> <1435659923-2211-2-git-send-email-den@openvz.org> <20150708211634.23206.41928@loki> In-Reply-To: <20150708211634.23206.41928@loki> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit 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: Michael Roth Cc: Olga Krishtal , qemu-devel@nongnu.org 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 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 *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 = fcntl(fd, F_GETFL); >> - if (old_flags == -1) { >> - error_setg_errno(err, errno, QERR_QGA_COMMAND_FAILED, >> - "failed to fetch filehandle flags"); >> - return -1; >> - } >> - >> - ret = fcntl(fd, F_SETFL, set ? (old_flags | flags) : (old_flags & ~flags)); >> - if (ret == -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 has_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 = 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 = 0; >> - WSAEventSelect(fd, NULL, 0); >> + HANDLE handle; >> + DWORD file_type, pipe_state; >> + >> + handle = (HANDLE)_get_osfhandle(fd); >> + if (handle == INVALID_HANDLE_VALUE) { >> + return; >> + } >> + >> + file_type = GetFileType(handle); >> + if (file_type != FILE_TYPE_PIPE) { >> + return; >> + } >> + >> + /* If file_type == 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 == !(pipe_state & PIPE_NOWAIT)) { >> + /* In this case we do not need perform any operation, because >> + * nonblocking = true and PIPE_NOWAIT is already set or >> + * nonblocking = false and PIPE_NOWAIT is not set */ >> + return; >> + } >> + >> + if (nonblocking) { >> + pipe_state |= PIPE_NOWAIT; >> + } else { >> + pipe_state &= ~PIPE_NOWAIT; >> + } >> + >> + SetNamedPipeHandleState(handle, &pipe_state, NULL, NULL); >> + return; >> + } >> + >> + /* The fd is socket fd */ >> + unsigned long opt = (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 = 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. 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. Den