Hi Henrik,
On Wed, May 15, 2013 at 6:09 PM, Henrik Nordström henrik@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@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