[U-Boot] [PATCH v3 2/3] dfu: ram support

[Gerhard Sittig]

On Fri, Sep 13, 2013 at 15:51 +0200, Marek Vasut wrote:
> 
> Dear Afzal Mohammed,
> 
> [...]
> 
> > +int dfu_fill_entity_ram(struct dfu_entity *dfu, char *s)
> > +{
> > +	char *st;
> > +
> > +	dfu->dev_type = DFU_DEV_RAM;
> > +	st = strsep(&s, " ");
> > +	if (strcmp(st, "ram")) {
> > +		error("unsupported device: %s\n", st);
> > +		return -ENODEV;
> > +	}
> > +
> > +	dfu->layout = DFU_RAM_ADDR;
> > +	dfu->data.ram.start = (void *)simple_strtoul(s, &s, 16);
> > +	dfu->data.ram.size = simple_strtoul(++s, &s, 16);
> 
> This ++s, &s is quite crazy ;-)

Actually it's not just a neat trick, but instead it's not
guaranteed to work.  It's an inviation for problems that you
don't need.

The order in which arguments for the call get evaluated isn't
specified, so behaviour is uncertain here.  Read: the
construction happens to work by coincidence for individual
setups, but isn't correct and need not work elsewhere, and may
break in arbitrary ways at any occasion, and then is hard to
track down since things may "seem to work" often enough or the
bug won't show up when you are diagnosing the problem.

The code needs to get fixed before getting picked up.  Doing the
increment in a separate instruction is cheap and simple, the
terse form is wrong.


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de