From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marex@denx.de>
Date: Wed, 5 Sep 2012 14:52:56 +0200
Subject: [U-Boot] [PATCH v3 1/2] Loop block device for sandbox
In-Reply-To: <1644511.X2JLGWi2Ll@merom>
References: <1346505549-6795-1-git-send-email-morpheus.ibis@gmail.com>
	<201209051333.14051.marex@denx.de> <1644511.X2JLGWi2Ll@merom>
Message-ID: <201209051452.57146.marex@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Dear Pavel Herrmann,

[...]

> > 
> > move this out.
> > 
> > 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, so i can safely call free() later, otherwise
> this segfaults when called before sata scan

See my previous email

> > > +	/* 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.
> 
> hint - scan_sata does the range check in this codepath

saw it, see below. Duplication of code is not good and is error prone.

> > > +		return scan_sata(dev);
> > > +
> > > +	case 3:
> > > +		if (!strncmp(argv[1], "inf", 3)) {
> > > +			dev = simple_strtoul(argv[2], NULL, 10);
> > 
> > Same here
> 
> see above
> 
> > > +			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 .
> 
> wont help, still need argc 3 or 4

Makes is simpler, you can bail out if it's not 3 or 4

> > > +	"sata_loop load devnum file - load file from host FS into loop
> > > devnum"
> > 
> > sata_loop is redundant above.
> 
> really? how do you figure?

Run "help sata_loop" and see the result ...

> 
> Pavel Herrmann