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

[David Herrmann]

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.

Thanks
David