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

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