[libvirt PATCH v2 33/56] src: introduce helper API for creating GSource for socket

Daniel P. Berrangé posted 56 patches 6 years ago
There is a newer version of this series
[libvirt PATCH v2 33/56] src: introduce helper API for creating GSource for socket
Posted by Daniel P. Berrangé 6 years ago
We need to be able to create event loop watches using the
GSource API for sockets. GIOChannel is able todo this, but
we don't want to use the GIOChannel APIs for reading/writing,
and testing shows just using its GSource APIs is unreliable
on Windows.

This patch thus creates a standalone helper API for creating
a GSource for a socket file descriptor. This impl is derived
from code in QEMU's io/channel-watch.c file that was written
by myself & Paolo Bonzini & thus under Red Hat copyright.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 build-aux/syntax-check.mk    |   3 +
 src/util/Makefile.inc.am     |   2 +
 src/util/vireventglibwatch.c | 248 +++++++++++++++++++++++++++++++++++
 src/util/vireventglibwatch.h |  48 +++++++
 4 files changed, 301 insertions(+)
 create mode 100644 src/util/vireventglibwatch.c
 create mode 100644 src/util/vireventglibwatch.h

diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk
index 376dc7c30d..b93052b1c0 100644
--- a/build-aux/syntax-check.mk
+++ b/build-aux/syntax-check.mk
@@ -2339,3 +2339,6 @@ exclude_file_name_regexp--sc_prohibit_backslash_alignment = \
 
 exclude_file_name_regexp--sc_prohibit_always_true_header_tests = \
   ^src/util/(virfile|virnetdev|virnetdevip)\.[c,h]|$$
+
+exclude_file_name_regexp--sc_prohibit_select = \
+  ^build-aux/syntax-check\.mk|src/util/vireventglibwatch\.c$$
diff --git a/src/util/Makefile.inc.am b/src/util/Makefile.inc.am
index 528c9f6cfe..faa6799e7d 100644
--- a/src/util/Makefile.inc.am
+++ b/src/util/Makefile.inc.am
@@ -63,6 +63,8 @@ UTIL_SOURCES = \
 	util/virerrorpriv.h \
 	util/virevent.c \
 	util/virevent.h \
+	util/vireventglibwatch.c \
+	util/vireventglibwatch.h \
 	util/vireventpoll.c \
 	util/vireventpoll.h \
 	util/virfcp.c \
diff --git a/src/util/vireventglibwatch.c b/src/util/vireventglibwatch.c
new file mode 100644
index 0000000000..f7b087e2ec
--- /dev/null
+++ b/src/util/vireventglibwatch.c
@@ -0,0 +1,248 @@
+/*
+ * vireventglibwatch.c: GSource impl for sockets
+ *
+ * Copyright (C) 2015-2020 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see
+ * <http://www.gnu.org/licenses/>.
+ */
+
+#include <config.h>
+
+#include "vireventglibwatch.h"
+
+#ifndef WIN32
+typedef struct virEventGLibFDSource virEventGLibFDSource;
+struct virEventGLibFDSource {
+    GSource parent;
+    GPollFD pollfd;
+    int fd;
+    GIOCondition condition;
+};
+
+
+static gboolean
+virEventGLibFDSourcePrepare(GSource *source G_GNUC_UNUSED,
+                            gint *timeout)
+{
+    *timeout = -1;
+
+    return FALSE;
+}
+
+
+static gboolean
+virEventGLibFDSourceCheck(GSource *source)
+{
+    virEventGLibFDSource *ssource = (virEventGLibFDSource *)source;
+
+    return ssource->pollfd.revents & ssource->condition;
+}
+
+
+static gboolean
+virEventGLibFDSourceDispatch(GSource *source,
+                             GSourceFunc callback,
+                             gpointer user_data)
+{
+    virEventGLibSocketFunc func = (virEventGLibSocketFunc)callback;
+    virEventGLibFDSource *ssource = (virEventGLibFDSource *)source;
+
+    return (*func)(ssource->fd,
+                   ssource->pollfd.revents & ssource->condition,
+                   user_data);
+}
+
+
+static void
+virEventGLibFDSourceFinalize(GSource *source G_GNUC_UNUSED)
+{
+}
+
+
+GSourceFuncs virEventGLibFDSourceFuncs = {
+    .prepare = virEventGLibFDSourcePrepare,
+    .check = virEventGLibFDSourceCheck,
+    .dispatch = virEventGLibFDSourceDispatch,
+    .finalize = virEventGLibFDSourceFinalize
+};
+
+
+GSource *virEventGLibCreateSocketWatch(int fd,
+                                       GIOCondition condition)
+{
+    GSource *source;
+    virEventGLibFDSource *ssource;
+
+    source = g_source_new(&virEventGLibFDSourceFuncs,
+                          sizeof(virEventGLibFDSource));
+    ssource = (virEventGLibFDSource *)source;
+
+    ssource->condition = condition;
+    ssource->fd = fd;
+
+    ssource->pollfd.fd = fd;
+    ssource->pollfd.events = condition;
+
+    g_source_add_poll(source, &ssource->pollfd);
+
+    return source;
+}
+
+#else /* WIN32 */
+
+# define WIN32_LEAN_AND_MEAN
+# include <winsock2.h>
+
+typedef struct virEventGLibSocketSource virEventGLibSocketSource;
+struct virEventGLibSocketSource {
+    GSource parent;
+    GPollFD pollfd;
+    int fd;
+    SOCKET socket;
+    HANDLE event;
+    int revents;
+    GIOCondition condition;
+};
+
+
+static gboolean
+virEventGLibSocketSourcePrepare(GSource *source G_GNUC_UNUSED,
+                                gint *timeout)
+{
+    *timeout = -1;
+
+    return FALSE;
+}
+
+
+/*
+ * NB, this impl only works when the socket is in non-blocking
+ * mode on Win32
+ */
+static gboolean
+virEventGLibSocketSourceCheck(GSource *source)
+{
+    static struct timeval tv0;
+
+    virEventGLibSocketSource *ssource = (virEventGLibSocketSource *)source;
+    WSANETWORKEVENTS ev;
+    fd_set rfds, wfds, xfds;
+
+    if (!ssource->condition)
+        return 0;
+
+    WSAEnumNetworkEvents(ssource->socket, ssource->event, &ev);
+
+    FD_ZERO(&rfds);
+    FD_ZERO(&wfds);
+    FD_ZERO(&xfds);
+    if (ssource->condition & G_IO_IN)
+        FD_SET(ssource->socket, &rfds);
+    if (ssource->condition & G_IO_OUT)
+        FD_SET(ssource->socket, &wfds);
+    if (ssource->condition & G_IO_PRI)
+        FD_SET(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
+virEventGLibSocketSourceDispatch(GSource *source,
+                                 GSourceFunc callback,
+                                 gpointer user_data)
+{
+    virEventGLibSocketFunc func = (virEventGLibSocketFunc)callback;
+    virEventGLibSocketSource *ssource = (virEventGLibSocketSource *)source;
+
+    return (*func)(ssource->fd, ssource->revents, user_data);
+}
+
+
+static void
+virEventGLibSocketSourceFinalize(GSource *source)
+{
+    virEventGLibSocketSource *ssource = (virEventGLibSocketSource *)source;
+
+    WSAEventSelect(ssource->socket, NULL, 0);
+}
+
+
+GSourceFuncs virEventGLibSocketSourceFuncs = {
+    .prepare = virEventGLibSocketSourcePrepare,
+    .check = virEventGLibSocketSourceCheck,
+    .dispatch = virEventGLibSocketSourceDispatch,
+    .finalize = virEventGLibSocketSourceFinalize
+};
+
+
+GSource *virEventGLibCreateSocketWatch(int fd,
+                                       GIOCondition condition)
+{
+    GSource *source;
+    virEventGLibSocketSource *ssource;
+
+    source = g_source_new(&virEventGLibSocketSourceFuncs,
+                          sizeof(virEventGLibSocketSource));
+    ssource = (virEventGLibSocketSource *)source;
+
+    ssource->condition = condition;
+    ssource->fd = fd;
+    ssource->socket = _get_osfhandle(fd);
+    ssource->event = CreateEvent(NULL, FALSE, FALSE, NULL);
+    ssource->revents = 0;
+
+    ssource->pollfd.fd = (gintptr)ssource->event;
+    ssource->pollfd.events = G_IO_IN;
+
+    WSAEventSelect(ssource->socket, ssource->event,
+                   FD_READ | FD_ACCEPT | FD_CLOSE |
+                   FD_CONNECT | FD_WRITE | FD_OOB);
+
+    g_source_add_poll(source, &ssource->pollfd);
+
+    return source;
+}
+
+#endif /* WIN32 */
+
+
+guint virEventGLibAddSocketWatch(int fd,
+                                 GIOCondition condition,
+                                 GMainContext *context,
+                                 virEventGLibSocketFunc func,
+                                 gpointer opaque,
+                                 GDestroyNotify notify)
+{
+    g_autoptr(GSource) source = NULL;
+
+    source = virEventGLibCreateSocketWatch(fd, condition);
+    g_source_set_callback(source, (GSourceFunc)func, opaque, notify);
+
+    return g_source_attach(source, context);
+}
diff --git a/src/util/vireventglibwatch.h b/src/util/vireventglibwatch.h
new file mode 100644
index 0000000000..2f7e61cfba
--- /dev/null
+++ b/src/util/vireventglibwatch.h
@@ -0,0 +1,48 @@
+/*
+ * vireventglibwatch.h: GSource impl for sockets
+ *
+ * Copyright (C) 2015-2020 Red Hat, Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see
+ * <http://www.gnu.org/licenses/>.
+ */
+
+#pragma once
+
+#include "internal.h"
+
+/**
+ * virEventGLibCreateSocketWatch:
+ * @fd: the file descriptor
+ * @condition: the I/O condition
+ *
+ * Create a new main loop source that is able to
+ * monitor the file descriptor @fd for the
+ * I/O conditions in @condition.
+ *
+ * Returns: the new main loop source
+ */
+GSource *virEventGLibCreateSocketWatch(int fd,
+                                       GIOCondition condition);
+
+typedef gboolean (*virEventGLibSocketFunc)(int fd,
+                                           GIOCondition condition,
+                                           gpointer data);
+
+guint virEventGLibAddSocketWatch(int fd,
+                                 GIOCondition condition,
+                                 GMainContext *context,
+                                 virEventGLibSocketFunc func,
+                                 gpointer opaque,
+                                 GDestroyNotify notify);
-- 
2.24.1

Re: [libvirt PATCH v2 33/56] src: introduce helper API for creating GSource for socket
Posted by Pavel Hrdina 6 years ago
On Tue, Jan 28, 2020 at 01:11:14PM +0000, Daniel P. Berrangé wrote:
> We need to be able to create event loop watches using the
> GSource API for sockets. GIOChannel is able todo this, but
> we don't want to use the GIOChannel APIs for reading/writing,
> and testing shows just using its GSource APIs is unreliable
> on Windows.
> 
> This patch thus creates a standalone helper API for creating
> a GSource for a socket file descriptor. This impl is derived
> from code in QEMU's io/channel-watch.c file that was written
> by myself & Paolo Bonzini & thus under Red Hat copyright.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  build-aux/syntax-check.mk    |   3 +
>  src/util/Makefile.inc.am     |   2 +
>  src/util/vireventglibwatch.c | 248 +++++++++++++++++++++++++++++++++++
>  src/util/vireventglibwatch.h |  48 +++++++
>  4 files changed, 301 insertions(+)
>  create mode 100644 src/util/vireventglibwatch.c
>  create mode 100644 src/util/vireventglibwatch.h
> 
> diff --git a/src/util/vireventglibwatch.c b/src/util/vireventglibwatch.c
> new file mode 100644
> index 0000000000..f7b087e2ec
> --- /dev/null
> +++ b/src/util/vireventglibwatch.c
> @@ -0,0 +1,248 @@
> +/*
> + * vireventglibwatch.c: GSource impl for sockets
> + *
> + * Copyright (C) 2015-2020 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library. If not, see
> + * <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <config.h>
> +
> +#include "vireventglibwatch.h"
> +
> +#ifndef WIN32
> +typedef struct virEventGLibFDSource virEventGLibFDSource;
> +struct virEventGLibFDSource {
> +    GSource parent;
> +    GPollFD pollfd;
> +    int fd;
> +    GIOCondition condition;
> +};
> +
> +
> +static gboolean
> +virEventGLibFDSourcePrepare(GSource *source G_GNUC_UNUSED,
> +                            gint *timeout)
> +{
> +    *timeout = -1;
> +
> +    return FALSE;
> +}
> +
> +
> +static gboolean
> +virEventGLibFDSourceCheck(GSource *source)
> +{
> +    virEventGLibFDSource *ssource = (virEventGLibFDSource *)source;
> +
> +    return ssource->pollfd.revents & ssource->condition;
> +}
> +
> +
> +static gboolean
> +virEventGLibFDSourceDispatch(GSource *source,
> +                             GSourceFunc callback,
> +                             gpointer user_data)
> +{
> +    virEventGLibSocketFunc func = (virEventGLibSocketFunc)callback;
> +    virEventGLibFDSource *ssource = (virEventGLibFDSource *)source;
> +
> +    return (*func)(ssource->fd,
> +                   ssource->pollfd.revents & ssource->condition,
> +                   user_data);
> +}
> +
> +
> +static void
> +virEventGLibFDSourceFinalize(GSource *source G_GNUC_UNUSED)
> +{
> +}
> +
> +
> +GSourceFuncs virEventGLibFDSourceFuncs = {
> +    .prepare = virEventGLibFDSourcePrepare,
> +    .check = virEventGLibFDSourceCheck,
> +    .dispatch = virEventGLibFDSourceDispatch,
> +    .finalize = virEventGLibFDSourceFinalize
> +};
> +
> +
> +GSource *virEventGLibCreateSocketWatch(int fd,
> +                                       GIOCondition condition)
> +{
> +    GSource *source;
> +    virEventGLibFDSource *ssource;
> +
> +    source = g_source_new(&virEventGLibFDSourceFuncs,
> +                          sizeof(virEventGLibFDSource));
> +    ssource = (virEventGLibFDSource *)source;
> +
> +    ssource->condition = condition;
> +    ssource->fd = fd;
> +
> +    ssource->pollfd.fd = fd;
> +    ssource->pollfd.events = condition;
> +
> +    g_source_add_poll(source, &ssource->pollfd);
> +
> +    return source;
> +}
> +
> +#else /* WIN32 */
> +
> +# define WIN32_LEAN_AND_MEAN
> +# include <winsock2.h>
> +
> +typedef struct virEventGLibSocketSource virEventGLibSocketSource;
> +struct virEventGLibSocketSource {
> +    GSource parent;
> +    GPollFD pollfd;
> +    int fd;
> +    SOCKET socket;
> +    HANDLE event;
> +    int revents;
> +    GIOCondition condition;
> +};
> +
> +
> +static gboolean
> +virEventGLibSocketSourcePrepare(GSource *source G_GNUC_UNUSED,
> +                                gint *timeout)
> +{
> +    *timeout = -1;
> +
> +    return FALSE;
> +}
> +
> +
> +/*
> + * NB, this impl only works when the socket is in non-blocking
> + * mode on Win32
> + */
> +static gboolean
> +virEventGLibSocketSourceCheck(GSource *source)
> +{
> +    static struct timeval tv0;
> +
> +    virEventGLibSocketSource *ssource = (virEventGLibSocketSource *)source;
> +    WSANETWORKEVENTS ev;
> +    fd_set rfds, wfds, xfds;
> +
> +    if (!ssource->condition)
> +        return 0;
> +
> +    WSAEnumNetworkEvents(ssource->socket, ssource->event, &ev);
> +
> +    FD_ZERO(&rfds);
> +    FD_ZERO(&wfds);
> +    FD_ZERO(&xfds);
> +    if (ssource->condition & G_IO_IN)
> +        FD_SET(ssource->socket, &rfds);
> +    if (ssource->condition & G_IO_OUT)
> +        FD_SET(ssource->socket, &wfds);
> +    if (ssource->condition & G_IO_PRI)
> +        FD_SET(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
> +virEventGLibSocketSourceDispatch(GSource *source,
> +                                 GSourceFunc callback,
> +                                 gpointer user_data)
> +{
> +    virEventGLibSocketFunc func = (virEventGLibSocketFunc)callback;
> +    virEventGLibSocketSource *ssource = (virEventGLibSocketSource *)source;
> +
> +    return (*func)(ssource->fd, ssource->revents, user_data);
> +}
> +
> +
> +static void
> +virEventGLibSocketSourceFinalize(GSource *source)
> +{
> +    virEventGLibSocketSource *ssource = (virEventGLibSocketSource *)source;
> +
> +    WSAEventSelect(ssource->socket, NULL, 0);
> +}
> +
> +
> +GSourceFuncs virEventGLibSocketSourceFuncs = {
> +    .prepare = virEventGLibSocketSourcePrepare,
> +    .check = virEventGLibSocketSourceCheck,
> +    .dispatch = virEventGLibSocketSourceDispatch,
> +    .finalize = virEventGLibSocketSourceFinalize
> +};
> +
> +
> +GSource *virEventGLibCreateSocketWatch(int fd,
> +                                       GIOCondition condition)
> +{
> +    GSource *source;
> +    virEventGLibSocketSource *ssource;
> +
> +    source = g_source_new(&virEventGLibSocketSourceFuncs,
> +                          sizeof(virEventGLibSocketSource));
> +    ssource = (virEventGLibSocketSource *)source;
> +
> +    ssource->condition = condition;
> +    ssource->fd = fd;
> +    ssource->socket = _get_osfhandle(fd);
> +    ssource->event = CreateEvent(NULL, FALSE, FALSE, NULL);

Based on going through QEMU code and windows documentation it looks
like we have to free the event using CloseHandle().

QEMU is doing it in io/channel.c in qio_channel_finalize() which is
probably called by unrefing ssource->ioc in
qio_channel_fd_pair_source_finalize().

That would mean we have to call CloseHandle() in
virEventGLibSocketSourceFinalize() to make sure we will not leak any
resources.

Otherwise looks good.

Pavel
Re: [libvirt PATCH v2 33/56] src: introduce helper API for creating GSource for socket
Posted by Daniel P. Berrangé 6 years ago
On Wed, Jan 29, 2020 at 05:33:20PM +0100, Pavel Hrdina wrote:
> On Tue, Jan 28, 2020 at 01:11:14PM +0000, Daniel P. Berrangé wrote:
> > We need to be able to create event loop watches using the
> > GSource API for sockets. GIOChannel is able todo this, but
> > we don't want to use the GIOChannel APIs for reading/writing,
> > and testing shows just using its GSource APIs is unreliable
> > on Windows.
> > 
> > This patch thus creates a standalone helper API for creating
> > a GSource for a socket file descriptor. This impl is derived
> > from code in QEMU's io/channel-watch.c file that was written
> > by myself & Paolo Bonzini & thus under Red Hat copyright.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  build-aux/syntax-check.mk    |   3 +
> >  src/util/Makefile.inc.am     |   2 +
> >  src/util/vireventglibwatch.c | 248 +++++++++++++++++++++++++++++++++++
> >  src/util/vireventglibwatch.h |  48 +++++++
> >  4 files changed, 301 insertions(+)
> >  create mode 100644 src/util/vireventglibwatch.c
> >  create mode 100644 src/util/vireventglibwatch.h
> > 
> > diff --git a/src/util/vireventglibwatch.c b/src/util/vireventglibwatch.c
> > new file mode 100644
> > index 0000000000..f7b087e2ec
> > --- /dev/null
> > +++ b/src/util/vireventglibwatch.c
> > @@ -0,0 +1,248 @@
> > +/*
> > + * vireventglibwatch.c: GSource impl for sockets
> > + *
> > + * Copyright (C) 2015-2020 Red Hat, Inc.
> > + *
> > + * This library is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2.1 of the License, or (at your option) any later version.
> > + *
> > + * This library is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with this library. If not, see
> > + * <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include <config.h>
> > +
> > +#include "vireventglibwatch.h"
> > +
> > +#ifndef WIN32
> > +typedef struct virEventGLibFDSource virEventGLibFDSource;
> > +struct virEventGLibFDSource {
> > +    GSource parent;
> > +    GPollFD pollfd;
> > +    int fd;
> > +    GIOCondition condition;
> > +};
> > +
> > +
> > +static gboolean
> > +virEventGLibFDSourcePrepare(GSource *source G_GNUC_UNUSED,
> > +                            gint *timeout)
> > +{
> > +    *timeout = -1;
> > +
> > +    return FALSE;
> > +}
> > +
> > +
> > +static gboolean
> > +virEventGLibFDSourceCheck(GSource *source)
> > +{
> > +    virEventGLibFDSource *ssource = (virEventGLibFDSource *)source;
> > +
> > +    return ssource->pollfd.revents & ssource->condition;
> > +}
> > +
> > +
> > +static gboolean
> > +virEventGLibFDSourceDispatch(GSource *source,
> > +                             GSourceFunc callback,
> > +                             gpointer user_data)
> > +{
> > +    virEventGLibSocketFunc func = (virEventGLibSocketFunc)callback;
> > +    virEventGLibFDSource *ssource = (virEventGLibFDSource *)source;
> > +
> > +    return (*func)(ssource->fd,
> > +                   ssource->pollfd.revents & ssource->condition,
> > +                   user_data);
> > +}
> > +
> > +
> > +static void
> > +virEventGLibFDSourceFinalize(GSource *source G_GNUC_UNUSED)
> > +{
> > +}
> > +
> > +
> > +GSourceFuncs virEventGLibFDSourceFuncs = {
> > +    .prepare = virEventGLibFDSourcePrepare,
> > +    .check = virEventGLibFDSourceCheck,
> > +    .dispatch = virEventGLibFDSourceDispatch,
> > +    .finalize = virEventGLibFDSourceFinalize
> > +};
> > +
> > +
> > +GSource *virEventGLibCreateSocketWatch(int fd,
> > +                                       GIOCondition condition)
> > +{
> > +    GSource *source;
> > +    virEventGLibFDSource *ssource;
> > +
> > +    source = g_source_new(&virEventGLibFDSourceFuncs,
> > +                          sizeof(virEventGLibFDSource));
> > +    ssource = (virEventGLibFDSource *)source;
> > +
> > +    ssource->condition = condition;
> > +    ssource->fd = fd;
> > +
> > +    ssource->pollfd.fd = fd;
> > +    ssource->pollfd.events = condition;
> > +
> > +    g_source_add_poll(source, &ssource->pollfd);
> > +
> > +    return source;
> > +}
> > +
> > +#else /* WIN32 */
> > +
> > +# define WIN32_LEAN_AND_MEAN
> > +# include <winsock2.h>
> > +
> > +typedef struct virEventGLibSocketSource virEventGLibSocketSource;
> > +struct virEventGLibSocketSource {
> > +    GSource parent;
> > +    GPollFD pollfd;
> > +    int fd;
> > +    SOCKET socket;
> > +    HANDLE event;
> > +    int revents;
> > +    GIOCondition condition;
> > +};
> > +
> > +
> > +static gboolean
> > +virEventGLibSocketSourcePrepare(GSource *source G_GNUC_UNUSED,
> > +                                gint *timeout)
> > +{
> > +    *timeout = -1;
> > +
> > +    return FALSE;
> > +}
> > +
> > +
> > +/*
> > + * NB, this impl only works when the socket is in non-blocking
> > + * mode on Win32
> > + */
> > +static gboolean
> > +virEventGLibSocketSourceCheck(GSource *source)
> > +{
> > +    static struct timeval tv0;
> > +
> > +    virEventGLibSocketSource *ssource = (virEventGLibSocketSource *)source;
> > +    WSANETWORKEVENTS ev;
> > +    fd_set rfds, wfds, xfds;
> > +
> > +    if (!ssource->condition)
> > +        return 0;
> > +
> > +    WSAEnumNetworkEvents(ssource->socket, ssource->event, &ev);
> > +
> > +    FD_ZERO(&rfds);
> > +    FD_ZERO(&wfds);
> > +    FD_ZERO(&xfds);
> > +    if (ssource->condition & G_IO_IN)
> > +        FD_SET(ssource->socket, &rfds);
> > +    if (ssource->condition & G_IO_OUT)
> > +        FD_SET(ssource->socket, &wfds);
> > +    if (ssource->condition & G_IO_PRI)
> > +        FD_SET(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
> > +virEventGLibSocketSourceDispatch(GSource *source,
> > +                                 GSourceFunc callback,
> > +                                 gpointer user_data)
> > +{
> > +    virEventGLibSocketFunc func = (virEventGLibSocketFunc)callback;
> > +    virEventGLibSocketSource *ssource = (virEventGLibSocketSource *)source;
> > +
> > +    return (*func)(ssource->fd, ssource->revents, user_data);
> > +}
> > +
> > +
> > +static void
> > +virEventGLibSocketSourceFinalize(GSource *source)
> > +{
> > +    virEventGLibSocketSource *ssource = (virEventGLibSocketSource *)source;
> > +
> > +    WSAEventSelect(ssource->socket, NULL, 0);
> > +}
> > +
> > +
> > +GSourceFuncs virEventGLibSocketSourceFuncs = {
> > +    .prepare = virEventGLibSocketSourcePrepare,
> > +    .check = virEventGLibSocketSourceCheck,
> > +    .dispatch = virEventGLibSocketSourceDispatch,
> > +    .finalize = virEventGLibSocketSourceFinalize
> > +};
> > +
> > +
> > +GSource *virEventGLibCreateSocketWatch(int fd,
> > +                                       GIOCondition condition)
> > +{
> > +    GSource *source;
> > +    virEventGLibSocketSource *ssource;
> > +
> > +    source = g_source_new(&virEventGLibSocketSourceFuncs,
> > +                          sizeof(virEventGLibSocketSource));
> > +    ssource = (virEventGLibSocketSource *)source;
> > +
> > +    ssource->condition = condition;
> > +    ssource->fd = fd;
> > +    ssource->socket = _get_osfhandle(fd);
> > +    ssource->event = CreateEvent(NULL, FALSE, FALSE, NULL);
> 
> Based on going through QEMU code and windows documentation it looks
> like we have to free the event using CloseHandle().
> 
> QEMU is doing it in io/channel.c in qio_channel_finalize() which is
> probably called by unrefing ssource->ioc in
> qio_channel_fd_pair_source_finalize().
> 
> That would mean we have to call CloseHandle() in
> virEventGLibSocketSourceFinalize() to make sure we will not leak any
> resources.

Yes, you are correct.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [libvirt PATCH v2 33/56] src: introduce helper API for creating GSource for socket
Posted by Pavel Hrdina 6 years ago
On Wed, Jan 29, 2020 at 04:34:46PM +0000, Daniel P. Berrangé wrote:
> On Wed, Jan 29, 2020 at 05:33:20PM +0100, Pavel Hrdina wrote:
> > On Tue, Jan 28, 2020 at 01:11:14PM +0000, Daniel P. Berrangé wrote:
> > > We need to be able to create event loop watches using the
> > > GSource API for sockets. GIOChannel is able todo this, but
> > > we don't want to use the GIOChannel APIs for reading/writing,
> > > and testing shows just using its GSource APIs is unreliable
> > > on Windows.
> > > 
> > > This patch thus creates a standalone helper API for creating
> > > a GSource for a socket file descriptor. This impl is derived
> > > from code in QEMU's io/channel-watch.c file that was written
> > > by myself & Paolo Bonzini & thus under Red Hat copyright.
> > > 
> > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > > ---
> > >  build-aux/syntax-check.mk    |   3 +
> > >  src/util/Makefile.inc.am     |   2 +
> > >  src/util/vireventglibwatch.c | 248 +++++++++++++++++++++++++++++++++++
> > >  src/util/vireventglibwatch.h |  48 +++++++
> > >  4 files changed, 301 insertions(+)
> > >  create mode 100644 src/util/vireventglibwatch.c
> > >  create mode 100644 src/util/vireventglibwatch.h
> > > 
> > > diff --git a/src/util/vireventglibwatch.c b/src/util/vireventglibwatch.c
> > > new file mode 100644
> > > index 0000000000..f7b087e2ec
> > > --- /dev/null
> > > +++ b/src/util/vireventglibwatch.c
> > > @@ -0,0 +1,248 @@
> > > +/*
> > > + * vireventglibwatch.c: GSource impl for sockets
> > > + *
> > > + * Copyright (C) 2015-2020 Red Hat, Inc.
> > > + *
> > > + * This library is free software; you can redistribute it and/or
> > > + * modify it under the terms of the GNU Lesser General Public
> > > + * License as published by the Free Software Foundation; either
> > > + * version 2.1 of the License, or (at your option) any later version.
> > > + *
> > > + * This library is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > > + * Lesser General Public License for more details.
> > > + *
> > > + * You should have received a copy of the GNU Lesser General Public
> > > + * License along with this library. If not, see
> > > + * <http://www.gnu.org/licenses/>.
> > > + */
> > > +
> > > +#include <config.h>
> > > +
> > > +#include "vireventglibwatch.h"
> > > +
> > > +#ifndef WIN32
> > > +typedef struct virEventGLibFDSource virEventGLibFDSource;
> > > +struct virEventGLibFDSource {
> > > +    GSource parent;
> > > +    GPollFD pollfd;
> > > +    int fd;
> > > +    GIOCondition condition;
> > > +};
> > > +
> > > +
> > > +static gboolean
> > > +virEventGLibFDSourcePrepare(GSource *source G_GNUC_UNUSED,
> > > +                            gint *timeout)
> > > +{
> > > +    *timeout = -1;
> > > +
> > > +    return FALSE;
> > > +}
> > > +
> > > +
> > > +static gboolean
> > > +virEventGLibFDSourceCheck(GSource *source)
> > > +{
> > > +    virEventGLibFDSource *ssource = (virEventGLibFDSource *)source;
> > > +
> > > +    return ssource->pollfd.revents & ssource->condition;
> > > +}
> > > +
> > > +
> > > +static gboolean
> > > +virEventGLibFDSourceDispatch(GSource *source,
> > > +                             GSourceFunc callback,
> > > +                             gpointer user_data)
> > > +{
> > > +    virEventGLibSocketFunc func = (virEventGLibSocketFunc)callback;
> > > +    virEventGLibFDSource *ssource = (virEventGLibFDSource *)source;
> > > +
> > > +    return (*func)(ssource->fd,
> > > +                   ssource->pollfd.revents & ssource->condition,
> > > +                   user_data);
> > > +}
> > > +
> > > +
> > > +static void
> > > +virEventGLibFDSourceFinalize(GSource *source G_GNUC_UNUSED)
> > > +{
> > > +}
> > > +
> > > +
> > > +GSourceFuncs virEventGLibFDSourceFuncs = {
> > > +    .prepare = virEventGLibFDSourcePrepare,
> > > +    .check = virEventGLibFDSourceCheck,
> > > +    .dispatch = virEventGLibFDSourceDispatch,
> > > +    .finalize = virEventGLibFDSourceFinalize
> > > +};
> > > +
> > > +
> > > +GSource *virEventGLibCreateSocketWatch(int fd,
> > > +                                       GIOCondition condition)
> > > +{
> > > +    GSource *source;
> > > +    virEventGLibFDSource *ssource;
> > > +
> > > +    source = g_source_new(&virEventGLibFDSourceFuncs,
> > > +                          sizeof(virEventGLibFDSource));
> > > +    ssource = (virEventGLibFDSource *)source;
> > > +
> > > +    ssource->condition = condition;
> > > +    ssource->fd = fd;
> > > +
> > > +    ssource->pollfd.fd = fd;
> > > +    ssource->pollfd.events = condition;
> > > +
> > > +    g_source_add_poll(source, &ssource->pollfd);
> > > +
> > > +    return source;
> > > +}
> > > +
> > > +#else /* WIN32 */
> > > +
> > > +# define WIN32_LEAN_AND_MEAN
> > > +# include <winsock2.h>
> > > +
> > > +typedef struct virEventGLibSocketSource virEventGLibSocketSource;
> > > +struct virEventGLibSocketSource {
> > > +    GSource parent;
> > > +    GPollFD pollfd;
> > > +    int fd;
> > > +    SOCKET socket;
> > > +    HANDLE event;
> > > +    int revents;
> > > +    GIOCondition condition;
> > > +};
> > > +
> > > +
> > > +static gboolean
> > > +virEventGLibSocketSourcePrepare(GSource *source G_GNUC_UNUSED,
> > > +                                gint *timeout)
> > > +{
> > > +    *timeout = -1;
> > > +
> > > +    return FALSE;
> > > +}
> > > +
> > > +
> > > +/*
> > > + * NB, this impl only works when the socket is in non-blocking
> > > + * mode on Win32
> > > + */
> > > +static gboolean
> > > +virEventGLibSocketSourceCheck(GSource *source)
> > > +{
> > > +    static struct timeval tv0;
> > > +
> > > +    virEventGLibSocketSource *ssource = (virEventGLibSocketSource *)source;
> > > +    WSANETWORKEVENTS ev;
> > > +    fd_set rfds, wfds, xfds;
> > > +
> > > +    if (!ssource->condition)
> > > +        return 0;
> > > +
> > > +    WSAEnumNetworkEvents(ssource->socket, ssource->event, &ev);
> > > +
> > > +    FD_ZERO(&rfds);
> > > +    FD_ZERO(&wfds);
> > > +    FD_ZERO(&xfds);
> > > +    if (ssource->condition & G_IO_IN)
> > > +        FD_SET(ssource->socket, &rfds);
> > > +    if (ssource->condition & G_IO_OUT)
> > > +        FD_SET(ssource->socket, &wfds);
> > > +    if (ssource->condition & G_IO_PRI)
> > > +        FD_SET(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
> > > +virEventGLibSocketSourceDispatch(GSource *source,
> > > +                                 GSourceFunc callback,
> > > +                                 gpointer user_data)
> > > +{
> > > +    virEventGLibSocketFunc func = (virEventGLibSocketFunc)callback;
> > > +    virEventGLibSocketSource *ssource = (virEventGLibSocketSource *)source;
> > > +
> > > +    return (*func)(ssource->fd, ssource->revents, user_data);
> > > +}
> > > +
> > > +
> > > +static void
> > > +virEventGLibSocketSourceFinalize(GSource *source)
> > > +{
> > > +    virEventGLibSocketSource *ssource = (virEventGLibSocketSource *)source;
> > > +
> > > +    WSAEventSelect(ssource->socket, NULL, 0);
> > > +}
> > > +
> > > +
> > > +GSourceFuncs virEventGLibSocketSourceFuncs = {
> > > +    .prepare = virEventGLibSocketSourcePrepare,
> > > +    .check = virEventGLibSocketSourceCheck,
> > > +    .dispatch = virEventGLibSocketSourceDispatch,
> > > +    .finalize = virEventGLibSocketSourceFinalize
> > > +};
> > > +
> > > +
> > > +GSource *virEventGLibCreateSocketWatch(int fd,
> > > +                                       GIOCondition condition)
> > > +{
> > > +    GSource *source;
> > > +    virEventGLibSocketSource *ssource;
> > > +
> > > +    source = g_source_new(&virEventGLibSocketSourceFuncs,
> > > +                          sizeof(virEventGLibSocketSource));
> > > +    ssource = (virEventGLibSocketSource *)source;
> > > +
> > > +    ssource->condition = condition;
> > > +    ssource->fd = fd;
> > > +    ssource->socket = _get_osfhandle(fd);
> > > +    ssource->event = CreateEvent(NULL, FALSE, FALSE, NULL);
> > 
> > Based on going through QEMU code and windows documentation it looks
> > like we have to free the event using CloseHandle().
> > 
> > QEMU is doing it in io/channel.c in qio_channel_finalize() which is
> > probably called by unrefing ssource->ioc in
> > qio_channel_fd_pair_source_finalize().
> > 
> > That would mean we have to call CloseHandle() in
> > virEventGLibSocketSourceFinalize() to make sure we will not leak any
> > resources.
> 
> Yes, you are correct.

With that fixed and a hopefully nothing else was missed

Reviewed-by: Pavel Hrdina <phrdina@redhat.com>