[Qemu-devel] [PATCH v4 for 2.5 0/3] qga: non-blocking fd cleanups

[Qemu-devel] [PATCH v4 for 2.5 0/3] qga: non-blocking fd cleanups

[Denis V. Lunev]

This patchset is reincarnation of one patch discussed in the scope of
QEMU 2.4 and rejected for that time. Actually we should use
non-blocking descriptors in QGA on Windows in guest-file-open exactly
like was done for Posix.

Changes from v3:
- handle_set_nonblocking now is local function in qga/commands-win32.c
  It works only in one way - set handle nonblocking.

Changes from v2:
- added fix for wrong argument to CloseHandle
- switched setting non-block for pipes to use separate function

Changes from v1:
- call to qemu_fd_register is moved to a proper place
- moved declaration of opt to a proper place

Signed-off-by: Denis V. Lunev <den@openvz.org>
Signed-off-by: Olga Krishtal <okrishtal@virtuozzo.com>
CC: Yuri Pudgorodskiy <yur@virtuozzo.com>
CC: Michael Roth <mdroth@linux.vnet.ibm.com>

Denis V. Lunev (1):
  qga: drop hand-made guest_file_toggle_flags helper

Olga Krishtal (2):
  qga: fixed CloseHandle in qmp_guest_file_open
  qga: set file descriptor in qmp_guest_file_open non-blocking on Win32

 qga/commands-posix.c | 27 ++-------------------------
 qga/commands-win32.c | 29 ++++++++++++++++++++++++++++-
 2 files changed, 30 insertions(+), 26 deletions(-)

-- 
2.1.4

[Qemu-devel] [PATCH 1/3] qga: drop hand-made guest_file_toggle_flags helper

[Denis V. Lunev]

We'd better use generic qemu_set_nonblock directly.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Yuri Pudgorodskiy <yur@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
CC: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/commands-posix.c | 27 ++-------------------------
 1 file changed, 2 insertions(+), 25 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 67a173a..0ebd473 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__
@@ -385,27 +386,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)
 {
@@ -426,10 +406,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) {
-- 
2.1.4

[Qemu-devel] [PATCH 2/3] qga: fixed CloseHandle in qmp_guest_file_open

[Denis V. Lunev]

From: Olga Krishtal <okrishtal@parallels.com>

CloseHandle use HANDLE as an argument, but not *HANDLE

Signed-off-by: Olga Krishtal <okrishtal@parallels.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Stefan Weil <sw@weilnetz.de>
CC: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/commands-win32.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index d9de23b..97f19d5 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -160,7 +160,7 @@ int64_t qmp_guest_file_open(const char *path, bool has_mode,
 
     fd = guest_file_handle_add(fh, errp);
     if (fd < 0) {
-        CloseHandle(&fh);
+        CloseHandle(fh);
         error_setg(errp, "failed to add handle to qmp handle table");
         return -1;
     }
-- 
2.1.4

[Qemu-devel] [PATCH 3/3] qga: set file descriptor in qmp_guest_file_open non-blocking on Win32

[Denis V. Lunev]

From: Olga Krishtal <okrishtal@parallels.com>

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.

The patch introduces qemu_set_handle_nonoblocking, the local analog
of qemu_set_nonblock for HANDLES.
The usage of handles in qemu_set_non/block is impossible, because for
win32 there is a difference between file discriptors and file handles,
and all file ops are made via Win32 api.

Signed-off-by: Olga Krishtal <okrishtal@parallels.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Michael Roth <mdroth@linux.vnet.ibm.com>
CC: Stefan Weil <sw@weilnetz.de>
---
 qga/commands-win32.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 97f19d5..a5306e7 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -128,6 +128,28 @@ static GuestFileHandle *guest_file_handle_find(int64_t id, Error **errp)
     return NULL;
 }
 
+static void handle_set_nonblocking(HANDLE fh)
+{
+    DWORD file_type, pipe_state;
+    file_type = GetFileType(fh);
+    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(fh, &pipe_state, NULL,
+                                 NULL, NULL, NULL, 0)) {
+        return;
+    }
+    /* The fd is named pipe fd */
+    if (pipe_state & PIPE_NOWAIT) {
+        return;
+    }
+
+    pipe_state |= PIPE_NOWAIT;
+    SetNamedPipeHandleState(fh, &pipe_state, NULL, NULL);
+}
+
 int64_t qmp_guest_file_open(const char *path, bool has_mode,
                             const char *mode, Error **errp)
 {
@@ -158,6 +180,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
+     */
+    handle_set_nonblocking(fh);
+
     fd = guest_file_handle_add(fh, errp);
     if (fd < 0) {
         CloseHandle(fh);
-- 
2.1.4

Re: [Qemu-devel] [PATCH v4 for 2.5 0/3] qga: non-blocking fd cleanups

[Michael Roth]

Quoting Denis V. Lunev (2015-10-28 10:13:54)
> This patchset is reincarnation of one patch discussed in the scope of
> QEMU 2.4 and rejected for that time. Actually we should use
> non-blocking descriptors in QGA on Windows in guest-file-open exactly
> like was done for Posix.
> 
> Changes from v3:
> - handle_set_nonblocking now is local function in qga/commands-win32.c
>   It works only in one way - set handle nonblocking.
> 
> Changes from v2:
> - added fix for wrong argument to CloseHandle
> - switched setting non-block for pipes to use separate function
> 
> Changes from v1:
> - call to qemu_fd_register is moved to a proper place
> - moved declaration of opt to a proper place
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Signed-off-by: Olga Krishtal <okrishtal@virtuozzo.com>
> CC: Yuri Pudgorodskiy <yur@virtuozzo.com>
> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
> 
> Denis V. Lunev (1):
>   qga: drop hand-made guest_file_toggle_flags helper
> 
> Olga Krishtal (2):
>   qga: fixed CloseHandle in qmp_guest_file_open
>   qga: set file descriptor in qmp_guest_file_open non-blocking on Win32

Thanks, applied to qga tree:

  https://github.com/mdroth/qemu/commits/qga

> 
>  qga/commands-posix.c | 27 ++-------------------------
>  qga/commands-win32.c | 29 ++++++++++++++++++++++++++++-
>  2 files changed, 30 insertions(+), 26 deletions(-)
> 
> -- 
> 2.1.4
> 

[Qemu-devel] [PATCH v3 for 2.5 0/3] qga: non-blocking fd cleanups

[Denis V. Lunev]

This patchset is reincarnation of one patch discussed in the scope of
QEMU 2.4 and rejected for that time. Actually we should use
non-blocking descriptors in QGA on Windows in guest-file-open exactly
like was done for Posix.

Changes from v2:
- added fix for wrong argument to CloseHandle
- switched setting non-block for pipes to use separate function

Changes from v1:
- call to qemu_fd_register is moved to a proper place
- moved declaration of opt to a proper place

Signed-off-by: Denis V. Lunev <den@openvz.org>
Signed-off-by: Olga Krishtal <okrishtal@virtuozzo.com>
CC: Yuri Pudgorodskiy <yur@virtuozzo.com>
CC: Michael Roth <mdroth@linux.vnet.ibm.com>

Denis V. Lunev (1):
  qga: drop hand-made guest_file_toggle_flags helper

Olga Krishtal (2):
  qga: fixed CloseHandle in qmp_guest_file_open
  qga: set file descriptor in qmp_guest_file_open non-blocking on Win32

 include/qemu/sockets.h |  2 ++
 qga/commands-posix.c   | 27 ++-------------------------
 qga/commands-win32.c   |  8 +++++++-
 util/oslib-win32.c     | 41 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 52 insertions(+), 26 deletions(-)

-- 
2.1.4

[Qemu-devel] [PATCH 1/3] qga: drop hand-made guest_file_toggle_flags helper

[Denis V. Lunev]

We'd better use generic qemu_set_nonblock directly.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Yuri Pudgorodskiy <yur@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
CC: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qga/commands-posix.c | 27 ++-------------------------
 1 file changed, 2 insertions(+), 25 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 67a173a..0ebd473 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__
@@ -385,27 +386,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)
 {
@@ -426,10 +406,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) {
-- 
2.1.4