From owner-svn-src-all@FreeBSD.ORG Sat Jan 14 10:59:33 2012 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 63880106566B; Sat, 14 Jan 2012 10:59:33 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail06.syd.optusnet.com.au (mail06.syd.optusnet.com.au [211.29.132.187]) by mx1.freebsd.org (Postfix) with ESMTP id 95F438FC0C; Sat, 14 Jan 2012 10:59:32 +0000 (UTC) Received: from c211-30-171-136.carlnfd1.nsw.optusnet.com.au (c211-30-171-136.carlnfd1.nsw.optusnet.com.au [211.30.171.136]) by mail06.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id q0EAxRMf010903 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sat, 14 Jan 2012 21:59:28 +1100 Date: Sat, 14 Jan 2012 21:59:27 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Guy Helmer In-Reply-To: <201201122249.q0CMnaZe030200@svn.freebsd.org> Message-ID: <20120114204720.Q1458@besplex.bde.org> References: <201201122249.q0CMnaZe030200@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r230037 - head/lib/libutil X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 14 Jan 2012 10:59:33 -0000 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 . 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 : */". 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 is a prerequisite, but I think that header was only needed for MAXPATHLEN here. I checked that 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 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 and are prerequisites, buf obviously they aren't, since compiles by itself. was also cleaned up in 2002, so is not a prerequisite for it. realhostname() uses struct in_addr, but this doesn't make 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