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

[Marek Vasut]

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

Would be cool indeed.

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

My opinion is that if you'll do the sanity checks, that'd be good. We're right 
before .07 release anyway, so the patches will hit the next merge window. Are 
you up for doing a lot of unrelated work to make this proper?

Best regards,
Marek Vasut