svn commit: r230037 - head/lib/libutil

[Pawel Jakub Dawidek]

On Sat, Jan 14, 2012 at 09:59:27PM +1100, Bruce Evans wrote:
> 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.

I think you mixed space with tab. All the others have a tab after
#define and typedef. I fully agree this should be consistent.

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

I already pointed that out to Guy and also introduced him to concept of
pre-commit reviews:)

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

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.

-- 
Pawel Jakub Dawidek                       http://www.wheelsystems.com
FreeBSD committer                         http://www.FreeBSD.org
Am I Evil? Yes, I Am!                     http://tupytaj.pl
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 196 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/svn-src-all/attachments/20120114/c067d7a9/attachment.pgp