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

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(+)

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
>

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

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

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
>

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