svn commit: r230037 - head/lib/libutil

Bruce Evans brde at optusnet.com.au
Sat Jan 14 21:02:35 UTC 2012


On Sat, 14 Jan 2012, Pawel Jakub Dawidek wrote:

> On Sat, Jan 14, 2012 at 09:59:27PM +1100, Bruce Evans wrote:
>> ...
>> It's good to declare mode_t, since pidfile_open() uses it and we want
>> to remove the dependency on <sys/param.h>.  However, this definition
>> doesn't follow KNF or the style of all the other typedef declarations
>> in the file, since all the others follow KNF and thus have a space
>> instead of a tab after #define and also after typedef.
>
> I think you mixed space with tab. All the others have a tab after
> #define and typedef. I fully agree this should be consistent.

Oops.

>>> -#ifdef _SYS_PARAM_H_
>>> int	pidfile_close(struct pidfh *_pfh);
>>> int	pidfile_fileno(const struct pidfh *_pfh);
>>> struct pidfh *
>>> 	pidfile_open(const char *_path, mode_t _mode, pid_t *_pidptr);
>>> int	pidfile_remove(struct pidfh *_pfh);
>>> int	pidfile_write(struct pidfh *_pfh);
>>> -#endif
>>
>> Now these are unsorted, since a separate section to hold them is not
>> needed.  It was used just to make the ifdef easier to read (we don't
>> want to split up the main list with blank lines around each ifdef, and
>> without such blank lines the ifdefs are harder to read).
>
> I'd prefer not to change that. All those functions are part of pidfile(3)
> API and it would be better, IMHO, to keep them together here too.

The functions have a unique prefix, so they are grouped nicely when sorted
into a long list.

While I'm here, I'll complain about the verboseness of that prefix :-).
Other APIs in the file mostly use short prefixes:
- kinfo_.  Should have been ki_ like its struct member names.  pidfile uses
   a good prefix for its struct member names too.
- properties_/property_.  Bad, like the rest of the API.
- uu_.  A weird nondescriptive name for serial device locking protocol.
   Is it from uucp?  But its weirdness makes it memorable, unlike a
   generic English word like `property'.  Better yet, I don't have to
   quote it here.
- f.  Stdio's prefix meaning `file'.  To fit indentifiers in 8 characters,
   it can't even have an underscore.
- pw_.  Old prefix/abbrieviation for `password'.  It's more readable than
   `password' once you are used to it.
- gr_.  Newer prefix for `group'.  More verbose than the g in gid.
- quota_.  At least the English word is short.

Just noticed some more disorder: the groups of the defines at the end
are in random (mostly historical) order (U*, HO*, F*, PW*, HN* (for
the last parameter of humanize_number()), HN* (for the second last
parameter...), HD*.

If the pidfile API had defines and if the API is kept in its own
section, its defines should be in that section.  Most of the other APIs
that have a man page are large enough to deserve the same treatment
if it is done for pidfile.  Some like dehumanize^Wscientificize^W
humanize_number() are larger although they have fewer functions, since
they have lots of defines.

Bruce


More information about the svn-src-all mailing list