svn commit: r286267 - head/usr.bin/ypcat

[Bruce Evans]

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