[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 <c-d.hailfinger.devel.2006 at gmx.net> 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 <stefan.tauner at student.tuwien.ac.at>
> >
> >
> > 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 <c-d.hailfinger.devel.2006 at gmx.net>

should i commit the attached version?

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Unsignify-lengths-and-addresses-in-chip-functions-an.patch
Type: text/x-patch
Size: 40770 bytes
Desc: not available
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20111122/71cb8036/attachment.patch>