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);
> }
>
