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

[Stefan Tauner]

On Sun, 13 Nov 2011 23:23:39 +0100
Carl-Daniel Hailfinger  wrote:
> 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.
> > 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.

see other mail, unchanged in attached patch.
> > /*
> >     * 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?

reverted here and below...
> > 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.

i am not sure this is what we want. mainly... because (the buffer
handling in) ft2232_spi.c is slightly awful :)
various "bufs" (well, 2), some of them static (ok, only the one in
send_command, but still this is messier than expected for such a
simple driver), the two ~equivalent failed/ret variables in send_command
etc. make it a bit unreadable.

we need to check for failure of the buffer methods anyway, so i dont
see a reason to do the size checks in the caller(s).

i have reverted the whole file now, because i am not sure how to
proceed and wanted to publish what i have. not much changed, but i had
to rebase it due to the addition of the opaque framework and hwseq.
> […]
> With the above comments this is
> Acked-by: Carl-Daniel Hailfinger

should i commit the attached version?