Hi Henrik, On Wed, May 15, 2013 at 6:09 PM, Henrik Nordström <hen...@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 >> <hen...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot