Re: [Qemu-devel] [PATCH 01/10] util, qga: drop guest_file_toggle_flags

["Denis V. Lunev" <den@openvz.org>]

On 30/06/15 13:25, Denis V. Lunev wrote:
> From: Olga Krishtal <okrishtal@virtuozzo.com>
>
> guest_file_toggle_flags is a copy from semi-portable qemu_set_nonblock.
> The latter is not working properly for Windows due to reduced Windows
> Posix implementation.
>
> 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 problem.
>
> Signed-off-by: Olga Krishtal <okrishtal@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Eric Blake <eblake@redhat.com>
> CC: Michael Roth <mdroth@linux.vnet.ibm.com>
Michael, Eric,

can you please consider merging of this patch. This is a
semi-independent cleanup which has sense on its own.

Den


> ---
>   qga/commands-posix.c | 27 ++-------------------------
>   util/oslib-win32.c   | 52 +++++++++++++++++++++++++++++++++++++++++++++++-----
>   2 files changed, 49 insertions(+), 30 deletions(-)
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index befd00b..40dbe25 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__
> @@ -376,27 +377,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)
>   {
> @@ -417,10 +397,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) {
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index 730a670..1a6ae72 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -119,17 +119,59 @@ struct tm *localtime_r(const time_t *timep, struct tm *result)
>       return p;
>   }
>   
> -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);
>   }
>