[systemd-devel] [PATCH 2/6] udev: builtin-keyboard: immediately EVIOCSKEYCODE when we have a pair

[David Herrmann]

Hi

On Mon, Mar 23, 2015 at 2:30 AM, Peter Hutterer
<peter.hutterer at who-t.net> wrote:
> Rather than building a map and looping through the map, immediately call the
> ioctl when we have a successfully parsed property.
>
> This has a side-effect: before the maximum number of ioctls was limited to the
> size of the map (1024), now it is unlimited.
> ---
>  src/udev/udev-builtin-keyboard.c | 46 +++++++++++++++++-----------------------
>  1 file changed, 19 insertions(+), 27 deletions(-)
>
> diff --git a/src/udev/udev-builtin-keyboard.c b/src/udev/udev-builtin-keyboard.c
> index b7c8791..b3396d5 100644
> --- a/src/udev/udev-builtin-keyboard.c
> +++ b/src/udev/udev-builtin-keyboard.c
> @@ -67,10 +67,10 @@ static int builtin_keyboard(struct udev_device *dev, int argc, char *argv[], boo
>          struct {
>                  unsigned scan;
>                  unsigned key;
> -        } map[1024];
> -        unsigned map_count = 0;
> +        } map;
>          unsigned release[1024];
>          unsigned release_count = 0;
> +        _cleanup_close_ int fd = -1;
>          const char *node;
>
>          node = udev_device_get_devnode(dev);
> @@ -125,37 +125,29 @@ static int builtin_keyboard(struct udev_device *dev, int argc, char *argv[], boo
>                          }
>                  }
>
> -                map[map_count].scan = scancode;
> -                map[map_count].key = keycode_num;
> -                if (map_count < ELEMENTSOF(map)-1)
> -                        map_count++;
> -        }
> -
> -        if (map_count > 0 || release_count > 0) {
> -                int fd;
> -                unsigned i;
> -
> -                fd = open(node, O_RDWR|O_CLOEXEC|O_NONBLOCK|O_NOCTTY);
> -                if (fd < 0) {
> -                        log_error_errno(errno, "Error, opening device '%s': %m", node);
> -                        return EXIT_FAILURE;
> +                if (fd == -1) {
> +                        fd = open(node, O_RDWR|O_CLOEXEC|O_NONBLOCK|O_NOCTTY);
> +                        if (fd < 0) {
> +                                log_error_errno(errno, "Error, opening device '%s': %m", node);
> +                                return EXIT_FAILURE;
> +                        }
>                  }
>
> -                /* install list of map codes */
> -                for (i = 0; i < map_count; i++) {
> -                        log_debug("keyboard: mapping scan code %d (0x%x) to key code %d (0x%x)",
> -                                  map[i].scan, map[i].scan, map[i].key, map[i].key);
> -                        if (ioctl(fd, EVIOCSKEYCODE, &map[i]) < 0)
> -                                log_error_errno(errno, "Error calling EVIOCSKEYCODE on device node '%s' (scan code 0x%x, key code %d): %m", node, map[i].scan, map[i].key);
> -                }
> +                map.scan = scancode;
> +                map.key = keycode_num;
> +
> +                log_debug("keyboard: mapping scan code %d (0x%x) to key code %d (0x%x)",
> +                          map.scan, map.scan, map.key, map.key);
>
> -                /* install list of force-release codes */
> -                if (release_count > 0)
> -                        install_force_release(dev, release, release_count);
> +                if (ioctl(fd, EVIOCSKEYCODE, &map) < 0)

Man, this ioctl is ugly. Why has there never been a payload-struct? We
might convert this to EVIOCSKEYCODE_V2 some day.. but unrelated to
this change.

> +                        log_error_errno(errno, "Error calling EVIOCSKEYCODE on device node '%s' (scan code 0x%x, key code %d): %m", node, map.scan, map.key);
>

Weird empty line left?

Looks all good to me. Please push!
David

> -                close(fd);
>          }
>
> +        /* install list of force-release codes */
> +        if (release_count > 0)
> +                install_force_release(dev, release, release_count);
> +
>          return EXIT_SUCCESS;
>  }
>
> --
> 2.3.2
>