qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH 2/2] qga: set file descriptors in qmp_guest_file


From: Denis V. Lunev
Subject: Re: [Qemu-devel] [PATCH 2/2] qga: set file descriptors in qmp_guest_file_open non-blocking on Win32
Date: Mon, 12 Oct 2015 20:47:42 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0

On 10/12/2015 08:38 PM, Michael Roth wrote:
Quoting Denis V. Lunev (2015-10-07 05:59:34)
From: Olga Krishtal <address@hidden>

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 <address@hidden>
Signed-off-by: Denis V. Lunev <address@hidden>
Reviewed-by: Yuri Pudgorodskiy <address@hidden>
CC: Michael Roth <address@hidden>
CC: Stefan Weil <address@hidden>
---
  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 
has_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 = 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 = 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);
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.

you are perfectly correct, I have missed this fact from previous discussion.
Sure this needs to be addressed.

Thank you for a review.

Den



reply via email to

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