Attention is currently required from: Xiang Wang, Stefan Reinauer.

View Change

11 comments:

• File Makefile:

  • Patch Set #4, Line 800:

    
    CONFIG_SYSFSGPIO ?= no
    

    nit: move this above CONFIG_PRINT_WIKI

• File meson.build:

  • Patch Set #4, Line 71: config_sysfsgpio

    We don't use this anywhere.

  • Patch Set #4, Line 313: if host_machine.system() == 'linux'

    Maybe it should be used here. Also, this depends on `config_bitbang_spi`. I have no idea how to tell meson about this dependency.

• File sysfsgpio.c:

  • Patch Set #1, Line 80: if (ret == (int)strlen(str))

    I tried it, and the compilation still reports an error

    Meh, silly compilers

  • Patch Set #1, Line 209:

    	/* parameter format: pins=cs_pin:sck_pin:mosi_pin:miso_pin */
    	char *pins = extract_programmer_param("pins");
    	int pins_inited = 0;
    	do {
    		struct pin_desc *pins_tab[] = {
    			&pin_cs, &pin_sck, &pin_mosi, &pin_miso
    		};
    		if (!(pins && strlen(pins)))
    			break;
    		char *token = strtok(pins, ":");
    		for (unsigned i = 0; i < ARRAY_SIZE(pins_tab); i++) {
    			long v;
    			if (!token)
    				break;
    			if (atoi_s(token, 1, &v))
    				break;
    			pins_tab[i]->pin = v;
    			if (export_sysfsgpio(pins_tab[i]))
    				break;
    			token = strtok(NULL, ":");
    			pins_inited = (i + 1 == ARRAY_SIZE(pins_tab));
    		}
    	} while (0);
    	if (pins)
    		free(pins);
    	if (!pins_inited)
    		return 1;
    

    Done

    Yeah, it would be great to have all functions be pure, but the ones in `bitbang_spi_master` are an exception 😞

• File sysfsgpio.c:

  • Patch Set #4, Line 83: if (ret == (int)strlen(str))

    Ah, I just thought of another way to avoid this cast:

     	/* Use an int to avoid signed-unsigned comparisons */
     	const int len = strlen(str);
     	int ret = write(fd, str, len);
     	close(fd);
     	if (ret == len)
     		return 0;

    This saves having to call `strlen` twice. This could have problems with extremely long strings, but it doesn't really matter since it's a static function with controlled inputs.

  • Patch Set #4, Line 93: snprintf(s, sizeof(s), "/sys/class/gpio/gpio%s/", desc->pin);

    Hmmm... I think you should be able to define the common prefix in a macro and do the following:

     #define SYSFS_GPIO "/sys/class/gpio/"
     
     ...
     
     snprintf(s, sizeof(s), SYSFS_GPIO "gpio%s/", desc->pin);

  • Patch Set #4, Line 99:

    snprintf(s, sizeof(s), "/sys/class/gpio/gpio%s/direction", desc->pin);
    	desc->fd_direction = open(s, O_WRONLY);
    	if (desc->fd_direction < 0) {
    		msg_perr("Error: failed to open %s\n", s);
    		return -1;
    	}
    
    	snprintf(s, sizeof(s), "/sys/class/gpio/gpio%s/value", desc->pin);
    	desc->fd_value = open(s, O_RDONLY);
    	if (desc->fd_value < 0) {
    		msg_perr("Error: failed to open %s\n", s);
    		return -1;
    	}
    

    Here, you could do the following instead:

     	const size_t n = strlen(s);
     
     	if (n + 10 >= sizeof(s))
     		return -1;	/* Or use malloc() to allocate a large enough buffer */
     
     	strcpy(&s[n], "direction");
     	desc->fd_direction = open(s, O_WRONLY);
     	if (desc->fd_direction < 0) {
     		msg_perr("Error: failed to open %s\n", s);
     		return -1;
     	}
     
     	strcpy(&s[n], "value");
     	desc->fd_value = open(s, O_RDONLY);
     	if (desc->fd_value < 0) {
     		msg_perr("Error: failed to open %s\n", s);
     		return -1;
     	}

    Yes, with `strcpy()`. As long as you know (i.e. include code to test!) that there's enough space in the destination buffer, it is safe to use.

    People think `strncpy()` is safer because it takes a length parameter, but it's just another place for a mistake to hide. `strncpy()` does not produce a null-terminated string when there's no terminating null byte in the first `n` bytes of `src`, and this is the case when `n = strlen(src)`.

  • Patch Set #4, Line 135: desc->fd_direction = -1;

    Also invalidate the pin string:

     strncpy(desc->pin, "", sizeof(desc->pin));

    Here, `strncpy()` makes sense because we want to fill the entire buffer with null bytes.

  • Patch Set #4, Line 179: .get_miso = sysfsgpio_get_miso,

    nit: I would align the equals signs to the same column:

      	.set_cs   = sysfsgpio_set_cs,
     	.set_sck  = sysfsgpio_set_sck,
     	.set_mosi = sysfsgpio_set_mosi,
     	.get_miso = sysfsgpio_get_miso,

  • Patch Set #4, Line 204: static int parse_pins(struct sysfsgpio_pins *pins)

    I had eight comments on this function, but it was very hard to understand what I wanted you to do. Instead, here's how I would rewrite this function:

    static int parse_pins(struct sysfsgpio_pins *pins)
    {
     	char *const arg = extract_programmer_param("pins");
     	if (!arg)
     		return 1;
     
     	struct pin_desc *const pins_tab[] = {
     		&pins->cs, &pins->sck, &pins->mosi, &pins->miso,
     	};
     
     	char *token = strtok(arg, ":");
     	unsigned int i;
     
     	for (i = 0; token && i < ARRAY_SIZE(pins_tab); i++) {
     		char *suffix;
     		const long v = strtol(token, &suffix, 10);
     		if (errno != 0 || token == suffix)
     			break;
     		if (v < 0 || v > 9999999)
     			break;
     
     		snprintf(pins_tab[i]->pin, sizeof(pins_tab[i]->pin), "%ld", v);
     		if (export_sysfsgpio(pins_tab[i]))
     			break;
     		token = strtok(NULL, ":");
     	}
     
     	free(arg);
     	return i != ARRAY_SIZE(pins_tab);
     }

To view, visit change 49254. To unsubscribe, or for help writing mail filters, visit settings.