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

[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

[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>
CC: Michael Roth <mdroth@linux.vnet.ibm.com>
CC: Stefan Weil <sw@weilnetz.de>
---
 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 analog of qemu_set_non/block 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>
---
 include/qemu/sockets.h |  2 ++
 qga/commands-win32.c   |  6 ++++++
 util/oslib-win32.c     | 41 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 49 insertions(+)

diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 5a183c5..e2323f4 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -39,6 +39,8 @@ int socket_set_cork(int fd, int v);
 int socket_set_nodelay(int fd);
 void qemu_set_block(int fd);
 void qemu_set_nonblock(int fd);
+void qemu_set_nonblock_by_handle(int64_t fh);
+void qemu_set_block_by_handle(int64_t fh);
 int socket_set_fast_reuse(int fd);
 int send_all(int fd, const void *buf, int len1);
 int recv_all(int fd, void *buf, int len1, bool single_read);
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 97f19d5..f959d75 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_by_handle((int64_t)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 09f9e98..4a9fd8e 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -121,6 +121,37 @@ struct tm *localtime_r(const time_t *timep, struct tm *result)
 }
 #endif /* CONFIG_LOCALTIME_R */
 
+static void qemu_set_handle_nonblocking(int64_t fh, bool nonblocking)
+{
+    DWORD file_type, pipe_state;
+    HANDLE handle = (HANDLE)fh;
+    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)) {
+        return;
+    }
+    /* 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);
+}
+
 void qemu_set_block(int fd)
 {
     unsigned long opt = 0;
@@ -135,6 +166,16 @@ void qemu_set_nonblock(int fd)
     qemu_fd_register(fd);
 }
 
+void qemu_set_block_by_handle(int64_t fh)
+{
+    qemu_set_handle_nonblocking(fh, false);
+}
+
+void qemu_set_nonblock_by_handle(int64_t fh)
+{
+    qemu_set_handle_nonblocking(fh, true);
+}
+
 int socket_set_fast_reuse(int fd)
 {
     /* Enabling the reuse of an endpoint that was used by a socket still in
-- 
2.1.4

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

[Stefan Weil]

[-- Attachment #1: Type: text/plain, Size: 977 bytes --]

Am 27.10.2015 um 18:48 schrieb 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>
> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
> CC: Stefan Weil <sw@weilnetz.de>
> ---
>  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;
>      }
> 

Reviewed-by: Stefan Weil <sw@weilnetz.de>



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Michael Roth]

Quoting Denis V. Lunev (2015-10-27 12:48:43)
> 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 analog of qemu_set_non/block 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.

If this is specific to HANDLEs, why do we need to cast back and forth
between int64_t and HANDLE? I haven't build tested, but it seems like
this would break for 32-bit mingw builds.

I would define these as qemu_set_*_by_handle(HANDLE fh, ...) instead
and make them win32 only. If someone wants to introduce a FILE*
variant for posix they can introduce it as
qemu_set_*_by_handle(FILE *fh, ...) rather than us needing to
abstract away the handle type.

> 
> 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>
> ---
>  include/qemu/sockets.h |  2 ++
>  qga/commands-win32.c   |  6 ++++++
>  util/oslib-win32.c     | 41 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 49 insertions(+)
> 
> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
> index 5a183c5..e2323f4 100644
> --- a/include/qemu/sockets.h
> +++ b/include/qemu/sockets.h
> @@ -39,6 +39,8 @@ int socket_set_cork(int fd, int v);
>  int socket_set_nodelay(int fd);
>  void qemu_set_block(int fd);
>  void qemu_set_nonblock(int fd);
> +void qemu_set_nonblock_by_handle(int64_t fh);
> +void qemu_set_block_by_handle(int64_t fh);
>  int socket_set_fast_reuse(int fd);
>  int send_all(int fd, const void *buf, int len1);
>  int recv_all(int fd, void *buf, int len1, bool single_read);
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 97f19d5..f959d75 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_by_handle((int64_t)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 09f9e98..4a9fd8e 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -121,6 +121,37 @@ struct tm *localtime_r(const time_t *timep, struct tm *result)
>  }
>  #endif /* CONFIG_LOCALTIME_R */
> 
> +static void qemu_set_handle_nonblocking(int64_t fh, bool nonblocking)
> +{
> +    DWORD file_type, pipe_state;
> +    HANDLE handle = (HANDLE)fh;
> +    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)) {
> +        return;
> +    }
> +    /* 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);
> +}
> +
>  void qemu_set_block(int fd)
>  {
>      unsigned long opt = 0;
> @@ -135,6 +166,16 @@ void qemu_set_nonblock(int fd)
>      qemu_fd_register(fd);
>  }
> 
> +void qemu_set_block_by_handle(int64_t fh)
> +{
> +    qemu_set_handle_nonblocking(fh, false);
> +}
> +
> +void qemu_set_nonblock_by_handle(int64_t fh)
> +{
> +    qemu_set_handle_nonblocking(fh, true);
> +}
> +
>  int socket_set_fast_reuse(int fd)
>  {
>      /* Enabling the reuse of an endpoint that was used by a socket still in
> -- 
> 2.1.4
> 

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

[Denis V. Lunev]

On 10/27/2015 10:11 PM, Michael Roth wrote:
> Quoting Denis V. Lunev (2015-10-27 12:48:43)
>> 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 analog of qemu_set_non/block 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.
> If this is specific to HANDLEs, why do we need to cast back and forth
> between int64_t and HANDLE? I haven't build tested, but it seems like
> this would break for 32-bit mingw builds.
>
> I would define these as qemu_set_*_by_handle(HANDLE fh, ...) instead
> and make them win32 only. If someone wants to introduce a FILE*
> variant for posix they can introduce it as
> qemu_set_*_by_handle(FILE *fh, ...) rather than us needing to
> abstract away the handle type.

may be it would be better to add static function for this in QGA for now?

I am eager to drop this code at once for Posix and Windows and
switch to GLIB like was done for guest exec.

Den

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

[Michael Roth]

Quoting Michael Roth (2015-10-27 14:11:12)
> Quoting Denis V. Lunev (2015-10-27 12:48:43)
> > 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 analog of qemu_set_non/block 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.
> 
> If this is specific to HANDLEs, why do we need to cast back and forth
> between int64_t and HANDLE? I haven't build tested, but it seems like
> this would break for 32-bit mingw builds.
> 
> I would define these as qemu_set_*_by_handle(HANDLE fh, ...) instead
> and make them win32 only. If someone wants to introduce a FILE*
> variant for posix they can introduce it as
> qemu_set_*_by_handle(FILE *fh, ...) rather than us needing to
> abstract away the handle type.

Actually that would be silly since win32 has FILE as well. So I think
this interface will only ever make sense for HANDLE so let's be
explicit about it.

> 
> > 
> > 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>
> > ---
> >  include/qemu/sockets.h |  2 ++
> >  qga/commands-win32.c   |  6 ++++++
> >  util/oslib-win32.c     | 41 +++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 49 insertions(+)
> > 
> > diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
> > index 5a183c5..e2323f4 100644
> > --- a/include/qemu/sockets.h
> > +++ b/include/qemu/sockets.h
> > @@ -39,6 +39,8 @@ int socket_set_cork(int fd, int v);
> >  int socket_set_nodelay(int fd);
> >  void qemu_set_block(int fd);
> >  void qemu_set_nonblock(int fd);
> > +void qemu_set_nonblock_by_handle(int64_t fh);
> > +void qemu_set_block_by_handle(int64_t fh);
> >  int socket_set_fast_reuse(int fd);
> >  int send_all(int fd, const void *buf, int len1);
> >  int recv_all(int fd, void *buf, int len1, bool single_read);
> > diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> > index 97f19d5..f959d75 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_by_handle((int64_t)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 09f9e98..4a9fd8e 100644
> > --- a/util/oslib-win32.c
> > +++ b/util/oslib-win32.c
> > @@ -121,6 +121,37 @@ struct tm *localtime_r(const time_t *timep, struct tm *result)
> >  }
> >  #endif /* CONFIG_LOCALTIME_R */
> > 
> > +static void qemu_set_handle_nonblocking(int64_t fh, bool nonblocking)
> > +{
> > +    DWORD file_type, pipe_state;
> > +    HANDLE handle = (HANDLE)fh;
> > +    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)) {
> > +        return;
> > +    }
> > +    /* 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);
> > +}
> > +
> >  void qemu_set_block(int fd)
> >  {
> >      unsigned long opt = 0;
> > @@ -135,6 +166,16 @@ void qemu_set_nonblock(int fd)
> >      qemu_fd_register(fd);
> >  }
> > 
> > +void qemu_set_block_by_handle(int64_t fh)
> > +{
> > +    qemu_set_handle_nonblocking(fh, false);
> > +}
> > +
> > +void qemu_set_nonblock_by_handle(int64_t fh)
> > +{
> > +    qemu_set_handle_nonblocking(fh, true);
> > +}
> > +
> >  int socket_set_fast_reuse(int fd)
> >  {
> >      /* Enabling the reuse of an endpoint that was used by a socket still in
> > -- 
> > 2.1.4
> > 

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

[Michael Roth]

Quoting Denis V. Lunev (2015-10-27 14:13:57)
> On 10/27/2015 10:11 PM, Michael Roth wrote:
> > Quoting Denis V. Lunev (2015-10-27 12:48:43)
> >> 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 analog of qemu_set_non/block 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.
> > If this is specific to HANDLEs, why do we need to cast back and forth
> > between int64_t and HANDLE? I haven't build tested, but it seems like
> > this would break for 32-bit mingw builds.
> >
> > I would define these as qemu_set_*_by_handle(HANDLE fh, ...) instead
> > and make them win32 only. If someone wants to introduce a FILE*
> > variant for posix they can introduce it as
> > qemu_set_*_by_handle(FILE *fh, ...) rather than us needing to
> > abstract away the handle type.
> 
> may be it would be better to add static function for this in QGA for now?

I'd be fine with either approach. It could be generally useful for
other w32 users. But if we're thinking about dropping the QGA
use case soon then maybe having it live in QGA is best.

> 
> I am eager to drop this code at once for Posix and Windows and
> switch to GLIB like was done for guest exec.

You mean switching all the guest-file-* interfaces to glib? I
took a stab at it once for w32 guest-file-* implementation, but
one issue I hit was that I couldn't figure out how to implement
guest-file-seek to report back the absolute position in the
file, or whether or not we'd hit EOF. You can set position
via g_io_channel_seek_position(), but if they hit EOF, or are
using relative offsets via G_SEEK_CUR, you don't really know
the position and glib doesn't seem to provide a way to query
that. We could maybe work around it by tracking it manually
via guest-file-* calls but that sounds terrible.

Hopefully I just missed something though. Also couldn't figure
out how you can get glib to report that you'd already seeked
to EOF. I had some comments about it in my WIP:

https://github.com/mdroth/qemu/commit/8b2e5c69266bb48e492af9826122c2aaa4a82197#diff-7f29c3e51a7b387cc7717e7be4f6e205R525

> 
> Den
> 

[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 3/3] qga: set file descriptor in qmp_guest_file_open non-blocking on Win32

[Denis V. Lunev]

On 10/27/2015 10:49 PM, Michael Roth wrote:
> Quoting Denis V. Lunev (2015-10-27 14:13:57)
>> On 10/27/2015 10:11 PM, Michael Roth wrote:
>>> Quoting Denis V. Lunev (2015-10-27 12:48:43)
>>>> 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 analog of qemu_set_non/block 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.
>>> If this is specific to HANDLEs, why do we need to cast back and forth
>>> between int64_t and HANDLE? I haven't build tested, but it seems like
>>> this would break for 32-bit mingw builds.
>>>
>>> I would define these as qemu_set_*_by_handle(HANDLE fh, ...) instead
>>> and make them win32 only. If someone wants to introduce a FILE*
>>> variant for posix they can introduce it as
>>> qemu_set_*_by_handle(FILE *fh, ...) rather than us needing to
>>> abstract away the handle type.
>> may be it would be better to add static function for this in QGA for now?
> I'd be fine with either approach. It could be generally useful for
> other w32 users. But if we're thinking about dropping the QGA
> use case soon then maybe having it live in QGA is best.
>
>> I am eager to drop this code at once for Posix and Windows and
>> switch to GLIB like was done for guest exec.
> You mean switching all the guest-file-* interfaces to glib? I
> took a stab at it once for w32 guest-file-* implementation, but
> one issue I hit was that I couldn't figure out how to implement
> guest-file-seek to report back the absolute position in the
> file, or whether or not we'd hit EOF. You can set position
> via g_io_channel_seek_position(), but if they hit EOF, or are
> using relative offsets via G_SEEK_CUR, you don't really know
> the position and glib doesn't seem to provide a way to query
> that. We could maybe work around it by tracking it manually
> via guest-file-* calls but that sounds terrible.
>
> Hopefully I just missed something though. Also couldn't figure
> out how you can get glib to report that you'd already seeked
> to EOF. I had some comments about it in my WIP:
>
> https://github.com/mdroth/qemu/commit/8b2e5c69266bb48e492af9826122c2aaa4a82197#diff-7f29c3e51a7b387cc7717e7be4f6e205R525

I see. Then we can have platform specific open code and use FILE*
interface for the rest. According to Olga HANDLE -> fd -> FILE*
transition is possible.

At least we will try :)

Den