[Qemu-devel] [PATCH 0/2] qga: non-blocking fd cleanups

[Qemu-devel] [PATCH 0/2] 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.

Signed-off-by: Denis V. Lunev <den@openvz.org>
Signed-off-by: Olga Krishtal <okrishtal@virtuozzo.com>
Reviewed-by: Yuri Pudgorodskiy <yur@virtuozzo.com>

CC: Michael Roth <mdroth@linux.vnet.ibm.com>

[Qemu-devel] [PATCH 1/2] 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>
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 8989912..1232d30 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

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

[Eric Blake]

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

On 10/07/2015 04:59 AM, Denis V. Lunev wrote:
> 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>
> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  qga/commands-posix.c | 27 ++-------------------------
>  1 file changed, 2 insertions(+), 25 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

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

[Denis V. Lunev]

From: Olga Krishtal <okrishtal@virtuozzo.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.

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 <okrishtal@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Yuri Pudgorodskiy <yur@virtuozzo.com>
CC: Michael Roth <mdroth@linux.vnet.ibm.com>
CC: Stefan Weil <sw@weilnetz.de>
---
 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);
 }
 
-- 
2.1.4

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

[Michael Roth]

Quoting Denis V. Lunev (2015-10-07 05:59:34)
> From: Olga Krishtal <okrishtal@virtuozzo.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.
> 
> 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 <okrishtal@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Reviewed-by: Yuri Pudgorodskiy <yur@virtuozzo.com>
> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
> CC: Stefan Weil <sw@weilnetz.de>
> ---
>  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.

>  }
> 
> -- 
> 2.1.4
> 

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

[Denis V. Lunev]

On 10/12/2015 08:38 PM, Michael Roth wrote:
> Quoting Denis V. Lunev (2015-10-07 05:59:34)
>> From: Olga Krishtal <okrishtal@virtuozzo.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.
>>
>> 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 <okrishtal@virtuozzo.com>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> Reviewed-by: Yuri Pudgorodskiy <yur@virtuozzo.com>
>> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
>> CC: Stefan Weil <sw@weilnetz.de>
>> ---
>>   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

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

[Stefan Weil]

Hi,

please see my inline comments below.

Am 12.10.2015 um 19:38 schrieb Michael Roth:
> Quoting Denis V. Lunev (2015-10-07 05:59:34)
>> From: Olga Krishtal <okrishtal@virtuozzo.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.
>>
>> 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 <okrishtal@virtuozzo.com>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> Reviewed-by: Yuri Pudgorodskiy <yur@virtuozzo.com>
>> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
>> CC: Stefan Weil <sw@weilnetz.de>
>> ---
>>  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 */

... or an anonymous pipe.

>> +    if (GetNamedPipeHandleState(handle, &pipe_state, NULL,
>> +                                NULL, NULL, NULL, 0)) {
>> +        /* The fd is named pipe fd */

pipe_state could be declared here.

>> +        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;

The QEMU style still requires variable declarations at the beginning of
a code block.

>> +    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.

Yes.

> 
> 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.
> 
>>  }

I'm sorry that I cannot run any tests, but as far as I can see,
the code will be fine if all the notes are taken into account.

Regards
Stefan

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

[Denis V. Lunev]

On 10/12/2015 09:39 PM, Stefan Weil wrote:
> Hi,
>
> please see my inline comments below.
>
> Am 12.10.2015 um 19:38 schrieb Michael Roth:
>> Quoting Denis V. Lunev (2015-10-07 05:59:34)
>>> From: Olga Krishtal <okrishtal@virtuozzo.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.
>>>
>>> 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 <okrishtal@virtuozzo.com>
>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>> Reviewed-by: Yuri Pudgorodskiy <yur@virtuozzo.com>
>>> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
>>> CC: Stefan Weil <sw@weilnetz.de>
>>> ---
>>>   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 */
> ... or an anonymous pipe.
ok

>>> +    if (GetNamedPipeHandleState(handle, &pipe_state, NULL,
>>> +                                NULL, NULL, NULL, 0)) {
>>> +        /* The fd is named pipe fd */
> pipe_state could be declared here.

not ok. pipe_state is initialized in GetNamedPipeHandleState above,
which is in the main scope of the function


>>> +        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;
> The QEMU style still requires variable declarations at the beginning of
> a code block.
ok

>>> +    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.
> Yes.
sure :) already discussed


>> 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.
>>
>>>   }
> I'm sorry that I cannot run any tests, but as far as I can see,
> the code will be fine if all the notes are taken into account.
>
> Regards
> Stefan
>

Re: [Qemu-devel] [PATCH 0/2] qga: non-blocking fd cleanups

[Denis V. Lunev]

On 10/07/2015 01:59 PM, Denis V. Lunev wrote:
> 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.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Signed-off-by: Olga Krishtal <okrishtal@virtuozzo.com>
> Reviewed-by: Yuri Pudgorodskiy <yur@virtuozzo.com>
>
> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
ping

[Qemu-devel] [PATCH v2 0/2] 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 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>
Reviewed-by: Yuri Pudgorodskiy <yur@virtuozzo.com>
CC: Michael Roth <mdroth@linux.vnet.ibm.com>

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

[Denis V. Lunev]

From: Olga Krishtal <okrishtal@virtuozzo.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.

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.

Despite the fact that qemu_fd_register() is used only once here and now
it is difficult to drop this function as it uses AioContext internals
which we do not want to involve here.

Signed-off-by: Olga Krishtal <okrishtal@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: Yuri Pudgorodskiy <yur@virtuozzo.com>
CC: Michael Roth <mdroth@linux.vnet.ibm.com>
CC: Stefan Weil <sw@weilnetz.de>
---
 qga/commands-win32.c |  6 ++++++
 util/oslib-win32.c   | 57 ++++++++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 57 insertions(+), 6 deletions(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 41bdd3f..745bddf 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
@@ -156,6 +157,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..e1580c8 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -121,18 +121,63 @@ 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;
+    unsigned long opt = (unsigned long)nonblocking;
+
+    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 */
+    if (!nonblocking) {
+        WSAEventSelect(fd, NULL, 0);
+    }
     ioctlsocket(fd, FIONBIO, &opt);
+    if (nonblocking) {
+        qemu_fd_register(fd);
+    }
+    return;
+}
+
+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_fd_register(fd);
+    qemu_set_fd_nonblocking(fd, true);
 }
 
 int socket_set_fast_reuse(int fd)
-- 
2.1.4

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

[Michael Roth]

Quoting Denis V. Lunev (2015-10-15 07:51:11)
> From: Olga Krishtal <okrishtal@virtuozzo.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.
> 
> 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.
> 
> Despite the fact that qemu_fd_register() is used only once here and now
> it is difficult to drop this function as it uses AioContext internals
> which we do not want to involve here.
> 
> Signed-off-by: Olga Krishtal <okrishtal@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Reviewed-by: Yuri Pudgorodskiy <yur@virtuozzo.com>
> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
> CC: Stefan Weil <sw@weilnetz.de>
> ---
>  qga/commands-win32.c |  6 ++++++
>  util/oslib-win32.c   | 57 ++++++++++++++++++++++++++++++++++++++++++++++------
>  2 files changed, 57 insertions(+), 6 deletions(-)
> 
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 41bdd3f..745bddf 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
> @@ -156,6 +157,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));

This doesn't seem to build for me:

  CC    qga/channel-win32.o
  /home/mdroth/w/qemu3.git/qga/commands-win32.c: In function
  ‘qmp_guest_file_open’:
  /home/mdroth/w/qemu3.git/qga/commands-win32.c:165: warning:
  dereferencing ‘void *’ pointer
  /home/mdroth/w/qemu3.git/qga/commands-win32.c:165: error: request for
  member ‘_file’ in something not a structure or union

That's with ubuntu 14.04 and running:

  ../qemu3.git/configure --cross-prefix=i586-mingw32msvc-
  --target-list=x86_64-softmmu --extra-cflags=-Wall && make -j4

fileno() expects a FILE *, but fh is actually a HANDLE. Is there an
environment where this is not the case? I think what you want is
_open_osfhandle().

Note that it looks like you then might need to track the resulting fd and
use _close() in place of CloseHandle() in guest-file-close:

  https://msdn.microsoft.com/en-us/library/bdts1c9x.aspx

That also make me a bit concerned that read/write operations on the
HANDLE would then need to be replaced with read()/write() etc... the
documentation doesn't seem very clear on this.

I'm starting to think it best we just introduce a
qemu_set_nonblock_w32_handle(HANDLE ...) to do this, and leave the
existing qemu_set_nonblock() as it was before (winsock FDs only).

If people want to risk whatever wierd stuff might come about by
switching between HANDLE/fd they can use
_open_osfhandle()/_get_osfhandle() prior to calling
qemu_set_nonblock*(). But since we started off with a HANDLE in the
first place, there's no reason to us to switch to fd, then back to
HANDLE.

> +
>      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..e1580c8 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -121,18 +121,63 @@ 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;
> +    unsigned long opt = (unsigned long)nonblocking;
> +
> +    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 */
> +    if (!nonblocking) {
> +        WSAEventSelect(fd, NULL, 0);
> +    }
>      ioctlsocket(fd, FIONBIO, &opt);
> +    if (nonblocking) {
> +        qemu_fd_register(fd);
> +    }
> +    return;
> +}
> +
> +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_fd_register(fd);
> +    qemu_set_fd_nonblocking(fd, true);
>  }
> 
>  int socket_set_fast_reuse(int fd)
> -- 
> 2.1.4
>