On Fri, Oct 24, 2014 at 11:52:39AM -0400, Martin Pitt wrote:
> Hello all,
>
> thanks Lennart for the detailled review! Took me a while to respond as
> I'm on a company sprint this week. I think I addressed all your
> points, plus fixing the unit condition. open_hwdb_bin() isn't the
> prettiest thing in the world after the change of immediately
> fopen()ing instead of just returning a path, but I don't have a good
> idea to beautify it.
>
> I tested ./udevadm hwdb --update with and without --vendor, and
> ./udevadm test-builtin hwdb in both cases.
>
> Thanks,
>
> Martin
>
> --
> Martin Pitt | http://www.piware.de
> Ubuntu Developer (www.ubuntu.com) | Debian Developer (www.debian.org)
> From 10f1faf25c2846964517c8d8d474e2b3f0b98bfc Mon Sep 17 00:00:00 2001
> From: Martin Pitt <martin.p...@ubuntu.com>
> Date: Fri, 17 Oct 2014 15:01:01 +0200
> Subject: [PATCH] udev hwdb: Support shipping pre-compiled database in system
> images
>
> In some cases it is preferable to ship system images with a pre-generated
> binary hwdb database, to avoid having to build it at runtime, avoid shipping
> the source hwdb files, or avoid storing large binary files in /etc.
>
> So if hwdb.bin does not exist in /etc/udev/, fall back to looking for it in
> UDEVLIBEXECDIR. This keeps the possibility to add files to /etc/udev/hwdb.d/
> and re-generating the database which trumps the one in /usr/lib.
>
> Add a new --vendor flag to "udevadm hwdb --update" which puts the database
> into UDEVLIBEXECDIR.
>
> Adjust systemd-udev-hwdb-update.service to not generate the file in /etc if we
> already have it in /usr.
> ---
> man/udev.xml | 4 +++-
> man/udevadm.xml | 9 +++++++++
> src/libudev/libudev-hwdb.c | 33
> +++++++++++++++++++++++++------
> src/udev/udevadm-hwdb.c | 13 +++++++++++-
> units/systemd-udev-hwdb-update.service.in | 3 +++
> 5 files changed, 54 insertions(+), 8 deletions(-)
>
> diff --git a/man/udev.xml b/man/udev.xml
> index d77cbb0..87c16c7 100644
> --- a/man/udev.xml
> +++ b/man/udev.xml
> @@ -766,7 +766,9 @@
>
> <para>The content of all hwdb files is read by
>
> <citerefentry><refentrytitle>udevadm</refentrytitle><manvolnum>8</manvolnum></citerefentry>
> - and compiled to a binary database located at
> <filename>/etc/udev/hwdb.bin</filename>.
> + and compiled to a binary database located at
> <filename>/etc/udev/hwdb.bin</filename>,
> + or alternatively <filename>/usr/lib/udev/hwdb.bin</filename> if you
> want ship the compiled
> + database in an immutable image.
> During runtime only the binary database is used.</para>
> </refsect1>
>
> diff --git a/man/udevadm.xml b/man/udevadm.xml
> index 749144d..571ef7d 100644
> --- a/man/udevadm.xml
> +++ b/man/udevadm.xml
> @@ -494,6 +494,15 @@
> </listitem>
> </varlistentry>
> <varlistentry>
> + <term><option>--vendor</option></term>
> + <listitem>
> + <para>Put the compiled database into
> <filename>/usr/lib/udev/hwdb.bin</filename> instead.
> + Use this if you want to ship a pre-compiled database in
> immutable system images, or
> + don't use <filename>/etc/udev/hwdb.d</filename> and want to
> avoid large binary files in
> + <filename>/etc</filename>.</para>
> + </listitem>
> + </varlistentry>
> + <varlistentry>
> <term><option>-t</option></term>
>
> <term><option>--test=<replaceable>string</replaceable></option></term>
> <listitem>
> diff --git a/src/libudev/libudev-hwdb.c b/src/libudev/libudev-hwdb.c
> index 8fb7240..bfb8a4d 100644
> --- a/src/libudev/libudev-hwdb.c
> +++ b/src/libudev/libudev-hwdb.c
> @@ -256,6 +256,26 @@ static int trie_search_f(struct udev_hwdb *hwdb, const
> char *search) {
> return 0;
> }
>
> +static FILE *open_hwdb_bin(const char **path) {
> + static const char* candidates[] = {
> + "/etc/udev/hwdb.bin",
> + UDEVLIBEXECDIR "/hwdb.bin",
> + NULL,
This enumeration is also used below... The definition should be shared.
You might want to also consider using NULSTR_FOREACH for iteration.
This function should have the prototype of 'int open_hwdb_bin(const char
**path, FILE *file)'
and return a proper value on error. Right now the return value is passed
through errno, which works but is inelegant.
> + };
> + FILE* f;
> + const char** c;
> +
> + for (c = candidates; *c; ++c) {
> + *path = *c;
> + f = fopen(*c, "re");
> + if (f)
> + return f;
> + else if (errno != ENOENT)
> + break;
> + }
> + return NULL;
> +}
> +
> /**
> * udev_hwdb_new:
> * @udev: udev library context
> @@ -267,6 +287,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;
>
> hwdb = new0(struct udev_hwdb, 1);
> if (!hwdb)
> @@ -275,30 +296,30 @@ _public_ struct udev_hwdb *udev_hwdb_new(struct udev
> *udev) {
> hwdb->refcount = 1;
> udev_list_init(udev, &hwdb->properties_list, true);
>
> - hwdb->f = fopen("/etc/udev/hwdb.bin", "re");
> + hwdb->f = open_hwdb_bin(&hwdb_bin_path);
> if (!hwdb->f) {
> - udev_dbg(udev, "error reading /etc/udev/hwdb.bin: %m");
> + udev_dbg(udev, "error reading %s: %m", hwdb_bin_path);
> udev_hwdb_unref(hwdb);
> return NULL;
> }
>
> if (fstat(fileno(hwdb->f), &hwdb->st) < 0 ||
> (size_t)hwdb->st.st_size < offsetof(struct trie_header_f,
> strings_len) + 8) {
> - udev_dbg(udev, "error reading /etc/udev/hwdb.bin: %m");
> + udev_dbg(udev, "error reading %s: %m", hwdb_bin_path);
> udev_hwdb_unref(hwdb);
> return NULL;
> }
>
> hwdb->map = mmap(0, hwdb->st.st_size, PROT_READ, MAP_SHARED,
> fileno(hwdb->f), 0);
> if (hwdb->map == MAP_FAILED) {
> - udev_dbg(udev, "error mapping /etc/udev/hwdb.bin: %m");
> + udev_dbg(udev, "error mapping %s: %m", hwdb_bin_path);
> udev_hwdb_unref(hwdb);
> return NULL;
> }
>
> if (memcmp(hwdb->map, sig, sizeof(hwdb->head->signature)) != 0 ||
> (size_t)hwdb->st.st_size != le64toh(hwdb->head->file_size)) {
> - udev_dbg(udev, "error recognizing the format of
> /etc/udev/hwdb.bin");
> + udev_dbg(udev, "error recognizing the format of %s",
> hwdb_bin_path);
> udev_hwdb_unref(hwdb);
> return NULL;
> }
> @@ -358,7 +379,7 @@ bool udev_hwdb_validate(struct udev_hwdb *hwdb) {
> return false;
> if (!hwdb->f)
> return false;
> - if (stat("/etc/udev/hwdb.bin", &st) < 0)
> + if (stat("/etc/udev/hwdb.bin", &st) < 0 && stat(UDEVLIBEXECDIR
> "/hwdb.bin", &st) < 0)
> return true;
> 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..017eda9 100644
> --- a/src/udev/udevadm-hwdb.c
> +++ b/src/udev/udevadm-hwdb.c
> @@ -536,14 +536,20 @@ 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");
> }
>
> static int adm_hwdb(struct udev *udev, int argc, char *argv[]) {
> + enum {
> + ARG_VENDOR = 0x100,
> + };
> +
> static const struct option options[] = {
> { "update", no_argument, NULL, 'u' },
> + { "vendor", no_argument, NULL, ARG_VENDOR },
> { "test", required_argument, NULL, 't' },
> { "root", required_argument, NULL, 'r' },
> { "help", no_argument, NULL, 'h' },
> @@ -551,6 +557,7 @@ static int adm_hwdb(struct udev *udev, int argc, char
> *argv[]) {
> };
> const char *test = NULL;
> const char *root = "";
> + const char *hwdb_bin_dir = "/etc/udev";
> bool update = false;
> struct trie *trie = NULL;
> int err, c;
> @@ -561,6 +568,9 @@ static int adm_hwdb(struct udev *udev, int argc, char
> *argv[]) {
> case 'u':
> update = true;
> break;
> + case ARG_VENDOR:
> + hwdb_bin_dir = UDEVLIBEXECDIR;
> + break;
> case 't':
> test = optarg;
> break;
> @@ -634,7 +644,8 @@ static int adm_hwdb(struct udev *udev, int argc, char
> *argv[]) {
> log_debug("strings dedup'ed: %8zu bytes (%8zu)",
> trie->strings->dedup_len,
> trie->strings->dedup_count);
>
> - if (asprintf(&hwdb_bin, "%s/etc/udev/hwdb.bin", root) < 0) {
> + hwdb_bin = strjoin(root, "/", hwdb_bin_dir, "/hwdb.bin",
> NULL);
> + if (!hwdb_bin) {
> rc = EXIT_FAILURE;
> goto out;
> }
> diff --git a/units/systemd-udev-hwdb-update.service.in
> b/units/systemd-udev-hwdb-update.service.in
> index cdbcd83..5b1f75d 100644
> --- a/units/systemd-udev-hwdb-update.service.in
> +++ b/units/systemd-udev-hwdb-update.service.in
> @@ -13,6 +13,9 @@ Conflicts=shutdown.target
> After=systemd-remount-fs.service
> Before=sysinit.target shutdown.target systemd-update-done.service
> ConditionNeedsUpdate=/etc
> +ConditionPathExists=|!@udevlibexecdir@/hwdb.bin
> +ConditionPathExists=|/etc/udev/hwdb.bin
> +ConditionDirectoryNotEmpty=|/etc/udev/hwdb.d/
>
> [Service]
> Type=oneshot
Zbyszek
_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
