| Message ID | 1457544504-8548-13-git-send-email-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 09/03/2016 18:28, Daniel P. Berrange wrote: > From: Paolo Bonzini <pbonzini@redhat.com> Reviewing my own patch looks weird. :) > On Win32 we cannot directly poll on socket handles. Instead we > create a Win32 event object and associate the socket handle with > the event. When the event signals readyness we then have to > use select to determine which events are ready. Creating Win32 > events is moderately heavyweight, so we don't want todo it > every time we create a GSource, so this associates a single > event with a QIOChannel. > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > --- > include/io/channel.h | 3 ++ > io/channel-socket.c | 9 ++++ > io/channel-watch.c | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++- > io/channel.c | 14 ++++++ > 4 files changed, 161 insertions(+), 1 deletion(-) > > diff --git a/include/io/channel.h b/include/io/channel.h > index 0a1f1ce..20b973a 100644 > --- a/include/io/channel.h > +++ b/include/io/channel.h > @@ -78,6 +78,9 @@ typedef gboolean (*QIOChannelFunc)(QIOChannel *ioc, > struct QIOChannel { > Object parent; > unsigned int features; /* bitmask of QIOChannelFeatures */ > +#ifdef _WIN32 > + HANDLE event; /* For use with GSource on Win23 */ Even s390 would have Win24 and Win31 but not Win23. :) > +#endif > }; > > /** > diff --git a/io/channel-socket.c b/io/channel-socket.c > index 6f7f594..ff49853 100644 > --- a/io/channel-socket.c > +++ b/io/channel-socket.c > @@ -55,6 +55,10 @@ qio_channel_socket_new(void) > ioc = QIO_CHANNEL(sioc); > ioc->features |= (1 << QIO_CHANNEL_FEATURE_SHUTDOWN); > > +#ifdef WIN32 > + ioc->event = CreateEvent(NULL, FALSE, FALSE, NULL); > +#endif > + > trace_qio_channel_socket_new(sioc); > > return sioc; > @@ -341,6 +345,11 @@ qio_channel_socket_accept(QIOChannelSocket *ioc, > cioc->remoteAddrLen = sizeof(ioc->remoteAddr); > cioc->localAddrLen = sizeof(ioc->localAddr); > > +#ifdef WIN32 > + ((QIOChannel *)cioc)->event = CreateEvent(NULL, FALSE, FALSE, NULL); > +#endif QIO_CHANNEL(cioc)->event? > + WSAEventSelect(ssource->socket, NULL, 0); This should probably be moved in qio_channel_socket_finalize. > > + /* WSAEnumNetworkEvents is edge-triggered, so we need a separate > + * call to select to find which events are actually available. > + * However, if there were no reported events at the time of the last > + * call, and no new events since then, we know that the socket is > + * quiescent. > + */ > + if (!ssource->revents && !ev.lNetworkEvents) { > + return 0; > + } > + This is unfortunately unsafe, because: 1) WSAEventSelect clears all pending events (so the next WSAEnumNetworkEvents returns no FD_READ, unless you have read all data in the buffer with recv) 2) setting a socket to non-blocking should call WSAEventSelect (see below), but cannot e.g. set ->revents to ~0 for all existing GSource. It's probably possible to add a generation count or something like that, but for now please revert this part (I had made it a separate commit in my prototype because I wasn't sure if it was okay). > + ssource->condition = condition; > + ssource->socket = socket; > + ssource->revents = 0; > + ssource->fd.fd = (gintptr)ioc->event; > + ssource->fd.events = G_IO_IN; > + > + g_source_add_poll(source, &ssource->fd); > + WSAEventSelect(ssource->socket, ioc->event, > + FD_READ | FD_ACCEPT | FD_CLOSE | > + FD_CONNECT | FD_WRITE | FD_OOB); This should be moved where the socket is made non-blocking in qio_channel_socket_set_blocking (because qemu_qemu_set_nonblock also calls WSAEventSelect). It's probably worth adding a comment that qio_channel_socket_source_check only works in non-blocking mode for Windows. Thanks, Paolo
On 03/09/2016 10:47 AM, Paolo Bonzini wrote: > On 09/03/2016 18:28, Daniel P. Berrange wrote: >> From: Paolo Bonzini <pbonzini@redhat.com> > > Reviewing my own patch looks weird. :) > >> On Win32 we cannot directly poll on socket handles. Instead we >> create a Win32 event object and associate the socket handle with >> the event. When the event signals readyness we then have to >> use select to determine which events are ready. Creating Win32 >> events is moderately heavyweight, so we don't want todo it >> every time we create a GSource, so this associates a single >> event with a QIOChannel. >> >> Signed-off-by: Daniel P. Berrange <berrange@redhat.com> >> --- Especially when it lacks your S-o-b :)
On 09/03/2016 20:59, Eric Blake wrote: > On 03/09/2016 10:47 AM, Paolo Bonzini wrote: >> > On 09/03/2016 18:28, Daniel P. Berrange wrote: >>> >> From: Paolo Bonzini <pbonzini@redhat.com> >> > >> > Reviewing my own patch looks weird. :) >> > >>> >> On Win32 we cannot directly poll on socket handles. Instead we >>> >> create a Win32 event object and associate the socket handle with >>> >> the event. When the event signals readyness we then have to >>> >> use select to determine which events are ready. Creating Win32 >>> >> events is moderately heavyweight, so we don't want todo it >>> >> every time we create a GSource, so this associates a single >>> >> event with a QIOChannel. >>> >> >>> >> Signed-off-by: Daniel P. Berrange <berrange@redhat.com> >>> >> --- > Especially when it lacks your S-o-b :) I'm innocent! :) https://github.com/bonzini/qemu/commit/win32-qio-watch^ Paolo
On Wed, Mar 09, 2016 at 10:24:51PM +0100, Paolo Bonzini wrote: > > > On 09/03/2016 20:59, Eric Blake wrote: > > On 03/09/2016 10:47 AM, Paolo Bonzini wrote: > >> > On 09/03/2016 18:28, Daniel P. Berrange wrote: > >>> >> From: Paolo Bonzini <pbonzini@redhat.com> > >> > > >> > Reviewing my own patch looks weird. :) > >> > > >>> >> On Win32 we cannot directly poll on socket handles. Instead we > >>> >> create a Win32 event object and associate the socket handle with > >>> >> the event. When the event signals readyness we then have to > >>> >> use select to determine which events are ready. Creating Win32 > >>> >> events is moderately heavyweight, so we don't want todo it > >>> >> every time we create a GSource, so this associates a single > >>> >> event with a QIOChannel. > >>> >> > >>> >> Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > >>> >> --- > > Especially when it lacks your S-o-b :) > > I'm innocent! :) > > https://github.com/bonzini/qemu/commit/win32-qio-watch^ Since I made non-trivial changes to this commit, I thought it was corrrect to remove your S-o-b, to avoid claiming that you'd already signed off on the changes I made. Was that not the right thing todo ? Regards, Daniel
On 10/03/2016 10:41, Daniel P. Berrange wrote: >> > >> > https://github.com/bonzini/qemu/commit/win32-qio-watch^ > Since I made non-trivial changes to this commit, I thought it was > corrrect to remove your S-o-b, to avoid claiming that you'd already > signed off on the changes I made. Was that not the right thing > todo ? You should have then also removed the authorship. I think in this case leaving the author and the s-o-b was the right thing to do, followed by removing the authorship and leaving the s-o-b. Signed-off-by is more of a legal thing than a "I think that these changes are good for QEMU". Paolo
On Wed, Mar 09, 2016 at 06:47:29PM +0100, Paolo Bonzini wrote: > On 09/03/2016 18:28, Daniel P. Berrange wrote: > > From: Paolo Bonzini <pbonzini@redhat.com> > > Reviewing my own patch looks weird. :) > > > On Win32 we cannot directly poll on socket handles. Instead we > > create a Win32 event object and associate the socket handle with > > the event. When the event signals readyness we then have to > > use select to determine which events are ready. Creating Win32 > > events is moderately heavyweight, so we don't want todo it > > every time we create a GSource, so this associates a single > > event with a QIOChannel. > > > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > > --- > > include/io/channel.h | 3 ++ > > io/channel-socket.c | 9 ++++ > > io/channel-watch.c | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++- > > io/channel.c | 14 ++++++ > > 4 files changed, 161 insertions(+), 1 deletion(-) > > diff --git a/io/channel-socket.c b/io/channel-socket.c > > index 6f7f594..ff49853 100644 > > --- a/io/channel-socket.c > > +++ b/io/channel-socket.c > > @@ -55,6 +55,10 @@ qio_channel_socket_new(void) > > ioc = QIO_CHANNEL(sioc); > > ioc->features |= (1 << QIO_CHANNEL_FEATURE_SHUTDOWN); > > > > +#ifdef WIN32 > > + ioc->event = CreateEvent(NULL, FALSE, FALSE, NULL); > > +#endif > > + > > trace_qio_channel_socket_new(sioc); > > > > return sioc; > > @@ -341,6 +345,11 @@ qio_channel_socket_accept(QIOChannelSocket *ioc, > > cioc->remoteAddrLen = sizeof(ioc->remoteAddr); > > cioc->localAddrLen = sizeof(ioc->localAddr); > > > > +#ifdef WIN32 > > + ((QIOChannel *)cioc)->event = CreateEvent(NULL, FALSE, FALSE, NULL); > > +#endif > > QIO_CHANNEL(cioc)->event? > > > + WSAEventSelect(ssource->socket, NULL, 0); > > This should probably be moved in qio_channel_socket_finalize. Yes, moved that. > > + /* WSAEnumNetworkEvents is edge-triggered, so we need a separate > > + * call to select to find which events are actually available. > > + * However, if there were no reported events at the time of the last > > + * call, and no new events since then, we know that the socket is > > + * quiescent. > > + */ > > + if (!ssource->revents && !ev.lNetworkEvents) { > > + return 0; > > + } > > + > > This is unfortunately unsafe, because: > > 1) WSAEventSelect clears all pending events (so the next > WSAEnumNetworkEvents returns no FD_READ, unless you have read all data > in the buffer with recv) > > 2) setting a socket to non-blocking should call WSAEventSelect (see > below), but cannot e.g. set ->revents to ~0 for all existing GSource. > > It's probably possible to add a generation count or something like that, > but for now please revert this part (I had made it a separate commit in > my prototype because I wasn't sure if it was okay). Ok, removing this chunk > > > + ssource->condition = condition; > > + ssource->socket = socket; > > + ssource->revents = 0; > > + ssource->fd.fd = (gintptr)ioc->event; > > + ssource->fd.events = G_IO_IN; > > + > > + g_source_add_poll(source, &ssource->fd); > > + WSAEventSelect(ssource->socket, ioc->event, > > + FD_READ | FD_ACCEPT | FD_CLOSE | > > + FD_CONNECT | FD_WRITE | FD_OOB); > > This should be moved where the socket is made non-blocking in > qio_channel_socket_set_blocking (because qemu_qemu_set_nonblock also > calls WSAEventSelect). Ok, I've moved that too. > It's probably worth adding a comment that > qio_channel_socket_source_check only works in non-blocking mode for Windows. And added this. Regards, Daniel
On 03/10/2016 02:54 AM, Paolo Bonzini wrote: > > > On 10/03/2016 10:41, Daniel P. Berrange wrote: >>>> >>>> https://github.com/bonzini/qemu/commit/win32-qio-watch^ >> Since I made non-trivial changes to this commit, I thought it was >> corrrect to remove your S-o-b, to avoid claiming that you'd already >> signed off on the changes I made. Was that not the right thing >> todo ? > > You should have then also removed the authorship. I think in this case > leaving the author and the s-o-b was the right thing to do, followed by > removing the authorship and leaving the s-o-b. Signed-off-by is more of > a legal thing than a "I think that these changes are good for QEMU". I've seen this pattern of keeping original authorship and S-o-b, coupled with an extension to the commit message, in the qemu.git history, something like: original title original message S-o-b: original [make the following additional changes] S-o-b: second author
diff --git a/include/io/channel.h b/include/io/channel.h index 0a1f1ce..20b973a 100644 --- a/include/io/channel.h +++ b/include/io/channel.h @@ -78,6 +78,9 @@ typedef gboolean (*QIOChannelFunc)(QIOChannel *ioc, struct QIOChannel { Object parent; unsigned int features; /* bitmask of QIOChannelFeatures */ +#ifdef _WIN32 + HANDLE event; /* For use with GSource on Win23 */ +#endif }; /** diff --git a/io/channel-socket.c b/io/channel-socket.c index 6f7f594..ff49853 100644 --- a/io/channel-socket.c +++ b/io/channel-socket.c @@ -55,6 +55,10 @@ qio_channel_socket_new(void) ioc = QIO_CHANNEL(sioc); ioc->features |= (1 << QIO_CHANNEL_FEATURE_SHUTDOWN); +#ifdef WIN32 + ioc->event = CreateEvent(NULL, FALSE, FALSE, NULL); +#endif + trace_qio_channel_socket_new(sioc); return sioc; @@ -341,6 +345,11 @@ qio_channel_socket_accept(QIOChannelSocket *ioc, cioc->remoteAddrLen = sizeof(ioc->remoteAddr); cioc->localAddrLen = sizeof(ioc->localAddr); +#ifdef WIN32 + ((QIOChannel *)cioc)->event = CreateEvent(NULL, FALSE, FALSE, NULL); +#endif + + retry: trace_qio_channel_socket_accept(ioc); cioc->fd = accept(ioc->fd, (struct sockaddr *)&cioc->remoteAddr, diff --git a/io/channel-watch.c b/io/channel-watch.c index dfac8f8..c5ee69f 100644 --- a/io/channel-watch.c +++ b/io/channel-watch.c @@ -30,6 +30,20 @@ struct QIOChannelFDSource { }; +#ifdef CONFIG_WIN32 +typedef struct QIOChannelSocketSource QIOChannelSocketSource; +struct QIOChannelSocketSource { + GSource parent; + GPollFD fd; + QIOChannel *ioc; + SOCKET socket; + int revents; + GIOCondition condition; +}; + +#endif + + typedef struct QIOChannelFDPairSource QIOChannelFDPairSource; struct QIOChannelFDPairSource { GSource parent; @@ -82,6 +96,104 @@ qio_channel_fd_source_finalize(GSource *source) } +#ifdef CONFIG_WIN32 +static gboolean +qio_channel_socket_source_prepare(GSource *source G_GNUC_UNUSED, + gint *timeout) +{ + *timeout = -1; + + return FALSE; +} + + +static gboolean +qio_channel_socket_source_check(GSource *source) +{ + static struct timeval tv0; + + QIOChannelSocketSource *ssource = (QIOChannelSocketSource *)source; + WSANETWORKEVENTS ev; + fd_set rfds, wfds, xfds; + + if (!ssource->condition) { + return 0; + } + + WSAEnumNetworkEvents(ssource->socket, ssource->ioc->event, &ev); + + /* WSAEnumNetworkEvents is edge-triggered, so we need a separate + * call to select to find which events are actually available. + * However, if there were no reported events at the time of the last + * call, and no new events since then, we know that the socket is + * quiescent. + */ + if (!ssource->revents && !ev.lNetworkEvents) { + return 0; + } + + FD_ZERO(&rfds); + FD_ZERO(&wfds); + FD_ZERO(&xfds); + if (ssource->condition & G_IO_IN) { + FD_SET((SOCKET)ssource->socket, &rfds); + } + if (ssource->condition & G_IO_OUT) { + FD_SET((SOCKET)ssource->socket, &wfds); + } + if (ssource->condition & G_IO_PRI) { + FD_SET((SOCKET)ssource->socket, &xfds); + } + ssource->revents = 0; + if (select(0, &rfds, &wfds, &xfds, &tv0) == 0) { + return 0; + } + + if (FD_ISSET(ssource->socket, &rfds)) { + ssource->revents |= G_IO_IN; + } + if (FD_ISSET(ssource->socket, &wfds)) { + ssource->revents |= G_IO_OUT; + } + if (FD_ISSET(ssource->socket, &xfds)) { + ssource->revents |= G_IO_PRI; + } + + return ssource->revents; +} + + +static gboolean +qio_channel_socket_source_dispatch(GSource *source, + GSourceFunc callback, + gpointer user_data) +{ + QIOChannelFunc func = (QIOChannelFunc)callback; + QIOChannelSocketSource *ssource = (QIOChannelSocketSource *)source; + + return (*func)(ssource->ioc, ssource->revents, user_data); +} + + +static void +qio_channel_socket_source_finalize(GSource *source) +{ + QIOChannelSocketSource *ssource = (QIOChannelSocketSource *)source; + + WSAEventSelect(ssource->socket, NULL, 0); + object_unref(OBJECT(ssource->ioc)); +} + + +GSourceFuncs qio_channel_socket_source_funcs = { + qio_channel_socket_source_prepare, + qio_channel_socket_source_check, + qio_channel_socket_source_dispatch, + qio_channel_socket_source_finalize +}; +#endif + + static gboolean qio_channel_fd_pair_source_prepare(GSource *source G_GNUC_UNUSED, gint *timeout) @@ -177,7 +289,29 @@ GSource *qio_channel_create_socket_watch(QIOChannel *ioc, int socket, GIOCondition condition) { - abort(); + GSource *source; + QIOChannelSocketSource *ssource; + + source = g_source_new(&qio_channel_socket_source_funcs, + sizeof(QIOChannelSocketSource)); + ssource = (QIOChannelSocketSource *)source; + + ssource->ioc = ioc; + object_ref(OBJECT(ioc)); + + ssource->condition = condition; + ssource->socket = socket; + ssource->revents = 0; + + ssource->fd.fd = (gintptr)ioc->event; + ssource->fd.events = G_IO_IN; + + g_source_add_poll(source, &ssource->fd); + WSAEventSelect(ssource->socket, ioc->event, + FD_READ | FD_ACCEPT | FD_CLOSE | + FD_CONNECT | FD_WRITE | FD_OOB); + + return source; } #else GSource *qio_channel_create_socket_watch(QIOChannel *ioc, diff --git a/io/channel.c b/io/channel.c index 3fc09f8..dd6fc0e 100644 --- a/io/channel.c +++ b/io/channel.c @@ -274,10 +274,24 @@ void qio_channel_wait(QIOChannel *ioc, } +#ifdef _WIN32 +static void qio_channel_finalize(Object *obj) +{ + QIOChannel *ioc = QIO_CHANNEL(obj); + + if (ioc->event) { + CloseHandle(ioc->event); + } +} +#endif + static const TypeInfo qio_channel_info = { .parent = TYPE_OBJECT, .name = TYPE_QIO_CHANNEL, .instance_size = sizeof(QIOChannel), +#ifdef _WIN32 + .instance_finalize = qio_channel_finalize, +#endif .abstract = true, .class_size = sizeof(QIOChannelClass), };