On Fri, 17.10.14 15:10, Martin Pitt (martin.p...@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 _______________________________________________ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel