[U-Boot] [PATCH v2 4/7] dfu: MMC specific routines for DFU operation

[Tom Rini]

On Thu, Jul 12, 2012 at 02:39:27PM +0200, Lukasz Majewski wrote:
> On Wed, 11 Jul 2012 04:54:31 -0700
> Tom Rini <trini at ti.com> wrote:
> 
> > On Tue, Jul 10, 2012 at 12:38:54PM +0200, Lukasz Majewski wrote:
> > > Hi Tom,
> > > 
> > > > On Wed, Jul 04, 2012 at 05:48:39PM +0200, Lukasz Majewski wrote:
> > > > > Support for MMC storage devices to work with DFU framework.
> > > > > 
> > > > > Signed-off-by: Lukasz Majewski <l.majewski at samsung.com>
> > > > > Signed-off-by: Kyungmin Park <kyungmin.park at samsung.com>
> > > > > Cc: Marek Vasut <marex at denx.de>
> > > > [snip]
> > > > > +	case RAW_ADDR:
> > > > > +		sprintf(cmd_buf, "mmc write 0x%x %x %x",
> > > > > (unsigned int) buf,
> > > > > +			dfu->data.mmc.lba_start,
> > > > > dfu->data.mmc.lba_size);
> > > > > +		break;
> > > > > +	case FAT:
> > > > > +		sprintf(cmd_buf, "fatwrite mmc %d:%d 0x%x %s
> > > > > %lx",
> > > > > +			dfu->data.mmc.dev, dfu->data.mmc.part,
> > > > > +			(unsigned int) buf, dfu->name, *len);
> > > > > +		break;
> > > > > +	default:
> > > > > +		printf("%s: Wrong layout!\n", __func__);
> > > > > +	}
> > > > > +
> > > > > +	debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf);
> > > > > +	run_command(cmd_buf, 0);
> > > > 
> > > > If we try and take the long-view here, that fatwrite/mmc write
> > > > don't perform a lot of sanity checking on input isn't good.  Lots
> > > > of commands I believe don't, but we can start somewhere. 
> > > Yes, indeed they don't. But I think, that it is a deeper problem.
> > > 
> > > When one looks into the cmd_mmc.c, the code is not checking the
> > > correctness of passed data. It performs strncmp, then simple_strtoul
> > > and with this parameter calls mmc->block_dev.block_read(). 
> > > 
> > > But I'm a bit concern if adding function:
> > > 
> > > do_mmcops_check(unsigned int lba_start, unsigned int lba_end, ...)
> > > to
> > > 
> > > do_mmcops(argc, argv) {
> > > 	int i = simple_strtol(argv[]);
> > > 	return do_mmcops_check(i);
> > > }
> > 
> > Well, what I was suggesting would be:
> > 
> > do_mmcops_real(uint lba_start, ...) { .. most of do_mmcops today .. }
> > do_mmcops_from_cmd(argc, argv) {
> >  ... convert user input today, maybe try and sanity check input
> > tomorrow ..
> > }
> > 
> > And then dfu calls do_mmcops_real(lba_start, ...).  A further clean-up
> > would be to make the interface the command uses to perform checking of
> > the arguments passed.  Does this make sense?
> > 
> 
> Generally it is in my opinion a good way to go.
> 
> However, why we aren't first writing sanity checks for passed arguments?

Simply because I didn't want to ask you to do a lot more unrelated work
:)  If you want to split and check the mmc (and fatwrite) argueuments
and then make the DFU series depend on that, by all means please do so!

> We are adding one more level of abstraction, but don't think of the main
> problem (checking values of passed arguments)?
> 
> Anyway we shall wait for Marek's opinion.

Yes, a good idea as well.

-- 
Tom