[PATCH libevdev 3/3] Add per-device log handlers

[Peter Hutterer]

On Mon, May 26, 2014 at 08:12:22AM +0200, David Herrmann wrote:
> Hi
> 
> On Mon, May 26, 2014 at 6:17 AM, Peter Hutterer
> <peter.hutterer at who-t.net> wrote:
> > 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.
> 
> I mentioned one significant advantage: We can make libevdev_uinput
> objects use the same logging infrastructure if needed.

we'll burn that bridge when we cross it :)

Cheers,
   Peter