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