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

[Tom Rini]

On Tue, Jul 03, 2012 at 05:07:04PM -0600, Stephen Warren wrote:
> On 07/03/2012 04:33 PM, Tom Rini wrote:
> > On Wed, Jul 04, 2012 at 12:31:14AM +0200, Marek Vasut wrote:
> >> Dear Tom Rini,
> >>
> >> [...]
> >>
> >>>>>>> +	debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf);
> >>>>>>> +	run_command(cmd_buf, 0);
> >>>>>>
> >>>>>> Holy Moly ... can we not make this into simple calls to those
> >>>>>> subsystems ? Instead invoking command is crazy ;-)
> >>>>>
> >>>>> Are they really simple?  There's a few other places we do this, and so
> >>>>> long as it's documented that DFU depends on CONFIG_FAT_WRITE for
> >>>>> writing to fat and so forth.
> >>>>
> >>>> Well ain't it easier to call fat_write() or similar?
> >>>
> >>> Assuming that most of the logic in do_fat_fswrite is needed, no.  And I
> >>> think a good portion of it is, at first glance at least.
> >>
> >> Abstracting it out into a function won't cut it?
> > 
> > My inclination would be to say that seems a bit sillier than just using
> > run_command(...) to call the existing command.
> 
> Abstracting into a function definitely means the compiler will be able
> to type-check all the arguments to the function. Is the same true using
> run_command; I assume everything gets serialized to an array of strings
> and hence any validation is deferred until run-time then?

string is constructed with sprintf and that gives us some type checking.
Only cast today is buf to an unsigned int.  And the information either
comes from tools that should/must have done sanity checking or from the
'dfu' command which again should (didn't verify that patch yet) have
done sanity checking to get us this far.

-- 
Tom