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

[Henrik Nordström]

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?

Regads
Henrik