From mboxrd@z Thu Jan  1 00:00:00 1970
From: Tom Rini <trini@ti.com>
Date: Wed, 11 Jul 2012 04:54:31 -0700
Subject: [U-Boot] [PATCH v2 4/7] dfu: MMC specific routines for DFU
 operation
In-Reply-To: <20120710123854.2c6de104@lmajewski.digital.local>
References: <1341308291-14663-1-git-send-email-l.majewski@samsung.com>
	<1341416922-13792-1-git-send-email-l.majewski@samsung.com>
	<1341416922-13792-5-git-send-email-l.majewski@samsung.com>
	<20120710084542.GB5053@oliver-linux>
	<20120710123854.2c6de104@lmajewski.digital.local>
Message-ID: <20120711115431.GA17783@oliver-linux>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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?

-- 
Tom