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

[Lennart Poettering]

On Fri, 24.10.14 16:24, Martin Pitt (martin.pitt at 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