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

Marek Vasut
2012-09-05T11:33:13Z

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 ;-)

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

> + block_dev_desc_t *pdev = &sata_dev_desc[dev];
> + int fd, old_fd;
> +
> +

Redundant newline

> + if (!zeroed) {
> + int i;
> + for (i = 0; i < CONFIG_SYS_SATA_MAX_DEVICE; i++)
> + filenames[i]=strdup("");
> + zeroed = 1;
> + }
> +
> + if ((dev < 0) || (dev >= CONFIG_SYS_SATA_MAX_DEVICE)) {
> + printf("File index %d is out of range.\n", dev);
> + return -EINVAL;
> + }
> +
> + fd = os_open(filenames[dev], OS_O_RDWR);
> + /* This is ugly, but saves allocation for 1 int. */
> + old_fd = (long) pdev->priv;
> + pdev->priv = (void *) (long) fd;
> + /*
> + * sadly we cannot set -1 to all as above, because "sata init" will zero
> + * this value before calling sata_init.

Sorry, I can't parse this.

> + */
> + if ((old_fd > 2) || (old_fd < 0))
> + os_close(old_fd);
> +
> + return 0;
> +}
> +
> +lbaint_t sata_read(int dev, lbaint_t start, lbaint_t blkcnt, void *buffer)
> +{
> + block_dev_desc_t *pdev = &sata_dev_desc[dev];
> + int fd = (long) pdev->priv;
> + lbaint_t start_byte = ATA_SECT_SIZE * start;
> + lbaint_t length_byte = ATA_SECT_SIZE * blkcnt;
> + lbaint_t retval;
> +
> + if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
> + return -1;
> +
> + retval = os_read(fd, buffer, length_byte);
> +
> + return retval/ATA_SECT_SIZE;
> +}
> +
> +lbaint_t sata_write(int dev, lbaint_t start, lbaint_t blkcnt, void
> *buffer) +{
> + block_dev_desc_t *pdev = &sata_dev_desc[dev];
> + int fd = (long) pdev->priv;
> + lbaint_t start_byte = ATA_SECT_SIZE * start;
> + lbaint_t length_byte = ATA_SECT_SIZE * blkcnt;
> + lbaint_t retval;
> +
> + if ( os_lseek(fd, start_byte, OS_SEEK_SET) != start_byte)
> + return -1;
> +
> + retval = os_write(fd, buffer, length_byte);
> +
> + return retval/ATA_SECT_SIZE;

a[space]operator[space]b ... did you ignore all my previous comments?

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

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

> +);