[U-Boot] [PATCH v2] sandbox: block driver using host file/device as backing store

[Simon Glass]

Hi Henrik,

On Wed, May 15, 2013 at 6:09 PM, Henrik Nordström
<henrik at henriknordstrom.net> wrote:
> ons 2013-05-15 klockan 15:20 -0700 skrev Simon Glass:
>> Hi Henrik,
>>
>> On Wed, May 15, 2013 at 2:54 PM, Henrik Nordström
>> <henrik at henriknordstrom.net> wrote:
>> > Version 2 of this patch addressing the code comments by Simon and adding
>> > some small missing pieces.
>>
>> Looks good.
>>
>> For change log, you should follow the standard approach - also instead
>> of 'comments by Simon' it is good to list the things you changed. You
>> might find patman useful for preparing and sending patches.
>
> Just looked into it and looks nice. Giving it a try for next version.
>
> Took a little while to get started, mostly because it crashes & burns
> when not finding an matching alias for sandbox.
>
>> blank line here, please fix globally (a blank line between
>> declarations and code).
>
> Ok.
>
>> > +static int do_sandbox_info(cmd_tbl_t *cmdtp, int flag, int argc,
>> > +                          char * const argv[])
>> > +{
>> > +       if (argc < 1 || argc > 2)
>> > +               return CMD_RET_USAGE;
>>
>> Probably best to put this after the declarations
>
> Ok. Also restrucured do_sandbox_bind a little to match this. There one
> of the declarations depends on this being checked first.
>
>
>> Move to top of function. Try to collect your declarations within a
>> block when it's easy to do so.
>
> Ok.
>
>
>> > +       printf("%3s %12s %s\n", "dev", "blocks", "path");
>> > +       for (dev = min_dev; dev <= max_dev; dev++) {
>> > +               printf("%3d ", dev);
>> > +               block_dev_desc_t *blk_dev = host_get_dev(dev);
>> > +               if (!blk_dev)
>> > +                       continue;
>>
>> Does this case ever happen? If so you don't print \n.
>
> Yes. And it then relies on the driver to print an error.
>
>> > +               struct host_block_dev *host_dev = blk_dev->priv;
>>
>> Maybe leave the assignment here but put the declaration at the start
>> of the block.
>
> Yes.
>
>> >  COBJS-$(CONFIG_IDE_SIL680) += sil680.o
>> >  COBJS-$(CONFIG_SCSI_SYM53C8XX) += sym53c8xx.o
>> >  COBJS-$(CONFIG_SYSTEMACE) += systemace.o
>> > +COBJS-$(CONFIG_SANDBOX) += sandbox.o
>>
>> Alphabetic order so this should go up two.
>
> up seven. sorted on filename here not CONFIG_..
>
>> > +#include <sandboxblockdev.h>
>>
>> How about sandbox-blockdev.h
>
> Yee.
>
>> puts() when you don't have format args. Please fix globally.
>
> Ok.
>
>> > +       if (host_dev->fd == -1) {
>> > +               printf("Failed to access host backing file '%s'\n",
>> > +                      host_dev->filename);
>> > +               return 1;
>>
>> -ENOENT might be better (include errno.h)
>
> or maybe just -errno?
>
> and handle the error in do_sandbox_bind().
>
>> > +int host_dev_bind(int dev, char *filename);
>>
>> Please add a comment as to what this function does, describing the
>> meaning and valid values for each argument.
>
> Ok.
>
>> =>ext4load host 0:3
>> Segmentation fault (core dumped)
>
> Doesn't ext2load expect a address & filename to load?

Sorry I meant:

=>ext4ls host 0:3
Segmentation fault (core dumped)

It may not be your code, but I think the segfault is there.

>
> Regads
> Henrik
>

Regards,
Simon