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

2014-10-27 Thread Lennart Poettering 
On Fri, 24.10.14 16:24, Martin Pitt (martin.p...@ubuntu.com) wrote:

 Hey Zbyszek,
 
 Zbigniew Jędrzejewski-Szmek [2014-10-24 19:45 +0200]:
  This enumeration is also used below... The definition should be shared.
  You might want to also consider using NULSTR_FOREACH for iteration.
 
 Ah, thanks for pointing that out.
 
  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.
 
 OK. Personally I prefer ret  0 and errno as that's the standard
 POSIX way and also allows using %m etc., but if strerror(-retval) is
 the systemd style, fair enough.

Yes, systemd style is the same as kernel style here: we return
negative-errno error codes, and thus strerror(-r) is the usual idiom
to get a human-readable string. Also see the CODING_STYLE text in the
repo for details on this.

  
 +static const char hwdb_bin_paths[] =
 +/etc/udev/hwdb.bin\0
 +UDEVLIBEXECDIR /hwdb.bin\0;
 +
 +
 +static int open_hwdb_bin(const char **path, FILE** f) {
 +const char* p;
 +
 +NULSTR_FOREACH(p, hwdb_bin_paths) {
 +*path = p;

Please do not write functions that clobber passed-in arguments on
failure. Please store any result in a temporary variable first, and
clobber the passed-in variables only on success.

 +*f = fopen(p, re);

same here.

I updated CODING_STYLE to clarify this, too.

 -if (stat(/etc/udev/hwdb.bin, st)  0)
 +
 +/* if hwdb.bin doesn't exist anywhere, we need to update */
 +NULSTR_FOREACH(p, hwdb_bin_paths) {
 +if (stat(p, st) = 0) {
 +found = true;
 +break;
 +}
 +}
 +if (!found)
  return true;

I'd prefer if you could use access(..., F_OK) here, for checking for
file existance, as stat() does substantially more than just checking
existance.

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

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

2014-10-25 Thread Zbigniew Jędrzejewski-Szmek 
On Fri, Oct 24, 2014 at 04:24:46PM -0400, Martin Pitt wrote:
 The updated patch fixes both now.
Looks fine and push-worthy. Two minor nitpicks below, feel free
to fix up before pushing or ignore as you see fit.

 From 1624d949c60a911f7bf578d0141c8cd0d98c54e9 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| 46 
 ++-
  src/udev/udevadm-hwdb.c   | 13 -
  units/systemd-udev-hwdb-update.service.in |  3 ++
  5 files changed, 66 insertions(+), 9 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 @@
  
paraThe content of all hwdb files is read by

 citerefentryrefentrytitleudevadm/refentrytitlemanvolnum8/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
 +  termoption--vendor/option/term
 +  listitem
 +paraPut 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
termoption-t/option/term

 termoption--test=replaceablestring/replaceable/option/term
listitem
 diff --git a/src/libudev/libudev-hwdb.c b/src/libudev/libudev-hwdb.c
 index 8fb7240..ed8cdff 100644
 --- a/src/libudev/libudev-hwdb.c
 +++ b/src/libudev/libudev-hwdb.c
 @@ -256,6 +256,25 @@ static int trie_search_f(struct udev_hwdb *hwdb, const 
 char *search) {
  return 0;
  }
  
 +static const char hwdb_bin_paths[] =
 +/etc/udev/hwdb.bin\0
 +UDEVLIBEXECDIR /hwdb.bin\0;
Actually we don't need to define a variable for this, a #define would be enough.

 +
 +
 +static int open_hwdb_bin(const char **path, FILE** f) {
 +const char* p;
 +
 +NULSTR_FOREACH(p, hwdb_bin_paths) {
 +*path = p;
 +*f = fopen(p, re);
 +if (*f)
 +return 0;
 +else if (errno != ENOENT)
 +return -errno;
 +}
 +return -errno;
 +}
 +
  /**
   * udev_hwdb_new:
   * @udev: udev library context
 @@ -266,6 +285,8 @@ 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;
 +int r;
 +const char *hwdb_bin_path;
  const char sig[] = HWDB_SIG;
  
  hwdb = new0(struct udev_hwdb, 1);
 @@ -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);
 -if (!hwdb-f) {
 -udev_dbg(udev, error reading /etc/udev/hwdb.bin: %m);
 +r = open_hwdb_bin(hwdb_bin_path, hwdb-f);
 +if (r  0) {
 +udev_dbg(udev, error reading %s: %s, hwdb_bin_path, 
 strerror(-r));
  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

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

2014-10-24 Thread Martin Pitt 
Hey Zbyszek,

Zbigniew Jędrzejewski-Szmek [2014-10-24 19:45 +0200]:
 This enumeration is also used below... The definition should be shared.
 You might want to also consider using NULSTR_FOREACH for iteration.

Ah, thanks for pointing that out.

 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.

OK. Personally I prefer ret  0 and errno as that's the standard
POSIX way and also allows using %m etc., but if strerror(-retval) is
the systemd style, fair enough.

The updated patch fixes both now.

Thanks,

Martin
-- 
Martin Pitt| http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)
From 1624d949c60a911f7bf578d0141c8cd0d98c54e9 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| 46 ++-
 src/udev/udevadm-hwdb.c   | 13 -
 units/systemd-udev-hwdb-update.service.in |  3 ++
 5 files changed, 66 insertions(+), 9 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 @@
 
   paraThe content of all hwdb files is read by
   citerefentryrefentrytitleudevadm/refentrytitlemanvolnum8/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
+  termoption--vendor/option/term
+  listitem
+paraPut 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
   termoption-t/option/term
   termoption--test=replaceablestring/replaceable/option/term
   listitem
diff --git a/src/libudev/libudev-hwdb.c b/src/libudev/libudev-hwdb.c
index 8fb7240..ed8cdff 100644
--- a/src/libudev/libudev-hwdb.c
+++ b/src/libudev/libudev-hwdb.c
@@ -256,6 +256,25 @@ static int trie_search_f(struct udev_hwdb *hwdb, const char *search) {
 return 0;
 }
 
+static const char hwdb_bin_paths[] =
+/etc/udev/hwdb.bin\0
+UDEVLIBEXECDIR /hwdb.bin\0;
+
+
+static int open_hwdb_bin(const char **path, FILE** f) {
+const char* p;
+
+NULSTR_FOREACH(p, hwdb_bin_paths) {
+*path = p;
+*f = fopen(p, re);
+if (*f)
+return 0;
+else if (errno != ENOENT)
+return -errno;
+}
+return -errno;
+}
+
 /**
  * udev_hwdb_new:
  * @udev: udev library context
@@ -266,6 +285,8 @@ 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;
+int r;
+const char *hwdb_bin_path;
 const char sig[] = HWDB_SIG;
 
 hwdb = new0(struct udev_hwdb, 1);
@@ -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);
-if (!hwdb-f) {
-udev_dbg(udev, error reading /etc/udev/hwdb.bin: %m);
+r = open_hwdb_bin(hwdb_bin_path,