Date: Tue, 4 Aug 2015 15:33:04 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Marcelo Araujo <araujo@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r286267 - head/usr.bin/ypcat Message-ID: <20150804150500.C896@besplex.bde.org> In-Reply-To: <201508040241.t742fFDi048172@repo.freebsd.org> References: <201508040241.t742fFDi048172@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 4 Aug 2015, Marcelo Araujo wrote: > Log: > Remove the 3rd clause of BSD LICENSE. > Sync the code with the OpenBSD version. > Small style(9) fix up. This has many style(9) fix downs. > Differential Revision: D3212 > Reviewed by: rodrigc, bapt > Obtained from: OpenBSD > Sponsored by: gandi.net > > Modified: > head/usr.bin/ypcat/ypcat.c > > Modified: head/usr.bin/ypcat/ypcat.c > ============================================================================== > --- head/usr.bin/ypcat/ypcat.c Tue Aug 4 02:34:51 2015 (r286266) > +++ head/usr.bin/ypcat/ypcat.c Tue Aug 4 02:41:14 2015 (r286267) > @@ -34,18 +33,20 @@ __FBSDID("$FreeBSD$"); > #include <sys/param.h> > #include <sys/types.h> > #include <sys/socket.h> > +#include <unistd.h> > +#include <string.h> > +#include <stdio.h> > +#include <stdlib.h> > +#include <ctype.h> > +#include <err.h> > > #include <rpc/rpc.h> > #include <rpc/xdr.h> > -#include <rpcsvc/yp_prot.h> > +#include <rpcsvc/yp.h> > #include <rpcsvc/ypclnt.h> > > -#include <ctype.h> > -#include <err.h> > -#include <stdio.h> > -#include <stdlib.h> > -#include <string.h> > -#include <unistd.h> Unsorting. The old organization was almost perfect except for including both sys/types and sys/param.h. Now there isn't even a blank line separating one pair of the 3 groupes of headers. > +void usage(void); > +int printit(u_long, char *, int, char *, int, void *); Unsorted declarations. Lost staticization. usage() was misplaced early in the file, so that since it was static, it didn't need a forward declaration, except style(9) requires one. usage() is its main example. > > static const struct ypalias { > char *alias, *name; > @@ -64,17 +65,18 @@ static const struct ypalias { > > static int key; > > -static void > +void Lost staticization. > usage(void) > { > - fprintf(stderr, "%s\n%s\n", > - "usage: ypcat [-kt] [-d domainname] mapname", > - " ypcat -x"); > + fprintf(stderr, > + "usage: ypcat [-kt] [-d domainname] mapname\n" > + " ypcat -x\n"); This fixes the indentation, but breaks the string using C90 string concatenation. style(9) doesn't specify using the "%s\n%s\n..." for printing multiple strings on multiple line by example, but almost all FreeBSD utilities including the old version of this one provide an example of this. > exit(1); > } > > -static int > -printit(unsigned long instatus, char *inkey, int inkeylen, char *inval, int invallen, void *dummy __unused) > +int > +printit(u_long instatus, char *inkey, int inkeylen, char *inval, int invallen, > + void *indata) Lost staticization. > { > if (instatus != YP_TRUE) > return (instatus); > @@ -87,31 +89,29 @@ printit(unsigned long instatus, char *in > int > main(int argc, char *argv[]) > { > - char *domainname = NULL; > + char *domain = NULL, *inmap; > struct ypall_callback ypcb; > - char *inmap; > - int notrans; > - int c, r; > + extern char *optarg; > + extern int optind; Extern declarations belong in a header file. Broken programs sometimes declare them directly to hide the bug that they don't include the correct header file, but this program still includes the correct header file (unistd.h>). Compilers are directed to warn about nested externs at a not very high WARNS. > + int notrans, c, r; Combining the declaration is good, but the variables are still unsorted. > u_int i; > > notrans = key = 0; > - > while ((c = getopt(argc, argv, "xd:kt")) != -1) > switch (c) { > case 'x': > - for (i = 0; i<sizeof ypaliases/sizeof ypaliases[0]; i++) > + for (i=0; i<sizeof ypaliases/sizeof ypaliases[0]; i++) Further away from KNF. The code is squished up to avoid line splitting, but the old version only squished 2 of 3 binary operations and that made the line length exactly 80. > ... > @@ -120,24 +120,29 @@ main(int argc, char *argv[]) > if (optind + 1 != argc) > usage(); > > - if (!domainname) > - yp_get_default_domain(&domainname); > + if (!domain) > + yp_get_default_domain(&domain); Still uses '!' on pointers. > > inmap = argv[optind]; > - for (i = 0; (!notrans) && i<sizeof ypaliases/sizeof ypaliases[0]; i++) > - if (strcmp(inmap, ypaliases[i].alias) == 0) > - inmap = ypaliases[i].name; > + if (!notrans) { > + for (i=0; i<sizeof ypaliases/sizeof ypaliases[0]; i++) Squished as above, but now the squishing is not even needed to avoid line-splitting. > case YPERR_YPBIND: > - errx(1, "not running ypbind"); > + errx(1, "ypcat: not running ypbind\n"); > + exit(1); Large regressions. Now the program name is printed twice, and the newline at the end of the message is printed twice, and there are 2 exits, with the new one unreachable. > default: > - errx(1, "no such map %s. reason: %s", inmap, yperr_string(r)); > + errx(1, "No such map %s. Reason: %s\n", > + inmap, yperr_string(r)); > + exit(1); As above, except the program name is not printed twice, plus - the first word after the program name is now capitalized. This is part of a sentence beginning with the program name and a colon. There are various style guides for capitilization after a colon. FreeBSD uses the following inconsistent rules in error messages: - don't capitalize - however, if the error message is from strerror(), then it capitalizes. Keep this. - the spacing and phrasing for the second sentence is not so good and is not improved by capitalization. It is more normal to use further punctuation in the same sentence. > } > exit(0); > } Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150804150500.C896>