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@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@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com Cc: Marek Vasut marex@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.