Skip site navigation (1)Skip section navigation (2)
Date:      Sat, 14 Jan 2012 21:59:27 +1100 (EST)
From:      Bruce Evans <brde@optusnet.com.au>
To:        Guy Helmer <ghelmer@FreeBSD.org>
Cc:        svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org
Subject:   Re: svn commit: r230037 - head/lib/libutil
Message-ID:  <20120114204720.Q1458@besplex.bde.org>
In-Reply-To: <201201122249.q0CMnaZe030200@svn.freebsd.org>
References:  <201201122249.q0CMnaZe030200@svn.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 12 Jan 2012, Guy Helmer wrote:

> Log:
>  Move struct pidfh definition into pidfile.c, and leave a forward declaration
>  for pidfh in libutil.h in its place.
>  This allows us to hide the contents of the pidfh structure, and also
>  allowed removal of the "#ifdef _SYS_PARAM_H" guard from around the
>  pidfile_* function prototypes.
>
>  Suggested by pjd.

This has some new style bugs, and I noticed some more old ones:

> Modified: head/lib/libutil/libutil.h
> ==============================================================================
> --- head/lib/libutil/libutil.h	Thu Jan 12 22:30:41 2012	(r230036)
> +++ head/lib/libutil/libutil.h	Thu Jan 12 22:49:36 2012	(r230037)
> @@ -48,6 +48,11 @@ typedef	__gid_t		gid_t;
> #define	_GID_T_DECLARED
> #endif
>
> +#ifndef _MODE_T_DECLARED
> +typedef __mode_t	mode_t;
> +#define _MODE_T_DECLARED
> +#endif
> +

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.

There are many other misfomatted spaces after #define's in this file:
- for the defines for the property API (PROPERTY*).  This API is ugly.
   I think it was mainly for sysinstall.  Maybe it can be removed.
   Other bugs in it include its man page source being named property.3
   but calling itself properties(3).  There is no link from property(3),
   so properties(3) doesn't exist.
- for the defines of UU*.  These have secondary style bugs:
   - space instead of tab[s] after the macro name.  This is worse, since
     it results in the macro bodies not being lined up.
   - bogus parentheses around macro bodies.  Such parentheses are ugly
     enough when they are necessary, but they are not necessary for 2
     of 9 cases here, since the macro body consists of a single token.
   - there is no comment before the UU* defines, unlike for all other
     groups of defines in the file.
- for the defines of UU*.  These have secondary style bugs:
   - bogus parentheses around macro bodies.  As above, but worse since
     none of the 4 macro bodies needs them.
- for the defines of PWSCAN*.
- for the defines of HN*.  The grouping for the 2 HN* sections is unclear
   The comment for the first group applies to both groups, while the
   comment for the second group is just unclear).  This is described
   clearly in the man page: the 2 groups are for 2 parameters.  In the
   header file, one of the parameters is misnamed and the other is not
   named.
- the defines for the idempotency ifdef and for FPARSELN* and HD* are
   actually formatted normally.

The section headers for flags definitions could be more uniform and
complete.  I like the style "/* Flags for <parameter name>: */".  No
need to mention the function name if the parameter name is unique
and/or if the macro names indicate the function or API name like
they should.

> -#ifdef _SYS_PARAM_H_
> -/* for pidfile.c */
> -struct pidfh {
> -	int	pf_fd;
> -	char	pf_path[MAXPATHLEN + 1];
> -	__dev_t	pf_dev;
> -	ino_t	pf_ino;
> -};
> -#endif

After moving this to pidfile.c, the man page seems to be bogus.  It
still says that <sys/param.h> is a prerequisite, but I think that
header was only needed for MAXPATHLEN here.  I checked that <libutil.h>
still compiles by itself (you had to add this typedef for that), and
that most or all of its manpages except pidfile(3) only say to include
it.

properties(3) still says that <sys/types.h> is a prerequisite, but that
hasn't been the case for 9 years or more, since other typedefs like the
one for mode_t were added in 2002.

realhostname(3) still says that <sys/types.h> and <netinet/in.h> are
prerequisites, buf obviously they aren't, since <libutil.h> compiles
by itself.  <netinet/in.h> was also cleaned up in 2002, so <sys/types.h>
is not a prerequisite for it.  realhostname() uses struct in_addr, but
this doesn't make <netinet/in.h> a prerequsite for using realhostname(),
since you can always use it with a null pointer, or more usefully, with
a pointer constructed somewhere else that knows the complete struct
in_addr.

pw_*(3) and PWSCAN* seem to be undocumented.  Almost everything else in
libutil.h is documented.

> -
> /* Avoid pulling in all the include files for no need */
> struct in_addr;
> struct kinfo_file;
> struct kinfo_proc;
> struct kinfo_vmentry;

The last 3 declarations are bogus.  The kinfo* structs aren't used in
parameter lists.  Thus forward-declaring them has no effect.

> +struct pidfh;

struct pidfh is a return type for one of the prototypes.  Unfortunately,
alphabetical ordering results other prototypes being declared first.
Thus this forward declaration is needed.

> @@ -174,14 +170,12 @@ struct group
> int	gr_tmp(int _mdf);
> #endif
>
> -#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).

Bruce



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120114204720.Q1458>