From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marex@denx.de>
Date: Fri, 13 Sep 2013 18:13:32 +0200
Subject: [U-Boot] [PATCH v3 2/3] dfu: ram support
In-Reply-To: <20130913153736.GG6479@book.gsilab.sittig.org>
References: <cover.1379055111.git.afzal.mohd.ma@gmail.com>
	<201309131551.22535.marex@denx.de>
	<20130913153736.GG6479@book.gsilab.sittig.org>
Message-ID: <201309131813.32985.marex@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Dear 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.

Full ACK.

Best regards,
Marek Vasut