qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 01/10] util, qga: drop guest_file_toggle_flags


From: Denis V. Lunev
Subject: Re: [Qemu-devel] [PATCH 01/10] util, qga: drop guest_file_toggle_flags
Date: Thu, 9 Jul 2015 01:40:30 +0300
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:31.0) Gecko/20100101 Thunderbird/31.7.0

On 09/07/15 00:16, Michael Roth wrote:
Quoting Denis V. Lunev (2015-06-30 05:25:14)
From: Olga Krishtal <address@hidden>

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 <address@hidden>
Signed-off-by: Denis V. Lunev <address@hidden>
CC: Eric Blake <address@hidden>
CC: Michael Roth <address@hidden>
---
  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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]