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
pgpy9R6zScj8o.pgp
Description: PGP signature
