From owner-svn-src-head@FreeBSD.ORG Sat Jan 14 18:29:11 2012 Return-Path: Delivered-To: svn-src-head@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id C61EC1065670; Sat, 14 Jan 2012 18:29:11 +0000 (UTC) (envelope-from pawel@dawidek.net) Received: from mail.dawidek.net (60.wheelsystems.com [83.12.187.60]) by mx1.freebsd.org (Postfix) with ESMTP id 37D3B8FC15; Sat, 14 Jan 2012 18:29:10 +0000 (UTC) Received: from localhost (89-73-195-149.dynamic.chello.pl [89.73.195.149]) by mail.dawidek.net (Postfix) with ESMTPSA id 8229DE23; Sat, 14 Jan 2012 19:29:09 +0100 (CET) Date: Sat, 14 Jan 2012 19:27:59 +0100 From: Pawel Jakub Dawidek To: Bruce Evans Message-ID: <20120114182758.GJ1694@garage.freebsd.pl> References: <201201122249.q0CMnaZe030200@svn.freebsd.org> <20120114204720.Q1458@besplex.bde.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="j+MD90OnwjQyWNYt" Content-Disposition: inline In-Reply-To: <20120114204720.Q1458@besplex.bde.org> X-OS: FreeBSD 9.0-CURRENT amd64 User-Agent: Mutt/1.5.21 (2010-09-15) Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Guy Helmer Subject: Re: svn commit: r230037 - head/lib/libutil X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 14 Jan 2012 18:29:12 -0000 --j+MD90OnwjQyWNYt Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, Jan 14, 2012 at 09:59:27PM +1100, Bruce Evans wrote: > On Thu, 12 Jan 2012, Guy Helmer wrote: >=20 > > Log: > > Move struct pidfh definition into pidfile.c, and leave a forward decla= ration > > 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. >=20 > This has some new style bugs, and I noticed some more old ones: >=20 > > Modified: head/lib/libutil/libutil.h > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D > > --- 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 > > + >=20 > 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. 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 >=20 > 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. 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 >=20 > 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. --=20 Pawel Jakub Dawidek http://www.wheelsystems.com FreeBSD committer http://www.FreeBSD.org Am I Evil? Yes, I Am! http://tupytaj.pl --j+MD90OnwjQyWNYt Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.14 (FreeBSD) iEYEARECAAYFAk8RyS4ACgkQForvXbEpPzTcAgCfU+II2xcUQ1KjDM7FywkOHoRj KiIAniUhLMhFGzl5s7ew9MVx+xES1tTA =Ehm9 -----END PGP SIGNATURE----- --j+MD90OnwjQyWNYt--