Re: [patch] give gpioctl.c some love

[Marc Balmer <marc%msys.ch@localhost>]

Am 22.08.15 um 02:22 schrieb Timo Buhrmester:

> I was troubleshooting a GPIO problem and came across usr.sbin/gpioctl/gpioctl.c. The code is well-written, but I figured it needs some love, readability-wise.  It uses unnecessarily deep nesting while trying to stay witin 80 characters per line at tabstop 8; I refactored parts of it to eliminate that problem (maintaining the 80-colums invariant, of course).
> 
> Apart from that, a few other things were changed here and there:
>  - const correctness
>  - meaningful prototypes
>  - avoidance of redundancies
>  - fix (and isolate) converting a string to `unsigned` (it errorneously accepted some negative values before, and didn't zero/restore errno (not that it has restore, but it certainly is good manners to do)
>  - declutter main() a bit
> 
> Overall, a patch that removes (slightly) more lines than it adds, so it must be good, no? :)
> I'll inline it here for easier commenting, if there are objections.

as the original author of gpioctl I add my comments inline....  I am
mostly against this patch, but to make it clear:  that is not against
you, but against the patch ;)

Imo, this patch does not solve a problem, but creates new ones, maybe...
 The constness that you sprinkle is definitely not needed, only makes
the code cluttered imo.


> 
> 
> Cheers,
> 
> Timo Buhrmester
> 
> P.S.: gpioctl's functionality or behavior is not changed, apart from the integer parsing issue (a missing `lval < 0`)
> 
> 
> ----------8<---------------------------------------------------------
> 
> --- usr.sbin/gpioctl/gpioctl.c.orig
> +++ usr.sbin/gpioctl/gpioctl.c
> @@ -36,17 +36,21 @@
>  #include <string.h>
>  #include <unistd.h>
>  
> -static char *dev;
> +static const char *dev;
>  static int devfd = -1;
>  static int quiet = 0;
>  static int state = 0;
>  
>  static void getinfo(void);
> -static void gpioread(int, char *);
> -static void gpiowrite(int, char *, int);
> -static void gpioset(int pin, char *name, int flags, char *alias);
> -static void gpiounset(int pin, char *name);
> -static void devattach(char *, int, uint32_t, uint32_t);
> +static void gpioread(int pin, const char *gp_name);
> +static void gpiowrite(int pin, const char *gp_name, int value);
> +static void gpioset(int pin, const char *name, int flags, const char *alias);
> +static void gpiounset(int pin, const char *name);
> +
> +static unsigned get_uint_or_die(const char *str, const char *name);
> +static void devattach(const char *driver, const char *offset,
> +    const char *mask, const char *flags);
> +

This is wrong.  We don't use variable names in function prototypes for
good reason.

>  __dead static void usage(void);
>  
>  static const struct bitstr {
> @@ -67,19 +71,14 @@ static const struct bitstr {
>  	{ 0, NULL },
>  };
>  
> +

What does it gain you to add an empty line here?

>  int
>  main(int argc, char *argv[])
>  {
>  	const struct bitstr *bs;
>  	int pin, ch, n, fl = 0, value = 0;
>  	const char *errstr;
> -	char *ep;
> -	int ga_offset = -1;
> -	uint32_t ga_mask = 0;
> -	uint32_t ga_flags = 0;
> -	long lval;
>  	char *nam = NULL;
> -	char *flags;
>  	char devn[32];
>  
>  	while ((ch = getopt(argc, argv, "qs")) != -1)
> @@ -115,84 +114,54 @@ main(int argc, char *argv[])
>  	}
>  
>  	if (!strcmp(argv[1], "attach")) {
> -		char *driver, *offset, *mask;
> -
>  		if (argc != 5 && argc != 6)
>  			usage();
>  
> -		driver = argv[2];
> -		offset = argv[3];
> -		mask = argv[4];
> -		flags = argc == 6 ? argv[5] : NULL;
> -
> -		ga_offset = strtonum(offset, 0, INT_MAX, &errstr);
> -		if (errstr)
> -			errx(EXIT_FAILURE, "offset is %s: %s", errstr, offset);
> -		lval = strtol(mask, &ep, 0);
> -		if (*mask == '\0' || *ep != '\0')
> -			errx(EXIT_FAILURE, "invalid mask (not a number)");
> -		if ((errno == ERANGE && (lval == LONG_MAX
> -		    || lval == LONG_MIN)) || (unsigned long)lval > UINT_MAX)
> -			errx(EXIT_FAILURE, "mask out of range");
> -		ga_mask = lval;
> -		if (flags != NULL) {
> -			lval = strtol(flags, &ep, 0);
> -			if (*flags == '\0' || *ep != '\0')
> -				errx(EXIT_FAILURE,
> -				    "invalid flag locator (not a number)");
> -			if ((errno == ERANGE && (lval == LONG_MAX
> -			    || lval == LONG_MIN))
> -			    || (unsigned long)lval > UINT_MAX)
> -				errx(EXIT_FAILURE, "flag locator out of range");
> -
> -			ga_flags = lval;
> -		}
> -		devattach(driver, ga_offset, ga_mask, ga_flags);
> +		devattach(argv[2], argv[3], argv[4], argv[5]);
> +
>  		return EXIT_SUCCESS;
> -	} else {
> -		char *nm = NULL;
> -
> -		/* expecting a pin number or name */
> -		pin = strtonum(argv[1], 0, INT_MAX, &errstr);
> -		if (errstr)
> -			nm = argv[1];	/* try named pin */
> -		if (argc > 2) {
> -			if (!strcmp(argv[2], "set")) {
> -				for (n = 3; n < argc; n++) {
> -					for (bs = pinflags; bs->string != NULL;
> -					     bs++) {
> -						if (!strcmp(argv[n],
> -						    bs->string)) {
> -							fl |= bs->mask;
> -							break;
> -						}
> -					}
> -					if (bs->string == NULL)
> -						nam = argv[n];
> -				}
> -				gpioset(pin, nm, fl, nam);
> -			} else if (!strcmp(argv[2], "unset"))
> -				gpiounset(pin, nm);
> -			else {
> -				value = strtonum(argv[2], INT_MIN, INT_MAX,
> -				   &errstr);
> -				if (errstr) {
> -					if (!strcmp(argv[2], "on"))
> -						value = GPIO_PIN_HIGH;
> -					else if (!strcmp(argv[2], "off"))
> -						value = GPIO_PIN_LOW;
> -					else if (!strcmp(argv[2], "toggle"))
> -						value = 2;
> -					else
> -						errx(EXIT_FAILURE,
> -						    "%s: invalid value",
> -						    argv[2]);
> +	}
> +
> +	char *nm = NULL;
> +	/* expecting a pin number or name */
> +	pin = strtonum(argv[1], 0, INT_MAX, &errstr);
> +	if (errstr)
> +		nm = argv[1];	/* try named pin */
> +
> +	if (argc <= 2) {
> +		gpioread(pin, nm);
> +		return EXIT_SUCCESS;
> +	}
> +
> +	if (!strcmp(argv[2], "set")) {
> +		for (n = 3; n < argc; n++) {
> +			for (bs = pinflags; bs->string; bs++) {
> +				if (!strcmp(argv[n], bs->string)) {
> +					fl |= bs->mask;
> +					break;
>  				}
> -				gpiowrite(pin, nm, value);
>  			}
> -		} else
> -			gpioread(pin, nm);
> +			if (!bs->string)
> +				nam = argv[n];
> +		}
> +		gpioset(pin, nm, fl, nam);
> +	} else if (!strcmp(argv[2], "unset")) {
> +		gpiounset(pin, nm);
> +	} else {
> +		value = strtonum(argv[2], INT_MIN, INT_MAX, &errstr);
> +		if (errstr) {
> +			if (!strcmp(argv[2], "on"))
> +				value = GPIO_PIN_HIGH;
> +			else if (!strcmp(argv[2], "off"))
> +				value = GPIO_PIN_LOW;
> +			else if (!strcmp(argv[2], "toggle"))
> +				value = 2;
> +			else
> +				errx(EXIT_FAILURE, "%s: invalid value",argv[2]);
> +		}
> +		gpiowrite(pin, nm, value);
>  	}
> +
>  	return EXIT_SUCCESS;
>  }
>  
> @@ -214,12 +183,12 @@ getinfo(void)
>  }
>  
>  static void
> -gpioread(int pin, char *gp_name)
> +gpioread(int pin, const char *gp_name)
>  {
>  	struct gpio_req req;
>  
>  	memset(&req, 0, sizeof(req));
> -	if (gp_name != NULL)
> +	if (gp_name)
>  		strlcpy(req.gp_name, gp_name, sizeof(req.gp_name));
>  	else
>  		req.gp_pin = pin;
> @@ -240,7 +209,7 @@ gpioread(int pin, char *gp_name)
>  }
>  
>  static void
> -gpiowrite(int pin, char *gp_name, int value)
> +gpiowrite(int pin, const char *gp_name, int value)
>  {
>  	struct gpio_req req;
>  
> @@ -248,7 +217,7 @@ gpiowrite(int pin, char *gp_name, int value)
>  		errx(EXIT_FAILURE, "%d: invalid value", value);
>  
>  	memset(&req, 0, sizeof(req));
> -	if (gp_name != NULL)
> +	if (gp_name)
>  		strlcpy(req.gp_name, gp_name, sizeof(req.gp_name));
>  	else
>  		req.gp_pin = pin;
> @@ -257,10 +226,8 @@ gpiowrite(int pin, char *gp_name, int value)
>  		req.gp_value = value;
>  		if (ioctl(devfd, GPIOWRITE, &req) == -1)
>  			err(EXIT_FAILURE, "GPIOWRITE");
> -	} else {
> -		if (ioctl(devfd, GPIOTOGGLE, &req) == -1)
> -			err(EXIT_FAILURE, "GPIOTOGGLE");
> -	}
> +	} else if (ioctl(devfd, GPIOTOGGLE, &req) == -1)
> +		err(EXIT_FAILURE, "GPIOTOGGLE");
>  
>  	if (state)
>  		printf("%d\n", value < 2 ? value : 1 - req.gp_value);
> @@ -277,19 +244,19 @@ gpiowrite(int pin, char *gp_name, int value)
>  }
>  

I don't see an advantage of just rearranging the code.

>  static void
> -gpioset(int pin, char *name, int fl, char *alias)
> +gpioset(int pin, const char *name, int fl, const char *alias)
>  {
>  	struct gpio_set set;
>  	const struct bitstr *bs;
>  
>  	memset(&set, 0, sizeof(set));
> -	if (name != NULL)
> +	if (name)

technically correct, but I prefer the expressivenes of the original
statement.

>  		strlcpy(set.gp_name, name, sizeof(set.gp_name));
>  	else
>  		set.gp_pin = pin;
>  	set.gp_flags = fl;
>  
> -	if (alias != NULL)
> +	if (alias)
>  		strlcpy(set.gp_name2, alias, sizeof(set.gp_name2));
>  
>  	if (ioctl(devfd, GPIOSET, &set) == -1)
> @@ -298,20 +265,20 @@ gpioset(int pin, char *name, int fl, char *alias)
>  	if (quiet)
>  		return;
>  
> -	if (name != NULL)
> +	if (name)
>  		printf("pin %s: caps:", name);
>  	else
>  		printf("pin %d: caps:", pin);
> -	for (bs = pinflags; bs->string != NULL; bs++)
> +	for (bs = pinflags; bs->string; bs++)
>  		if (set.gp_caps & bs->mask)
>  			printf(" %s", bs->string);
>  	printf(", flags:");
> -	for (bs = pinflags; bs->string != NULL; bs++)
> +	for (bs = pinflags; bs->string; bs++)
>  		if (set.gp_flags & bs->mask)
>  			printf(" %s", bs->string);
>  	if (fl > 0) {
>  		printf(" ->");
> -		for (bs = pinflags; bs->string != NULL; bs++)
> +		for (bs = pinflags; bs->string; bs++)
>  			if (fl & bs->mask)
>  				printf(" %s", bs->string);
>  	}
> @@ -319,12 +286,12 @@ gpioset(int pin, char *name, int fl, char *alias)
>  }
>  
>  static void
> -gpiounset(int pin, char *name)
> +gpiounset(int pin, const char *name)
>  {
>  	struct gpio_set set;
>  
>  	memset(&set, 0, sizeof(set));
> -	if (name != NULL)
> +	if (name)
>  		strlcpy(set.gp_name, name, sizeof(set.gp_name));
>  	else
>  		set.gp_pin = pin;
> @@ -333,16 +300,45 @@ gpiounset(int pin, char *name)
>  		err(EXIT_FAILURE, "GPIOUNSET");
>  }
>  
> +static unsigned
> +get_uint_or_die(const char *str, const char *name)
> +{
> +	int oerrno;
> +	char *ep;
> +
> +	long lval = strtol(str, &ep, 0);
> +	if (!*str || *ep)
> +		errx(EXIT_FAILURE, "invalid %s (not a number)", name);
> +
> +	oerrno = errno, errno = 0;
> +
> +	if ((errno == ERANGE && (lval == LONG_MAX || lval == LONG_MIN))
> +	    || lval < 0 || (unsigned long)lval > UINT_MAX)
> +		errx(EXIT_FAILURE, "%s out of range", name);
> +
> +	errno = oerrno;
> +
> +	return (unsigned)lval;
> +}
> +
>  static void
> -devattach(char *dvname, int offset, uint32_t mask, uint32_t flags)
> +devattach(const char *driver, const char *offset,
> +    const char *mask, const char *flags)
>  {
> -	struct gpio_attach attach;
> +	const char *errstr;
> +	struct gpio_attach attach = { .ga_flags = 0 };
> +
> +	attach.ga_offset = strtonum(offset, 0, INT_MAX, &errstr);
> +	if (errstr)
> +		errx(EXIT_FAILURE, "offset is %s: %s", errstr, offset);
> +
> +	attach.ga_mask = get_uint_or_die(mask, "mask");
> +
> +	if (flags)
> +		attach.ga_flags = get_uint_or_die(flags, "flags");
> +
> +	strlcpy(attach.ga_dvname, driver, sizeof(attach.ga_dvname));
>  
> -	memset(&attach, 0, sizeof(attach));
> -	strlcpy(attach.ga_dvname, dvname, sizeof(attach.ga_dvname));
> -	attach.ga_offset = offset;
> -	attach.ga_mask = mask;
> -	attach.ga_flags = flags;
>  	if (ioctl(devfd, GPIOATTACH, &attach) == -1)
>  		err(EXIT_FAILURE, "GPIOATTACH");
>  }
> @@ -350,17 +346,14 @@ devattach(char *dvname, int offset, uint32_t mask, uint32_t flags)
>  static void
>  usage(void)
>  {
> -	const char *progname;
> -
> -	progname = getprogname();
> -	fprintf(stderr, "usage: %s [-qs] device [pin] [0 | 1 | 2 | "
> -	    "on | off | toggle]\n", progname);
> -	fprintf(stderr, "       %s [-q] device pin set [flags] [name]\n",
> -	    progname);
> -	fprintf(stderr, "       %s [-q] device pin unset\n", progname);
> -	fprintf(stderr, "       %s [-q] device attach device offset mask "
> -	    "[flag]\n",
> -	    progname);
> +	const char *pn = getprogname();
> +
> +	#define U(...) fprintf(stderr, __VA_ARGS__)
> +	U("usage: %s [-qs] device [pin] [0 | 1 | 2 | on | off | toggle]\n", pn);
> +	U("       %s [-q] device pin set [flags] [name]\n", pn);
> +	U("       %s [-q] device pin unset\n", pn);
> +	U("       %s [-q] device attach device offset mask [flag]\n", pn);
> +	#undef U

This is ugly to say the lease, please, no, no, no...

In conclusion, I don't like your patch, and since same/similar code is
used in other BSDs, I think we should not apply it.

>  
>  	exit(EXIT_FAILURE);
>  }
>