[systemd-devel] [PATCH] udev hwdb: Support shipping pre-compiled database in system images

[Lennart Poettering]

On Fri, 17.10.14 15:10, Martin Pitt (martin.pitt at ubuntu.com) wrote:

Looks generally OK (as discussed in Düsseldorf). I'll leave this for
Kay to merge though, as udev is more his thing. Kay!

> --- a/src/libudev/libudev-hwdb.c
> +++ b/src/libudev/libudev-hwdb.c
> @@ -256,6 +256,13 @@ static int trie_search_f(struct udev_hwdb *hwdb, const char *search) {
>          return 0;
>  }
>  
> +static const char *get_hwdb_bin_path(void) {
> +        if (access("/etc/udev/hwdb.bin", F_OK) >= 0)
> +                return "/etc/udev/hwdb.bin";
> +        else
> +                return UDEVLIBEXECDIR "/hwdb.bin";
> +}
> +

No! Please always try to open() the files right-away, let's not do
access() in advance, since that is not only slower, but also
inherently racy.

Hence, please fopen() the version in /etc, and if that fails with
ENOENT fopen() the version in /usr, and give up otherwise.

>  /**
>   * udev_hwdb_new:
>   * @udev: udev library context
> @@ -267,6 +274,7 @@ static int trie_search_f(struct udev_hwdb *hwdb, const char *search) {
>  _public_ struct udev_hwdb *udev_hwdb_new(struct udev *udev) {
>          struct udev_hwdb *hwdb;
>          const char sig[] = HWDB_SIG;
> +        const char *hwdb_bin_path = get_hwdb_bin_path();

Also, we nowadays try to avoid declaring variables and invoking
functions in the same line...

>          if (!hwdb->f)
>                  return false;
> -        if (stat("/etc/udev/hwdb.bin", &st) < 0)
> +        if (stat(get_hwdb_bin_path(), &st) < 0)
>                  return true;

In this case too, please stat() first one path, and on ENOENT check
the other one, don't do access() before...

>          if (timespec_load(&hwdb->st.st_mtim) != timespec_load(&st.st_mtim))
>                  return true;
> diff --git a/src/udev/udevadm-hwdb.c b/src/udev/udevadm-hwdb.c
> index 64273fb..2127213 100644
> --- a/src/udev/udevadm-hwdb.c
> +++ b/src/udev/udevadm-hwdb.c
> @@ -536,6 +536,7 @@ static int import_file(struct udev *udev, struct trie *trie, const char *filenam
>  static void help(void) {
>          printf("Usage: udevadm hwdb OPTIONS\n"
>                 "  -u,--update          update the hardware database\n"
> +               "  --vendor             generate in " UDEVLIBEXECDIR " instead of /etc/udev\n"
>                 "  -t,--test=MODALIAS   query database and print result\n"
>                 "  -r,--root=PATH       alternative root path in the filesystem\n"
>                 "  -h,--help\n\n");
> @@ -544,6 +545,7 @@ static void help(void) {
>  static int adm_hwdb(struct udev *udev, int argc, char *argv[]) {
>          static const struct option options[] = {
>                  { "update", no_argument,       NULL, 'u' },
> +                { "vendor", no_argument,       NULL, 'V' },

Please don#t introduce a short version of this, just define an enum
with value beyond 255 for this.

>  
> -                if (asprintf(&hwdb_bin, "%s/etc/udev/hwdb.bin", root) < 0) {
> +                if (asprintf(&hwdb_bin, "%s/%s/hwdb.bin", root, hwdb_bin_dir) < 0) {
>                          rc = EXIT_FAILURE;

Not that it would matter much in this case, but you get an extra bonus
point if you replace this asprintf() invocation with strjoin() while
you are at it, it's much nicer (and faster) as long as we just
concatenate a few strings!

Thanks!


Lennart

-- 
Lennart Poettering, Red Hat