On Thu, Jul 12, 2012 at 02:39:27PM +0200, Lukasz Majewski wrote:
> On Wed, 11 Jul 2012 04:54:31 -0700
> Tom Rini <tr...@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.majew...@samsung.com>
> > > > > Signed-off-by: Kyungmin Park <kyungmin.p...@samsung.com>
> > > > > Cc: Marek Vasut <ma...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
