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

[Pavel Herrmann <morpheus.ibis@gmail.com>]

second try...

On Wednesday 05 September 2012 13:33:13 Marek Vasut wrote:
> Dear Pavel Herrmann,
> 
> > This driver uses files as block devices, can be used for testing disk
> > operations on sandbox.
> > A new command "sata_loop" is introduced to load files in runtime.
> > 
> > Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
> > CC: Marek Vasut <marex@denx.de>
> > CC: Mike Frysinger <vapier@gentoo.org>
> 
> ERROR: "foo * bar" should be "foo *bar"
> #136: FILE: drivers/block/sata_loopback.c:36:
> +static char * filenames[CONFIG_SYS_SATA_MAX_DEVICE];
> 
> WARNING: externs should be avoided in .c files
> #138: FILE: drivers/block/sata_loopback.c:38:
> +extern block_dev_desc_t sata_dev_desc[];
> 
> ERROR: do not initialise statics to 0 or NULL
> #142: FILE: drivers/block/sata_loopback.c:42:
> +       static int zeroed = 0;
> 
> ERROR: spaces required around that '=' (ctx:VxV)
> #150: FILE: drivers/block/sata_loopback.c:50:
> +                       filenames[i]=strdup("");
>                                     ^
> 
> ERROR: space prohibited after that open parenthesis '('
> #181: FILE: drivers/block/sata_loopback.c:81:
> +       if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
> 
> ERROR: space prohibited after that open parenthesis '('
> #197: FILE: drivers/block/sata_loopback.c:97:
> +       if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
> 
> ERROR: do not initialise statics to 0 or NULL
> #256: FILE: drivers/block/sata_loopback.c:156:
> +       static int zeroed = 0;
> 
> total: 6 errors, 1 warnings, 207 lines checked
> 
> Checkpatch and I never sleep, don't even try ;-)
> 

sorry, my bad, i thought i had it on pre-commit

> > ---
> > 
> > Changes for v3:
> >   introduce sata_loop command
> > 
> > Changes for v2:
> >   split sandbox config off into separate patch (2/2)
> >   rename file to signify exported API
> >   style fixes
> >   show end of long filenames rather than beginning
> >   check for lseek errors to indicate non-regular file
> >  
> >  drivers/block/Makefile        |   1 +
> >  drivers/block/sata_loopback.c | 200
> > 
> > ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 201
> > insertions(+)
> > 
> >  create mode 100644 drivers/block/sata_loopback.c
> > 
> > diff --git a/drivers/block/Makefile b/drivers/block/Makefile
> > index f1ebdcc..c95651a 100644
> > --- a/drivers/block/Makefile
> > +++ b/drivers/block/Makefile
> > @@ -40,6 +40,7 @@ COBJS-$(CONFIG_SATA_SIL) += sata_sil.o
> > 
> >  COBJS-$(CONFIG_IDE_SIL680) += sil680.o
> >  COBJS-$(CONFIG_SCSI_SYM53C8XX) += sym53c8xx.o
> >  COBJS-$(CONFIG_SYSTEMACE) += systemace.o
> > 
> > +COBJS-${CONFIG_SATA_LOOP} += sata_loopback.o
> > 
> >  COBJS	:= $(COBJS-y)
> >  SRCS	:= $(COBJS:.o=.c)
> > 
> > diff --git a/drivers/block/sata_loopback.c b/drivers/block/sata_loopback.c
> > new file mode 100644
> > index 0000000..0e6923b
> > --- /dev/null
> > +++ b/drivers/block/sata_loopback.c
> > @@ -0,0 +1,200 @@
> > +/*
> > + * (C) Copyright 2012
> > + * Pavel Herrmann <morpheus.ibis@gmail.com>
> > + *
> > + * See file CREDITS for list of people who contributed to this
> > + * project.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation; either version 2 of
> > + * the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> > + * MA 02111-1307 USA
> > + */
> > +
> > +#include <common.h>
> > +#include <part.h>
> > +#include <ata.h>
> > +#include <libata.h>
> > +#include <errno.h>
> > +#include <os.h>
> > +#include <command.h>
> > +#include <malloc.h>
> > +
> > +static const char revision[] = "0.0";
> > +static const char vendor[] = "SATA loopback";
> > +
> > +static char * filenames[CONFIG_SYS_SATA_MAX_DEVICE];
> > +
> > +extern block_dev_desc_t sata_dev_desc[];
> > +
> > +int init_sata(int dev)
> > +{
> > +	static int zeroed = 0;
> 
> Is this really needed here and below? Pull this crap out.

yes, because you dont know whether this gets called first from sata_loop or 
from "sata init"

> > +int scan_sata(int dev)
> > +{
> > +	block_dev_desc_t *pdev = &sata_dev_desc[dev];
> > +	int fd = (long) pdev->priv;
> > +	int namelen;
> > +	const char *filename = filenames[dev];
> > +	lbaint_t bytes = 0;
> > +
> > +	if ((dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE)) {
> > +		printf("File index %d is out of range.\n", dev);
> > +		return -EINVAL;
> > +	}
> > +
> > +	memcpy(pdev->vendor, vendor, sizeof(vendor));
> > +	memcpy(pdev->revision, revision, sizeof(revision));
> > +	namelen = strlen(filenames[dev]);
> > +
> > +	if (namelen > 20) {
> > +		/* take the last 17 chars, prepend them with "..." */
> > +		filename += (namelen - 17);
> > +		memcpy(pdev->product, "...", 3);
> > +		memcpy(pdev->product+3, filename, 17);
> > +	} else
> > +		memcpy(pdev->product, filename, namelen);
> > +
> > +	pdev->product[20] = 0;
> > +
> > +	bytes = os_lseek(fd, 0, OS_SEEK_END);
> > +	if (bytes != -1) {
> > +		pdev->type = DEV_TYPE_HARDDISK;
> > +		pdev->blksz = ATA_SECT_SIZE;
> > +		pdev->lun = 0;
> > +		pdev->lba = bytes/ATA_SECT_SIZE;
> > +		printf("SATA loop %d:\nfilename: %s\nsize: %lu\nblock count:"
> > +			" %lu\n", dev, filenames[dev], bytes, pdev->lba);
> > +	} else {
> > +		pdev->type = DEV_TYPE_HARDDISK;
> > +		pdev->blksz = ATA_SECT_SIZE;
> > +		pdev->lun = 0;
> > +		pdev->lba = 0;
> > +		printf("SATA loop %d:\nfilename: %s\nFAILED TO OPEN\n",
> > +			 dev, filenames[dev]);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +
> > +int do_loop(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> > +{
> > +	int dev = 0;
> > +	static int zeroed = 0;
> 
> 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

> > +	/* 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

> > +		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

> > +	"sata_loop load devnum file - load file from host FS into loop devnum"
> 
> sata_loop is redundant above.

really? how do you figure?

Pavel Herrmann