Re: [flashrom] [PATCH] Unsignify lengths and addresses in chip functions and structs

[Carl-Daniel Hailfinger]

Am 19.09.2011 02:01 schrieb Stefan Tauner:
> Push those changes forward where needed to prevent new sign
> conversion warnings where possible.

Thanks for your patch.
> this is a crude rework of the reverted patch. i have tried to push
> unsignedness wherever possible, but there are some problems:
> ffs, min, max and some library/system calls require signed ints or
> return them or both.

We might want to provide type-safe min/max macros for those cases. An
example is at
http://gcc.gnu.org/onlinedocs/gcc/Typeof.html
Not essential for this patch, but might be nice to have anyway. Later.
> Signed-off-by: Stefan Tauner
>
> diff --git a/flash.h b/flash.h
> index 535c1b8..e7bfd4e 100644
> --- a/flash.h
> +++ b/flash.h
> @@ -121,8 +121,10 @@ struct flashchip {
>
> int (*probe) (struct flashchip *flash);
> /* Delay after "enter/exit ID mode" commands in microseconds. */
> int probe_timing;
> /* Delay after "enter/exit ID mode" commands in microseconds.
> * NB: negative values have special meanings, see TIMING_* below.
> */
> signed int probe_timing;

AFAICS the C standard defines that int is always signed. The only
exception is char which is implementation-defined. Please kill signed
here, but keep your comment.
> /*
>     * Erase blocks and associated erase function. Any chip erase function
> diff --git a/ft2232_spi.c b/ft2232_spi.c
> index 8ab89fa..8f00685 100644
> --- a/ft2232_spi.c
> +++ b/ft2232_spi.c
> @@ -106,10 +106,10 @@ static const char *get_ft2232_vendorname(int ft2232_vid, int ft2232_type)
>  }
>
> static int send_buf(struct ftdi_context *ftdic, const unsigned char *buf,
>     int size)
>     unsigned int size)
>
> {
>    int r;
> r = ftdi_write_data(ftdic, (unsigned char *) buf, size);
> r = ftdi_write_data(ftdic, (unsigned char *) buf, (int)size);

This cast is a sign that something is odd. Can you keep size as a signed
int?
> if (r < 0) {
>    	msg_perr("ftdi_write_data: %d, %s\n", r,
>    			ftdi_get_error_string(ftdic));
> @@ -119,19 +119,19 @@ static int send_buf(struct ftdi_context *ftdic, const unsigned char *buf,
>  }
>
> static int get_buf(struct ftdi_context *ftdic, const unsigned char *buf,
>    int size)
>    unsigned int size)
>
> {
>    int r;
>
> while (size > 0) {
> r = ftdi_read_data(ftdic, (unsigned char *) buf, size);
> r = ftdi_read_data(ftdic, (unsigned char *) buf, (int)size);

Same here.
> if (r < 0) {
> 	msg_perr("ftdi_read_data: %d, %s\n", r,
> 			ftdi_get_error_string(ftdic));
> 	return 1;
> }
> buf += r;
> size -= r;
> buf += (unsigned int)r;
> size -= (unsigned int)r;

...
> }
>    return 0;
>  }
> @@ -332,9 +332,10 @@ static int ft2232_spi_send_command(unsigned int writecnt, unsigned int readcnt,
>    struct ftdi_context *ftdic = &ftdic_context;
>    static unsigned char *buf = NULL;
>    /* failed is special. We use bitwise ops, but it is essentially bool. */
> int i = 0, ret = 0, failed = 0;
> int bufsize;
> static int oldbufsize = 0;
>
> int ret = 0, failed = 0;
>
> unsigned int i = 0;
>
> unsigned int bufsize;
>
> static unsigned int oldbufsize = 0;
>
> if (writecnt > 65536 || readcnt > 65536)
> return SPI_INVALID_LENGTH;

get_buf and send_buf calls should be checked for size>=INT_MAX and the
cast should happen in the call, not in get_buf/send_buf.
> diff --git a/serprog.c b/serprog.c
> index 9d554c9..94ae1c6 100644
> --- a/serprog.c
> +++ b/serprog.c
> @@ -707,7 +707,7 @@ uint8_t serprog_chip_readb(const chipaddr addr)
>  /* Local version that really does the job, doesn't care of max_read_n. */
>  static void sp_do_read_n(uint8_t * buf, const chipaddr addr, size_t len)
>  {
> int rd_bytes = 0;
> unsigned int rd_bytes = 0;
> unsigned char sbuf[6];
> msg_pspew("%s: addr=0x%lx len=%lu\n", __func__, addr, (unsigned long)len);
> /* Stream the read-n -- as above. */
>
> @@ -725,7 +725,7 @@ static void sp_do_read_n(uint8_t * buf, const chipaddr addr, size_t len)
>    	int r = read(sp_fd, buf + rd_bytes, len - rd_bytes);
>    	if (r <= 0)
>    		sp_die("Error: cannot read read-n data");
> rd_bytes += r;
> rd_bytes += (unsigned int)r;

Odd cast, but safe. You correctly analyzed that situation.
> } while (rd_bytes != len);
>    return;
>  }

The flashrom.c changes were really hard to verify, but that's not your
fault. The code in there is not easy to understand, but you got it
exactly right.
With the above comments this is
Acked-by: Carl-Daniel Hailfinger

Regards,
Carl-Daniel