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
