[PATCH libevdev 3/3] Add per-device log handlers
Peter Hutterer
peter.hutterer at who-t.net
Sun May 25 21:17:37 PDT 2014
On Sun, May 25, 2014 at 02:27:20PM +0200, David Herrmann wrote:
> Hi
>
> On Mon, May 19, 2014 at 6:01 AM, Peter Hutterer
> <peter.hutterer at who-t.net> wrote:
> > The global log handler isn't a good choice for a low-level library. In the
> > caser of the X server, both evdev and synaptics are now using the libevdev but
> > are loaded from the same server process. Thus, there's only one log handler,
> > but evdev and synaptics don't talk to each other (a bit childish, I know).
> >
> > Add a per-device log handler that overrides the global log handler, and fall
> > back to the global log handler if no device log handler is set. The log
> > macros take care of that automatically, especially as we can't do per-device
> > log handlers for the uinput code.
> >
> > This also makes libevdev_new_from_fd() a bit less useful since we can't set
> > the log handler beforehand.
>
> Multi-function constructors have never been a good idea, imho. I a
> constructor does more than just initialize memory, you always end up
> with problematic log-handling and error-recovery. Therefore, I think
> your patch is fine.
>
> > Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> > ---
> > Not really happy with this, but I'm yet to find a better solution. Comments
> > appreciated.
> >
> > libevdev/libevdev-int.h | 31 +++++++--------
> > libevdev/libevdev-uinput.c | 6 +--
> > libevdev/libevdev.c | 85 +++++++++++++++++++++++++++++++----------
> > libevdev/libevdev.h | 95 +++++++++++++++++++++++++++++++++++++++++++---
> > libevdev/libevdev.sym | 8 ++++
> > test/test-libevdev-init.c | 74 ++++++++++++++++++++++++++++++++++++
> > 6 files changed, 255 insertions(+), 44 deletions(-)
> >
> > diff --git a/libevdev/libevdev-int.h b/libevdev/libevdev-int.h
> > index f587e76..5b6d7de 100644
> > --- a/libevdev/libevdev-int.h
> > +++ b/libevdev/libevdev-int.h
> > @@ -106,31 +106,32 @@ struct libevdev {
> > unsigned long *tracking_id_changes;
> > size_t tracking_id_changes_sz; /* in bytes */
> > } mt_sync;
> > -};
> >
> > -struct logdata {
> > - enum libevdev_log_priority priority; /** minimum logging priority */
> > - libevdev_log_func_t handler; /** handler function */
> > - void *userdata; /** user-defined data pointer */
> > + struct {
> > + enum libevdev_log_priority priority; /** minimum logging priority */
> > + libevdev_device_log_func_t handler; /** handler function */
> > + void *userdata; /** user-defined data pointer */
> > + } log;
> > };
>
> Why don't you keep "struct logdata" so we can add one to uinput-code,
> too? For now uinput uses the global logger as it doesn't have a
> libevdev object. If you directly pass "struct logdata" instead of the
> libevdev object, we can use the same log-handlers for everything.
[...]
> > diff --git a/libevdev/libevdev.c b/libevdev/libevdev.c
> > index a316831..1e472a5 100644
> > --- a/libevdev/libevdev.c
> > +++ b/libevdev/libevdev.c
> > @@ -36,6 +36,12 @@
> >
> > #define MAXEVENTS 64
> >
> > +struct logdata {
> > + enum libevdev_log_priority priority; /** minimum logging priority */
> > + libevdev_log_func_t handler; /** handler function */
> > + void *userdata; /** user-defined data pointer */
> > +};
>
> Ehhh, what? You keep that structure for the global log-data, but use a
> separate one for "struct libevdev"? Why don't you use the same for
> both?
the log function signatures differ between normal and per-device logging,
hence the two different structs. I'll send out a patch to re-use the struct
but I don't see much benefit in passing the logdata vs the device.
> The log functions then get a "struct logdata*" pointer. If it's NULL,
> they use the global log_data, if not, they use it directly. Or what am
> I missing?
>
[...]
> > /**
> > + * @ingroup logging
> > + *
> > + * Logging function called by library-internal logging for a specific
> > + * libevdev context. This function is expected to treat its input like
> > + * printf would.
> > + *
> > + * @param dev The evdev device
> > + * @param priority Log priority of this message
> > + * @param data User-supplied data pointer (see libevdev_set_log_function())
> > + * @param file libevdev source code file generating this message
> > + * @param line libevdev source code line generating this message
> > + * @param func libevdev source code function generating this message
> > + * @param format printf-style format string
> > + * @param args List of arguments
> > + *
> > + * @see libevdev_set_log_function
> > + */
> > +typedef void (*libevdev_device_log_func_t)(const struct libevdev *dev,
> > + enum libevdev_log_priority priority,
> > + void *data,
> > + const char *file, int line,
> > + const char *func,
> > + const char *format, va_list args)
> > + LIBEVDEV_ATTRIBUTE_PRINTF(7, 0);
> > +
> > +/**
> > + * @ingroup logging
> > + *
> > + * Set a printf-style logging handler for library-internal logging for this
> > + * device context. The default logging function is NULL, i.e. the global log
> > + * handler is invoked. If a context-specific log handler is set, the global
> > + * log handler is not invoked for this device.
> > + *
> > + * @note This log function applies for this device context only, even if
> > + * another context exists for the same fd.
> > + *
> > + * @param dev The evdev device
> > + * @param logfunc The logging function for this device. If NULL, the current
> > + * logging function is unset and logging falls back to the global log
> > + * handler, if any.
> > + * @param priority Minimum priority to be printed to the log.
> > + * @param data User-specific data passed to the log handler.
> > + *
> > + * @note This function may be called before libevdev_set_fd().
> > + */
> > +void libevdev_set_device_log_function(struct libevdev *dev,
> > + libevdev_device_log_func_t logfunc,
> > + enum libevdev_log_priority priority,
> > + void *data);
> > +
>
> I like this interface! (except for the things mentioned)
>
> One thing: Please always keep the per-log 'data' field. In case we
> ever add libevdev_set/get_data() helpers to set per-context data, we
> should *not* mix this up with per-logging data. Otherwise,
> log-forwarding is getting problematic.
I've got no intention of removing the log data field, so that should be
fine. as for user-data, yeah, if it's needed we can add it but so far I
haven't seen the need for it. it's something that's generally needed where
callbacks are involved but libevdev is (aside from the log handler)
callback-free.
thanks for the review, much appreciated
Cheers,
Peter
>
> So if you fix up the "struct logdata" mess, I can give you a
> reviewed-by. So far:
>
> Acked-by: David Herrmann <dh.herrmann at gmail.com>
>
> Thanks
> David
>
> > +/**
> > * @ingroup init
> > */
> > enum libevdev_grab_mode {
> > diff --git a/libevdev/libevdev.sym b/libevdev/libevdev.sym
> > index ef4f06b..228e555 100644
> > --- a/libevdev/libevdev.sym
> > +++ b/libevdev/libevdev.sym
> > @@ -102,3 +102,11 @@ global:
> > local:
> > *;
> > };
> > +
> > +LIBEVDEV_1_3 {
> > +global:
> > + libevdev_set_device_log_function;
> > +
> > +local:
> > + *;
> > +} LIBEVDEV_1;
> > diff --git a/test/test-libevdev-init.c b/test/test-libevdev-init.c
> > index 61fea4b..d45a319 100644
> > --- a/test/test-libevdev-init.c
> > +++ b/test/test-libevdev-init.c
> > @@ -271,6 +271,79 @@ START_TEST(test_log_data)
> > }
> > END_TEST
> >
> > +struct libevdev *devlogdata;
> > +static int dev_log_fn_called = 0;
> > +static void devlogfunc(const struct libevdev *dev,
> > + enum libevdev_log_priority priority,
> > + void *data,
> > + const char *file, int line, const char *func,
> > + const char *f, va_list args)
> > +{
> > + ck_assert(dev == data);
> > + dev_log_fn_called++;
> > +}
> > +
> > +START_TEST(test_device_log_init)
> > +{
> > + struct libevdev *dev = NULL;
> > + enum libevdev_log_priority old;
> > +
> > + old = libevdev_get_log_priority();
> > + libevdev_set_log_priority(LIBEVDEV_LOG_DEBUG);
> > + libevdev_set_log_function(logfunc, logdata);
> > +
> > + /* error for NULL device */
> > + libevdev_set_device_log_function(NULL, NULL,
> > + LIBEVDEV_LOG_ERROR, NULL);
> > + ck_assert_int_eq(log_fn_called, 1);
> > +
> > + /* error for NULL device */
> > + libevdev_set_device_log_function(NULL, devlogfunc,
> > + LIBEVDEV_LOG_ERROR, NULL);
> > + ck_assert_int_eq(log_fn_called, 2);
> > +
> > + log_fn_called = 0;
> > +
> > + dev = libevdev_new();
> > + ck_assert(dev != NULL);
> > +
> > + libevdev_set_device_log_function(dev, NULL,
> > + LIBEVDEV_LOG_ERROR, NULL);
> > +
> > + /* libevdev_next_event(dev, LIBEVDEV_READ_FLAG_NORMAL, NULL) should
> > + trigger a log message. */
> > +
> > + /* expect global handler triggered */
> > + libevdev_next_event(dev, LIBEVDEV_READ_FLAG_NORMAL, NULL);
> > + ck_assert_int_eq(log_fn_called, 1);
> > + ck_assert_int_eq(dev_log_fn_called, 0);
> > +
> > + /* expect device handler triggered */
> > + libevdev_set_device_log_function(dev, devlogfunc,
> > + LIBEVDEV_LOG_ERROR, dev);
> > + libevdev_next_event(dev, LIBEVDEV_READ_FLAG_NORMAL, NULL);
> > + ck_assert_int_eq(log_fn_called, 1);
> > + ck_assert_int_eq(dev_log_fn_called, 1);
> > +
> > + /* device handler set, but priority filters. don't expect any log
> > + handler to be called.
> > + we don't have any log msgs > ERROR at the moment, so test it by
> > + setting an invalid priority. */
> > + libevdev_set_device_log_function(dev, devlogfunc,
> > + LIBEVDEV_LOG_ERROR - 1, dev);
> > + libevdev_next_event(dev, LIBEVDEV_READ_FLAG_NORMAL, NULL);
> > + ck_assert_int_eq(log_fn_called, 1);
> > + ck_assert_int_eq(dev_log_fn_called, 1);
> > +
> > + libevdev_free(dev);
> > +
> > + log_fn_called = 0;
> > + libevdev_set_log_priority(old);
> > + libevdev_set_log_function(test_logfunc_abort_on_error, NULL);
> > +
> > +}
> > +END_TEST
> > +
> > START_TEST(test_device_init)
> > {
> > struct uinput_device* uidev;
> > @@ -477,6 +550,7 @@ libevdev_init_test(void)
> > tcase_add_test(tc, test_log_set_get_priority);
> > tcase_add_test(tc, test_log_default_priority);
> > tcase_add_test(tc, test_log_data);
> > + tcase_add_test(tc, test_device_log_init);
> > suite_add_tcase(s, tc);
> >
> > tc = tcase_create("device fd init");
> > --
> > 1.9.0
> >
> > _______________________________________________
> > Input-tools mailing list
> > Input-tools at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/input-tools
More information about the Input-tools
mailing list