[U-Boot] [PATCH v3 1/2] Loop block device for sandbox

[Marek Vasut <marex@denx.de>]

Dear Pavel Herrmann,

[...]

> > besides, I think it'd be much systematic to just scream at user to call
> > "sata rescan" and bail out instead of doing it for him.
> 
> i dont actually need a sata rescan, i just need to make sure i have
> dynamically allocated names

Sorry, I can't parse this ... but ...

> , so i can safely call free() later, otherwise
> this segfaults when called before sata scan

The free() function frees the memory space pointed to by ptr, which must have 
been returned by a previous call to malloc(), calloc() or realloc().  Otherwise, 
or if free(ptr) has already been called before, undefined  behavior  occurs. If 
ptr is NULL, no operation is performed.

So if you call free() on null pointer, nothing happens. Where's the real 
problem?

> > > +	/* make sure we have valid filenames */
> > > +	if (!zeroed) {
> > > +		init_sata(0);
> > > +		zeroed = 1;
> > > +	}
> > > +
> > > +	switch (argc) {
> > > +	case 0:
> > > +	case 1:
> > > +		return CMD_RET_USAGE;
> > > +	case 2:
> > > +		dev = simple_strtoul(argv[1], NULL, 10);
> > 
> > Ok, so if I run this command and ask for device 0xb00bf33d ... will this
> > survive? Hint: it won't, rangecheck missing.
> > 
> > > +		return scan_sata(dev);
> > > +
> > > +	case 3:
> > > +		if (!strncmp(argv[1], "inf", 3)) {
> > > +			dev = simple_strtoul(argv[2], NULL, 10);
> > 
> > Same here
> > 
> > > +			return scan_sata(dev);
> > > +		}
> > > +		return CMD_RET_USAGE;
> > > +	case 4:
> > > +		if (!strncmp(argv[1], "load", 4)) {
> > > +			dev = simple_strtoul(argv[2], NULL, 10);
> > > +			if ((dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE)) {
> > 
> > And here you have it ?
> > 
> > Uh oh, I see, sata_scan() does it for you ... I'd say, abstract it out
> > into lightweight static inline function.
> > 
> > > +				printf("File index %d is out of range.\n", dev);
> > > +				return -EINVAL;
> > > +			}
> > > +			free(filenames[dev]);
> > > +			filenames[dev] = strdup(argv[3]);
> > > +			init_sata(dev);
> > > +			return scan_sata(dev);
> > > +		}
> > > +		return CMD_RET_USAGE;
> > > +	}
> > > +	return CMD_RET_USAGE;
> > > +}
> > > +
> > > +U_BOOT_CMD(
> > > +	sata_loop, 4, 1, do_loop,
> > > +	"SATA loopback",
> > > +	"[info] devnum - show info about loop devnum\n"
> > 
> > Make this "info" part mandatory. Than you can cut the whole argc loop
> > into simple "if argc != 2 ; then fail" . And do simple checking for the
> > first letter of the argument being either i or d .
> > 
> > > +	"sata_loop load devnum file - load file from host FS into loop
> > > devnum"
> > 
> > sata_loop is redundant above.
> > 
> > > +);