On 13/01/18(Sat) 16:13, Martin Pieuchot wrote:
> Diff below is a redesign of the kqueue(2) backend for GIO/glib.
> It fixes BZ #739424 and a couple of more bugs and races & reduces
> the size of the backend by 1K+ lines.
>
> The backend is still not generating all the events it should, but
> that's a different problem. More details about my work can be found
> in the git commit below.
>
> I'd appreciate test reports before I push that upstream.
Anyone?
> Index: Makefile
> ===================================================================
> RCS file: /cvs/ports/devel/glib2/Makefile,v
> retrieving revision 1.268
> diff -u -p -r1.268 Makefile
> --- Makefile 9 Jan 2018 12:31:54 -0000 1.268
> +++ Makefile 13 Jan 2018 14:55:24 -0000
> @@ -9,6 +9,7 @@ COMMENT= general-purpose utility librar
> GNOME_PROJECT= glib
> GNOME_VERSION= 2.54.3
> PKGNAME= ${DISTNAME:S/glib/glib2/}
> +REVISION= 0
>
> CATEGORIES= devel
>
> @@ -54,6 +55,12 @@ CONFIGURE_ARGS= --enable-static \
> --with-pcre=system \
> --disable-fam
>
> +AUTOCONF_VERSION= 2.69
> +AUTOMAKE_VERSION= 1.15
> +BUILD_DEPENDS += ${MODGNU_AUTOCONF_DEPENDS} \
> + ${MODGNU_AUTOMAKE_DEPENDS} \
> + devel/libtool
> +
> .include <bsd.port.arch.mk>
> .if ${PROPERTIES:Mgcc3}
> PATCH_LIST= patch-* gcc3-*
> @@ -71,5 +78,9 @@ pre-configure:
> sed -i "s,/usr/share,${PREFIX}/share,g" \
> ${WRKSRC}/docs/reference/gio/html/glib-compile-schemas.html \
> ${WRKSRC}/docs/reference/gio/glib-compile-schemas.xml
> + cd ${WRKSRC} && env -i \
> + AUTOCONF_VERSION=${AUTOCONF_VERSION} \
> + AUTOMAKE_VERSION=${AUTOMAKE_VERSION} \
> + autoreconf -fi
>
> .include <bsd.port.mk>
> Index: patches/patch-gio_kqueue_gkqueuefilemonitor_c
> ===================================================================
> RCS file: patches/patch-gio_kqueue_gkqueuefilemonitor_c
> diff -N patches/patch-gio_kqueue_gkqueuefilemonitor_c
> --- patches/patch-gio_kqueue_gkqueuefilemonitor_c 8 May 2017 16:57:08 -0000 1.2
> +++ /dev/null 1 Jan 1970 00:00:00 -0000
> @@ -1,50 +0,0 @@
> -$OpenBSD: patch-gio_kqueue_gkqueuefilemonitor_c,v 1.2 2017/05/08 16:57:08 ajacoutot Exp $
> -
> -Prevent a use-after-free resulting in a crash of all apps depending
> -on glib/GIO mostly at "pkg_add -u" time.
> -https://bugzilla.gnome.org/show_bug.cgi?id=739424
> -
> -Index: gio/kqueue/gkqueuefilemonitor.c
> ---- gio/kqueue/gkqueuefilemonitor.c.orig
> -+++ gio/kqueue/gkqueuefilemonitor.c
> -@@ -30,6 +30,16 @@
> - #include <gio/giomodule.h>
> -
> -
> -+/*
> -+ * Because `kqueue_sub'' are not refcounted, we need
> -+ * ensure no other thread is getting a reference to
> -+ * the element we want to free.
> -+ *
> -+ * That's why _kh_cancel_sub() must be called with
> -+ * this lock held to prevent a race.
> -+ */
> -+G_LOCK_EXTERN (hash_lock);
> -+
> - struct _GKqueueFileMonitor
> - {
> - GLocalFileMonitor parent_instance;
> -@@ -80,9 +90,11 @@ g_kqueue_file_monitor_finalize (GObject *object)
> -
> - if (kqueue_monitor->sub)
> - {
> -+ G_LOCK (hash_lock);
> - _kh_cancel_sub (kqueue_monitor->sub);
> - _kh_sub_free (kqueue_monitor->sub);
> - kqueue_monitor->sub = NULL;
> -+ G_UNLOCK (hash_lock);
> - }
> -
> - if (kqueue_monitor->fallback)
> -@@ -181,9 +193,11 @@ g_kqueue_file_monitor_cancel (GFileMonitor *monitor)
> -
> - if (kqueue_monitor->sub)
> - {
> -+ G_LOCK (hash_lock);
> - _kh_cancel_sub (kqueue_monitor->sub);
> - _kh_sub_free (kqueue_monitor->sub);
> - kqueue_monitor->sub = NULL;
> -+ G_UNLOCK (hash_lock);
> - }
> - else if (kqueue_monitor->fallback)
> - {
> Index: patches/patch-gio_kqueue_kqueue-helper_c
> ===================================================================
> RCS file: patches/patch-gio_kqueue_kqueue-helper_c
> diff -N patches/patch-gio_kqueue_kqueue-helper_c
> --- patches/patch-gio_kqueue_kqueue-helper_c 9 Jan 2018 12:31:54 -0000 1.4
> +++ /dev/null 1 Jan 1970 00:00:00 -0000
> @@ -1,64 +0,0 @@
> -$OpenBSD: patch-gio_kqueue_kqueue-helper_c,v 1.4 2018/01/09 12:31:54 ajacoutot Exp $
> -
> -Prevent a use-after-free resulting in a crash of all apps depending
> -on glib/GIO mostly at "pkg_add -u" time.
> -https://bugzilla.gnome.org/show_bug.cgi?id=739424
> -
> -Index: gio/kqueue/kqueue-helper.c
> ---- gio/kqueue/kqueue-helper.c.orig
> -+++ gio/kqueue/kqueue-helper.c
> -@@ -43,7 +43,7 @@ static gboolean kh_debug_enabled = FALSE;
> - #define KH_W if (kh_debug_enabled) g_warning
> -
> - static GHashTable *subs_hash_table = NULL;
> --G_LOCK_DEFINE_STATIC (hash_lock);
> -+G_LOCK_DEFINE (hash_lock);
> -
> - static int kqueue_descriptor = -1;
> - static int kqueue_socket_pair[] = {-1, -1};
> -@@ -293,10 +293,10 @@ process_kqueue_notifications (GIOChannel *gioc,
> -
> - G_LOCK (hash_lock);
> - sub = (kqueue_sub *) g_hash_table_lookup (subs_hash_table, GINT_TO_POINTER (n.fd));
> -- G_UNLOCK (hash_lock);
> -
> - if (sub == NULL)
> - {
> -+ G_UNLOCK (hash_lock);
> - KH_W ("Got a notification for a deleted or non-existing subscription %d",
> - n.fd);
> - return TRUE;
> -@@ -338,6 +338,7 @@ process_kqueue_notifications (GIOChannel *gioc,
> - g_file_monitor_source_handle_event (source, mask, NULL, NULL, NULL, g_get_monotonic_time ());
> - }
> -
> -+ G_UNLOCK (hash_lock);
> - return TRUE;
> - }
> -
> -@@ -453,13 +454,14 @@ _kh_start_watching (kqueue_sub *sub)
> -
> - G_LOCK (hash_lock);
> - g_hash_table_insert (subs_hash_table, GINT_TO_POINTER (sub->fd), sub);
> -- G_UNLOCK (hash_lock);
> -
> - _kqueue_thread_push_fd (sub->fd);
> -
> - /* Bump the kqueue thread. It will pick up a new sub entry to monitor */
> - if (!_ku_write (kqueue_socket_pair[0], "A", 1))
> - KH_W ("Failed to bump the kqueue thread (add fd, error %d)", errno);
> -+ G_UNLOCK (hash_lock);
> -+
> - return TRUE;
> - }
> -
> -@@ -506,9 +508,7 @@ _kh_cancel_sub (kqueue_sub *sub)
> -
> - _km_remove (sub);
> -
> -- G_LOCK (hash_lock);
> - removed = g_hash_table_remove (subs_hash_table, GINT_TO_POINTER (sub->fd));
> -- G_UNLOCK (hash_lock);
> -
> - if (removed)
> - {
> Index: patches/patch-gio_tests_file_c
> ===================================================================
> RCS file: /cvs/ports/devel/glib2/patches/patch-gio_tests_file_c,v
> retrieving revision 1.7
> diff -u -p -r1.7 patch-gio_tests_file_c
> --- patches/patch-gio_tests_file_c 24 Sep 2015 10:23:42 -0000 1.7
> +++ patches/patch-gio_tests_file_c 13 Jan 2018 14:59:18 -0000
> @@ -1,7 +1,6 @@
> $OpenBSD: patch-gio_tests_file_c,v 1.7 2015/09/24 10:23:42 ajacoutot Exp $
>
> -XXX why isn't this working with kqueue?
> -push upstream when fixed
> +The current kqueue backend does not monitor non-existing file correctly.
>
> --- gio/tests/file.c.orig Wed Aug 19 05:35:30 2015
> +++ gio/tests/file.c Wed Sep 23 09:25:35 2015
> Index: patches/patch-kqueue_fix
> ===================================================================
> RCS file: patches/patch-kqueue_fix
> diff -N patches/patch-kqueue_fix
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ patches/patch-kqueue_fix 13 Jan 2018 14:59:47 -0000
> @@ -0,0 +1,2083 @@
> +commit 59fcc304e9f29467e0cc68c4ec6d4b3c0d0fcd59
> +Author: Martin Pieuchot <mpi@openbsd.org>
> +Date: Sat Jan 13 15:25:30 2018 +0100
> +
> + kqueue: Multiple fixes and simplifications.
> +
> + - Stop using a custom thread for listening to kqueue(2) events. Instead
> + call kevent(2) in non blocking mode in a monitor callback. Under the
> + hood poll(2) is used to figure out if new events are available.
> +
> + - Do not use a socketpair with a custom protocol requiring 2 supplementary
> + context switches per event to commicate between multiple threads. Calling
> + kevent(2), in non blocking mode, to add/remove events is fine from any
> + context.
> +
> + - Add kqueue(2) events without the EV_ONESHOT flag. This removes a race
> + were some notifications were lost because events had to be re-added for
> + every new notification.
> +
> + - Get rid of the global hash table and its associated lock and races. Use
> + the 'cookie' argument of kevent(2) to pass the associated descriptor when
> + registering an event.
> +
> + - Fix _kh_file_appeared_cb() by properly passing a monitor instead of a
> + source to g_file_monitor_emit_event().
> +
> + - Porperly refcount sources.
> +
> + - Remove a lot of abstraction making it harder to fix the remaining issues.
> +
> + This diff does not improve the correctness of the generate events, a lot
> + can still be done.
> +
> + Fix https://bugzilla.gnome.org/show_bug.cgi?id=739424
> +
> +diff --git gio/kqueue/Makefile.am gio/kqueue/Makefile.am
> +index d5657d7e4..24e9724e5 100644
> +--- gio/kqueue/Makefile.am
> ++++ gio/kqueue/Makefile.am
> +@@ -4,19 +4,9 @@ noinst_LTLIBRARIES += libkqueue.la
> +
> + libkqueue_la_SOURCES = \
> + gkqueuefilemonitor.c \
> +- gkqueuefilemonitor.h \
> + kqueue-helper.c \
> + kqueue-helper.h \
> +- kqueue-thread.c \
> +- kqueue-thread.h \
> +- kqueue-sub.c \
> +- kqueue-sub.h \
> + kqueue-missing.c \
> +- kqueue-missing.h \
> +- kqueue-utils.c \
> +- kqueue-utils.h \
> +- kqueue-exclusions.c \
> +- kqueue-exclusions.h \
> + dep-list.c \
> + dep-list.h \
> + $(NULL)
> +diff --git gio/kqueue/gkqueuefilemonitor.c gio/kqueue/gkqueuefilemonitor.c
> +index 78b749637..a9107b0a9 100644
> +--- gio/kqueue/gkqueuefilemonitor.c
> ++++ gio/kqueue/gkqueuefilemonitor.c
> +@@ -22,33 +22,72 @@
> +
> + #include "config.h"
> +
> +-#include "gkqueuefilemonitor.h"
> +-#include "kqueue-helper.h"
> +-#include "kqueue-exclusions.h"
> ++#include <sys/types.h>
> ++#include <sys/event.h>
> ++#include <sys/time.h>
> ++#include <sys/socket.h>
> ++#include <sys/stat.h>
> ++
> ++#include <errno.h>
> ++#include <fcntl.h>
> ++#include <string.h>
> ++
> ++#include <glib-object.h>
> ++#include <gio/gfilemonitor.h>
> ++#include <gio/glocalfilemonitor.h>
> ++#include <gio/giomodule.h>
> + #include <gio/gpollfilemonitor.h>
> + #include <gio/gfile.h>
> +-#include <gio/giomodule.h>
> ++#include <glib-unix.h>
> ++#include "glib-private.h"
> ++
> ++#include "kqueue-helper.h"
> ++#include "dep-list.h"
> ++
> ++static GSource *kq_source;
> ++static int kq_queue = -1;
> ++
> ++#define G_TYPE_KQUEUE_FILE_MONITOR (g_kqueue_file_monitor_get_type ())
> ++#define G_KQUEUE_FILE_MONITOR(inst) (G_TYPE_CHECK_INSTANCE_CAST ((inst), \
> ++ G_TYPE_KQUEUE_FILE_MONITOR, GKqueueFileMonitor))
> +
> ++typedef GLocalFileMonitorClass GKqueueFileMonitorClass;
> +
> +-struct _GKqueueFileMonitor
> ++typedef struct
> + {
> + GLocalFileMonitor parent_instance;
> +
> + kqueue_sub *sub;
> +-
> ++#ifndef O_EVTONLY
> + GFileMonitor *fallback;
> + GFile *fbfile;
> +-};
> ++
No comments:
Post a Comment