Quoting Denis V. Lunev (2015-10-15 07:51:11) > From: Olga Krishtal <okrish...@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 <okrish...@virtuozzo.com> > Signed-off-by: Denis V. Lunev <d...@openvz.org> > Reviewed-by: Yuri Pudgorodskiy <y...@virtuozzo.com> > CC: Michael Roth <mdr...@linux.vnet.ibm.com> > CC: Stefan Weil <s...@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 >
