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

[Lukasz Majewski]

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?

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.

-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group