[systemd-devel] [RFC 06/12] gfx: add keyboard layer

[David Herrmann]

Hi

On Wed, Nov 27, 2013 at 11:17 PM, Lennart Poettering
<lennart at poettering.net> wrote:
> On Wed, 27.11.13 19:48, David Herrmann (dh.herrmann at gmail.com) wrote:
>
>> +
>> +enum {
>> +        SD_GFX__LED_NUML,
>> +        SD_GFX__LED_CAPSL,
>> +        SD_GFX__LED_SCROLLL,
>> +        SD_GFX__LED_COUNT,
>> +};
>
> Double underscores?

Yes.

>> +
>> +struct sd_gfx_kbd {
>> +        char *name;
>> +        struct libevdev *evdev;
>> +        int fd;
>> +        struct xkb_state *state;
>> +        sd_event *ev;
>> +        sd_event_source *fd_source;
>> +        sd_event_source *timer;
>> +        sd_gfx_kbd_event_fn event_fn;
>> +        void *data;
>> +        void *fn_data;
>> +
>> +        unsigned int delay;
>> +        unsigned int rate;
>> +
>> +        unsigned int sym_count;
>> +        sd_gfx_kbd_event event;
>> +        sd_gfx_kbd_event repeat;
>> +
>> +        xkb_mod_index_t xkb_mods[SD_GFX__MOD_COUNT];
>> +        xkb_led_index_t xkb_leds[SD_GFX__LED_COUNT];
>> +
>> +        unsigned int awake : 1;
>> +        unsigned int need_sync : 1;
>> +        unsigned int repeating : 1;
>
> If these are bools, then make them bools! C99 bools (i.e. the type
> "bool" from stdbool.h) are awesome for bit fields! [ Please use C99 bools
> everywhere in internal code, and "int" as bool type for public APIs,
> since that's what pre-C99 code usually did, despite the stupidity this
> results in when people use bit fields ]

You can use "_Bool/bool" for bitfields? Didn't know that. I always use
"bool" for boolean values, I only thought they don't work for
bitfields. Will fix that up.

Thanks
David

>> +        if (r < 0) {
>> +                if (r == -EACCES)
>> +                        log_debug("kbd %s: EACCES on led-update", kbd->name);
>> +                else
>> +                        log_error("kbd %s: cannot update leds: %d",
>> +                                  kbd->name, r);
>
> Please do not log from libraries. (Well, except when that's the primary
> purpose of your function, or when you do so on LOG_DEBUG level, since
> that is not visible normally anyway.)
>
> Lennart
>
> --
> Lennart Poettering, Red Hat