[PATCH libinput 1/2] Make context reference counted
Peter Hutterer
peter.hutterer at who-t.net
Mon Jun 23 18:29:37 PDT 2014
On Tue, Jun 24, 2014 at 01:20:47AM +0300, Ran Benita wrote:
> On Mon, Jun 23, 2014 at 11:56:41PM +0200, Jonas Ådahl wrote:
> > Instead of only allowing one owner keeping a libinput context alive,
> > make context reference counted, replacing libinput_destroy() with
> > libinput_unref() while adding another function libinput_ref().
> >
> > Even though there might not be any current use cases, it doesn't mean we
> > should hard code this usage model in the API. The old behaviour can be
> > emulated by never calling libinput_ref() while replacing
> > libinput_destroy() with libinput_unref().
> >
> > Signed-off-by: Jonas Ådahl <jadahl at gmail.com>
> > ---
> >
> > There are no users I know of which currently would benefit from this but
> > I'm thinking it might be better to at least not disallow. Yay or nay?
The Xorg libinput driver is that use-case. It shares the libinput context
between separate modules and I did have a patchset to refcount the libinput
context at some point but ditched it.
The specific details here are: the driver module has a static libinput
context, initialized with the first device. All devices are then
added to/removed from that context. ref/unref works fine until you remove
the last device - then the context becomes invalid but you don't know, and
the next time you add a device things blow up.
So this patchset doesn't really help, I'd still need to refcount manually
to figure out when the context was destroyed. With Ran's suggestion the
story is a bit different: if we make ref() return the object and unref()
return the object or NULL if it was destroyed, then I can use this in the
xorg driver. [and for consistency that would have to apply to all other
ref/unref() functions too.]
There's one more problem:
libinput_destroy() is an explicit "kill everthing", making all objects
invalid.
libinput_unref() doesn't, objects stay valid or not and without knowing the
refcount you don't really know. And as your patch says: "It is up to the
caller to keep track of the number of references held". I wonder what use
the refcounting is then if we rely on the caller to track it anyway?
So I'm a bit meh on this: it changes the semantics for what I can tell is of
no benefit right now, and the benefits that could come from it aren't
available in this API.
Cheers,
Peter
> > src/libinput-private.h | 1 +
> > src/libinput.c | 14 +++++++++++++-
> > src/libinput.h | 25 ++++++++++++++++++++++---
> > src/udev-seat.c | 2 +-
> > test/keyboard.c | 2 +-
> > test/litest.c | 2 +-
> > test/log.c | 10 +++++-----
> > test/misc.c | 10 +++++-----
> > test/path.c | 20 ++++++++++----------
> > test/pointer.c | 2 +-
> > test/udev.c | 16 ++++++++--------
> > tools/event-debug.c | 4 ++--
> > tools/event-gui.c | 2 +-
> > 13 files changed, 71 insertions(+), 39 deletions(-)
> >
> > diff --git a/src/libinput-private.h b/src/libinput-private.h
> > index f0bda1f..cfe6535 100644
> > --- a/src/libinput-private.h
> > +++ b/src/libinput-private.h
> > @@ -57,6 +57,7 @@ struct libinput {
> > const struct libinput_interface *interface;
> > const struct libinput_interface_backend *interface_backend;
> > void *user_data;
> > + int refcount;
> > };
> >
> > typedef void (*libinput_seat_destroy_func) (struct libinput_seat *seat);
> > diff --git a/src/libinput.c b/src/libinput.c
> > index c4f7fe1..d3e2149 100644
> > --- a/src/libinput.c
> > +++ b/src/libinput.c
> > @@ -502,6 +502,7 @@ libinput_init(struct libinput *libinput,
> > libinput->interface = interface;
> > libinput->interface_backend = interface_backend;
> > libinput->user_data = user_data;
> > + libinput->refcount = 1;
> > list_init(&libinput->source_destroy_list);
> > list_init(&libinput->seat_list);
> >
> > @@ -531,7 +532,13 @@ libinput_drop_destroyed_sources(struct libinput *libinput)
> > }
> >
> > LIBINPUT_EXPORT void
> > -libinput_destroy(struct libinput *libinput)
> > +libinput_ref(struct libinput *libinput)
>
> How about making this function return its argument? Often you have code
> like
>
> void obj_new(struct libinput *libinput)
> {
> [...]
> obj->libinput = libinput;
> libinput_ref(libinput);
> [...]
> }
>
> I think this is better:
>
> obj->libinput = libinput_ref(libinput);
> > +{
> > + libinput->refcount++;
> > +}
> > +
> > +LIBINPUT_EXPORT void
> > +libinput_unref(struct libinput *libinput)
> > {
> > struct libinput_event *event;
> > struct libinput_device *device, *next_device;
> > @@ -540,6 +547,11 @@ libinput_destroy(struct libinput *libinput)
> > if (libinput == NULL)
> > return;
> >
> > + assert(libinput->refcount > 0);
> > + libinput->refcount--;
> > + if (libinput->refcount > 0)
> > + return;
> > +
> > libinput_suspend(libinput);
> >
> > libinput->interface_backend->destroy(libinput);
> > diff --git a/src/libinput.h b/src/libinput.h
> > index b1b1124..1eada8f 100644
> > --- a/src/libinput.h
> > +++ b/src/libinput.h
> > @@ -793,6 +793,9 @@ struct libinput_interface {
> > * device are ignored. Such devices and those that failed to open
> > * ignored until the next call to libinput_resume().
> > *
> > + * The reference count of the context is initialized to 1. See @ref
> > + * libinput_unref.
> > + *
> > * @param interface The callback interface
> > * @param user_data Caller-specific data passed to the various callback
> > * interfaces.
> > @@ -818,6 +821,9 @@ libinput_udev_create_for_seat(const struct libinput_interface *interface,
> > * The context is fully initialized but will not generate events until at
> > * least one device has been added.
> > *
> > + * The reference count of the context is initialized to 1. See @ref
> > + * libinput_unref.
> > + *
> > * @param interface The callback interface
> > * @param user_data Caller-specific data passed to the various callback
> > * interfaces.
> > @@ -967,13 +973,26 @@ libinput_suspend(struct libinput *libinput);
> > /**
> > * @ingroup base
> > *
> > - * Destroy the libinput context. After this, object references associated with
> > - * the destroyed context are invalid and may not be interacted with.
> > + * Add a reference to the context. A context is destroyed whenever the
> > + * reference count reaches 0. See @ref libinput_unref.
> > + *
> > + * @param libinput A previously initialized valid libinput context
> > + */
> > +void
> > +libinput_ref(struct libinput *libinput);
> > +
> > +/**
> > + * @ingroup base
> > + *
> > + * Dereference the libinput context. After this, the context may have been
> > + * destroyed, if the last reference was dereferenced. If so, the context is
> > + * invalid and may not be interacted with. It is up to the caller to keep
> > + * track of the number of references held.
> > *
> > * @param libinput A previously initialized libinput context
> > */
> > void
> > -libinput_destroy(struct libinput *libinput);
> > +libinput_unref(struct libinput *libinput);
> >
> > /**
> > * @ingroup base
> > diff --git a/src/udev-seat.c b/src/udev-seat.c
> > index 38a13b7..5b61dee 100644
> > --- a/src/udev-seat.c
> > +++ b/src/udev-seat.c
> > @@ -358,7 +358,7 @@ libinput_udev_create_for_seat(const struct libinput_interface *interface,
> >
> > if (udev_input_enable(&input->base) < 0) {
> > udev_unref(udev);
> > - libinput_destroy(&input->base);
> > + libinput_unref(&input->base);
> > free(input);
> > return NULL;
> > }
> > diff --git a/test/keyboard.c b/test/keyboard.c
> > index a518b66..83153bb 100644
> > --- a/test/keyboard.c
> > +++ b/test/keyboard.c
> > @@ -108,7 +108,7 @@ START_TEST(keyboard_seat_key_count)
> >
> > for (i = 0; i < num_devices; ++i)
> > litest_delete_device(devices[i]);
> > - libinput_destroy(libinput);
> > + libinput_unref(libinput);
> > }
> > END_TEST
> >
> > diff --git a/test/litest.c b/test/litest.c
> > index 0a9cc72..55ba678 100644
> > --- a/test/litest.c
> > +++ b/test/litest.c
> > @@ -573,7 +573,7 @@ litest_delete_device(struct litest_device *d)
> >
> > libinput_device_unref(d->libinput_device);
> > if (d->owns_context)
> > - libinput_destroy(d->libinput);
> > + libinput_unref(d->libinput);
> > libevdev_free(d->evdev);
> > libevdev_uinput_destroy(d->uinput);
> > memset(d,0, sizeof(*d));
> > diff --git a/test/log.c b/test/log.c
> > index a281820..fe67d68 100644
> > --- a/test/log.c
> > +++ b/test/log.c
> > @@ -86,7 +86,7 @@ START_TEST(log_handler_invoked)
> > ck_assert_int_gt(log_handler_called, 0);
> > log_handler_called = 0;
> >
> > - libinput_destroy(li);
> > + libinput_unref(li);
> > libinput_log_set_priority(pri);
> > }
> > END_TEST
> > @@ -106,7 +106,7 @@ START_TEST(log_userdata_NULL)
> > ck_assert_int_gt(log_handler_called, 0);
> > log_handler_called = 0;
> >
> > - libinput_destroy(li);
> > + libinput_unref(li);
> >
> > libinput_log_set_priority(pri);
> > }
> > @@ -127,7 +127,7 @@ START_TEST(log_userdata)
> > ck_assert_int_gt(log_handler_called, 0);
> > log_handler_called = 0;
> >
> > - libinput_destroy(li);
> > + libinput_unref(li);
> > libinput_log_set_priority(pri);
> > }
> > END_TEST
> > @@ -148,7 +148,7 @@ START_TEST(log_handler_NULL)
> > log_handler_called = 0;
> > libinput_log_set_handler(simple_log_handler, NULL);
> >
> > - libinput_destroy(li);
> > + libinput_unref(li);
> > libinput_log_set_priority(pri);
> > }
> > END_TEST
> > @@ -173,7 +173,7 @@ START_TEST(log_priority)
> >
> > log_handler_called = 0;
> >
> > - libinput_destroy(li);
> > + libinput_unref(li);
> > libinput_log_set_priority(pri);
> > }
> > END_TEST
> > diff --git a/test/misc.c b/test/misc.c
> > index 133bdb6..ad2e1f6 100644
> > --- a/test/misc.c
> > +++ b/test/misc.c
> > @@ -133,7 +133,7 @@ START_TEST(event_conversion_device_notify)
> > libinput_event_destroy(event);
> > }
> >
> > - libinput_destroy(li);
> > + libinput_unref(li);
> > libevdev_uinput_destroy(uinput);
> >
> > ck_assert_int_gt(device_added, 0);
> > @@ -194,7 +194,7 @@ START_TEST(event_conversion_pointer)
> > libinput_event_destroy(event);
> > }
> >
> > - libinput_destroy(li);
> > + libinput_unref(li);
> > libevdev_uinput_destroy(uinput);
> >
> > ck_assert_int_gt(motion, 0);
> > @@ -254,7 +254,7 @@ START_TEST(event_conversion_pointer_abs)
> > libinput_event_destroy(event);
> > }
> >
> > - libinput_destroy(li);
> > + libinput_unref(li);
> > libevdev_uinput_destroy(uinput);
> >
> > ck_assert_int_gt(motion, 0);
> > @@ -304,7 +304,7 @@ START_TEST(event_conversion_key)
> > libinput_event_destroy(event);
> > }
> >
> > - libinput_destroy(li);
> > + libinput_unref(li);
> > libevdev_uinput_destroy(uinput);
> >
> > ck_assert_int_gt(key, 0);
> > @@ -364,7 +364,7 @@ START_TEST(event_conversion_touch)
> > libinput_event_destroy(event);
> > }
> >
> > - libinput_destroy(li);
> > + libinput_unref(li);
> > libevdev_uinput_destroy(uinput);
> >
> > ck_assert_int_gt(touch, 0);
> > diff --git a/test/path.c b/test/path.c
> > index 24f60e0..99b474e 100644
> > --- a/test/path.c
> > +++ b/test/path.c
> > @@ -65,7 +65,7 @@ START_TEST(path_create_NULL)
> > ck_assert(li == NULL);
> > li = libinput_path_create_context(&simple_interface, NULL);
> > ck_assert(li != NULL);
> > - libinput_destroy(li);
> > + libinput_unref(li);
> >
> > ck_assert_int_eq(open_func_count, 0);
> > ck_assert_int_eq(close_func_count, 0);
> > @@ -92,7 +92,7 @@ START_TEST(path_create_invalid)
> > ck_assert_int_eq(open_func_count, 0);
> > ck_assert_int_eq(close_func_count, 0);
> >
> > - libinput_destroy(li);
> > + libinput_unref(li);
> > ck_assert_int_eq(close_func_count, 0);
> >
> > open_func_count = 0;
> > @@ -126,7 +126,7 @@ START_TEST(path_create_destroy)
> > ck_assert_int_eq(open_func_count, 1);
> >
> > libevdev_uinput_destroy(uinput);
> > - libinput_destroy(li);
> > + libinput_unref(li);
> > ck_assert_int_eq(close_func_count, 1);
> >
> > open_func_count = 0;
> > @@ -372,7 +372,7 @@ START_TEST(path_suspend)
> > libinput_resume(li);
> >
> > libevdev_uinput_destroy(uinput);
> > - libinput_destroy(li);
> > + libinput_unref(li);
> >
> > open_func_count = 0;
> > close_func_count = 0;
> > @@ -406,7 +406,7 @@ START_TEST(path_double_suspend)
> > libinput_resume(li);
> >
> > libevdev_uinput_destroy(uinput);
> > - libinput_destroy(li);
> > + libinput_unref(li);
> >
> > open_func_count = 0;
> > close_func_count = 0;
> > @@ -440,7 +440,7 @@ START_TEST(path_double_resume)
> > libinput_resume(li);
> >
> > libevdev_uinput_destroy(uinput);
> > - libinput_destroy(li);
> > + libinput_unref(li);
> >
> > open_func_count = 0;
> > close_func_count = 0;
> > @@ -523,7 +523,7 @@ START_TEST(path_add_device_suspend_resume)
> >
> > libevdev_uinput_destroy(uinput1);
> > libevdev_uinput_destroy(uinput2);
> > - libinput_destroy(li);
> > + libinput_unref(li);
> >
> > open_func_count = 0;
> > close_func_count = 0;
> > @@ -614,7 +614,7 @@ START_TEST(path_add_device_suspend_resume_fail)
> > ck_assert_int_eq(nevents, 2);
> >
> > libevdev_uinput_destroy(uinput2);
> > - libinput_destroy(li);
> > + libinput_unref(li);
> >
> > open_func_count = 0;
> > close_func_count = 0;
> > @@ -704,7 +704,7 @@ START_TEST(path_add_device_suspend_resume_remove_device)
> > ck_assert_int_eq(nevents, 1);
> >
> > libevdev_uinput_destroy(uinput1);
> > - libinput_destroy(li);
> > + libinput_unref(li);
> >
> > open_func_count = 0;
> > close_func_count = 0;
> > @@ -790,7 +790,7 @@ START_TEST(path_seat_recycle)
> >
> > ck_assert(found == 1);
> >
> > - libinput_destroy(li);
> > + libinput_unref(li);
> >
> > libevdev_uinput_destroy(uinput);
> > }
> > diff --git a/test/pointer.c b/test/pointer.c
> > index 346e59b..7d5668f 100644
> > --- a/test/pointer.c
> > +++ b/test/pointer.c
> > @@ -292,7 +292,7 @@ START_TEST(pointer_seat_button_count)
> >
> > for (i = 0; i < num_devices; ++i)
> > litest_delete_device(devices[i]);
> > - libinput_destroy(libinput);
> > + libinput_unref(libinput);
> > }
> > END_TEST
> >
> > diff --git a/test/udev.c b/test/udev.c
> > index 6af2cb0..09c2a94 100644
> > --- a/test/udev.c
> > +++ b/test/udev.c
> > @@ -97,7 +97,7 @@ START_TEST(udev_create_seat0)
> > ck_assert(event != NULL);
> >
> > libinput_event_destroy(event);
> > - libinput_destroy(li);
> > + libinput_unref(li);
> > udev_unref(udev);
> > }
> > END_TEST
> > @@ -124,7 +124,7 @@ START_TEST(udev_create_empty_seat)
> > ck_assert(event == NULL);
> >
> > libinput_event_destroy(event);
> > - libinput_destroy(li);
> > + libinput_unref(li);
> > udev_unref(udev);
> > }
> > END_TEST
> > @@ -169,7 +169,7 @@ START_TEST(udev_added_seat_default)
> >
> > ck_assert(default_seat_found);
> >
> > - libinput_destroy(li);
> > + libinput_unref(li);
> > udev_unref(udev);
> > }
> > END_TEST
> > @@ -200,7 +200,7 @@ START_TEST(udev_double_suspend)
> > libinput_resume(li);
> >
> > libinput_event_destroy(event);
> > - libinput_destroy(li);
> > + libinput_unref(li);
> > udev_unref(udev);
> > }
> > END_TEST
> > @@ -231,7 +231,7 @@ START_TEST(udev_double_resume)
> > libinput_resume(li);
> >
> > libinput_event_destroy(event);
> > - libinput_destroy(li);
> > + libinput_unref(li);
> > udev_unref(udev);
> > }
> > END_TEST
> > @@ -289,7 +289,7 @@ START_TEST(udev_suspend_resume)
> > process_events_count_devices(li, &num_devices);
> > ck_assert_int_gt(num_devices, 0);
> >
> > - libinput_destroy(li);
> > + libinput_unref(li);
> > udev_unref(udev);
> > }
> > END_TEST
> > @@ -322,7 +322,7 @@ START_TEST(udev_device_sysname)
> > libinput_event_destroy(ev);
> > }
> >
> > - libinput_destroy(li);
> > + libinput_unref(li);
> > udev_unref(udev);
> > }
> > END_TEST
> > @@ -396,7 +396,7 @@ START_TEST(udev_seat_recycle)
> >
> > ck_assert(found == 1);
> >
> > - libinput_destroy(li);
> > + libinput_unref(li);
> > udev_unref(udev);
> > }
> > END_TEST
> > diff --git a/tools/event-debug.c b/tools/event-debug.c
> > index 34acfce..95e7628 100644
> > --- a/tools/event-debug.c
> > +++ b/tools/event-debug.c
> > @@ -163,7 +163,7 @@ open_device(struct libinput **li, const char *path)
> > device = libinput_path_add_device(*li, path);
> > if (!device) {
> > fprintf(stderr, "Failed to initialized device %s\n", path);
> > - libinput_destroy(*li);
> > + libinput_unref(*li);
> > return 1;
> > }
> >
> > @@ -478,7 +478,7 @@ main(int argc, char **argv)
> >
> > mainloop(li);
> >
> > - libinput_destroy(li);
> > + libinput_unref(li);
> > if (udev)
> > udev_unref(udev);
> >
> > diff --git a/tools/event-gui.c b/tools/event-gui.c
> > index e080ea8..d2b0428 100644
> > --- a/tools/event-gui.c
> > +++ b/tools/event-gui.c
> > @@ -467,7 +467,7 @@ main(int argc, char *argv[])
> >
> > gtk_main();
> >
> > - libinput_destroy(li);
> > + libinput_unref(li);
> > udev_unref(udev);
> >
> > return 0;
> > --
> > 1.9.1
More information about the wayland-devel
mailing list