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

[Martin Pitt]

Hello Lennart,

Lennart Poettering [2014-10-27 16:09 +0100]:
> > +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.

Done for the FILE*, but I kept the behaviour for the path as the
caller uses the path in the error message on failure. I added a
comment to clarify this.

> > -        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.

I'm aware of access(), but the code below actually uses the stat value
for timestamp comparison, so we really need stat() here.

I also changed the hwdb option to --usr now. It's not very
descriptive and wrong for /lib, but then again this is only really
being used in package maintainer scripts or custom system image
builds; users will hardly ever see this.

Thanks,

Martin

-- 
Martin Pitt                        | http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-udev-hwdb-Support-shipping-pre-compiled-database-in-.patch
Type: text/x-diff
Size: 9583 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/systemd-devel/attachments/20141028/c91466fe/attachment-0001.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.freedesktop.org/archives/systemd-devel/attachments/20141028/c91466fe/attachment-0001.sig>