[U-Boot] [PATCH v2] sandbox: block driver using host file/device as backing store

Simon Glass sjg at chromium.org
Thu May 16 00:20:30 CEST 2013


Hi Henrik,

On Wed, May 15, 2013 at 2:54 PM, Henrik Nordström
<henrik at henriknordstrom.net> wrote:
> Version 2 of this patch addressing the code comments by Simon and adding
> some small missing pieces.

Looks good.

For change log, you should follow the standard approach - also instead
of 'comments by Simon' it is good to list the things you changed. You
might find patman useful for preparing and sending patches.

>
> left on the to-do is documentation and adding test suite tests.
>
> ---
> A simple "host" block driver using any host file/device
> as backing store.
>
> Adds two sb subcommands for managing host device bindings:
> sb bind <dev> [<filename>] - bind "host" device to file
> sb info [<dev>]            - show device binding & info
>
> Signed-off-by: Henrik Nordstrom <henrik at henriknordstrom.net>
> ---
>  common/cmd_sandbox.c       |  54 +++++++++++++++++++
>  disk/part.c                |  23 +++-----
>  drivers/block/Makefile     |   1 +
>  drivers/block/sandbox.c    | 127 +++++++++++++++++++++++++++++++++++++++++++++
>  include/config_fallbacks.h |   3 +-
>  include/configs/sandbox.h  |   6 +++
>  include/part.h             |   3 ++
>  include/sandboxblockdev.h  |  29 +++++++++++
>  8 files changed, 228 insertions(+), 18 deletions(-)
>  create mode 100644 drivers/block/sandbox.c
>  create mode 100644 include/sandboxblockdev.h
>
> diff --git a/common/cmd_sandbox.c b/common/cmd_sandbox.c
> index a28a844..59fbc97 100644
> --- a/common/cmd_sandbox.c
> +++ b/common/cmd_sandbox.c
> @@ -19,6 +19,8 @@
>
>  #include <common.h>
>  #include <fs.h>
> +#include <part.h>
> +#include <sandboxblockdev.h>
>
>  static int do_sandbox_load(cmd_tbl_t *cmdtp, int flag, int argc,
>                            char * const argv[])
> @@ -38,10 +40,60 @@ static int do_sandbox_save(cmd_tbl_t *cmdtp, int flag, int argc,
>         return do_save(cmdtp, flag, argc, argv, FS_TYPE_SANDBOX, 16);
>  }
>
> +static int do_sandbox_bind(cmd_tbl_t *cmdtp, int flag, int argc,
> +                          char * const argv[])
> +{
> +       if (argc < 2 || argc > 3)
> +               return CMD_RET_USAGE;
> +       char *ep;
> +       char *dev_str = argv[1];
> +       char *file = argc >= 3 ? argv[2] : NULL;
> +       int dev = simple_strtoul(dev_str, &ep, 16);

blank line here, please fix globally (a blank line between
declarations and code).

> +       if (*ep) {
> +               printf("** Bad device specification %s **\n", dev_str);
> +               return CMD_RET_USAGE;
> +       }
> +       return host_dev_bind(dev, file);
> +}
> +
> +static int do_sandbox_info(cmd_tbl_t *cmdtp, int flag, int argc,
> +                          char * const argv[])
> +{
> +       if (argc < 1 || argc > 2)
> +               return CMD_RET_USAGE;

Probably best to put this after the declarations

> +       int min_dev = 0;
> +       int max_dev = CONFIG_HOST_MAX_DEVICES - 1;

blank line here

> +       if (argc >= 2) {
> +               char *ep;
> +               char *dev_str = argv[1];
> +               int dev = simple_strtoul(dev_str, &ep, 16);

and here

> +               if (*ep) {
> +                       printf("** Bad device specification %s **\n", dev_str);
> +                       return CMD_RET_USAGE;
> +               }
> +               min_dev = dev;
> +               max_dev = dev;
> +       }
> +       int dev;

Move to top of function. Try to collect your declarations within a
block when it's easy to do so.

> +       printf("%3s %12s %s\n", "dev", "blocks", "path");
> +       for (dev = min_dev; dev <= max_dev; dev++) {
> +               printf("%3d ", dev);
> +               block_dev_desc_t *blk_dev = host_get_dev(dev);
> +               if (!blk_dev)
> +                       continue;

Does this case ever happen? If so you don't print \n.

> +               struct host_block_dev *host_dev = blk_dev->priv;

Maybe leave the assignment here but put the declaration at the start
of the block.

> +               printf("%12lu %s\n", (unsigned long)blk_dev->lba,
> +                      host_dev->filename);
> +       }
> +       return 0;
> +}
> +
>  static cmd_tbl_t cmd_sandbox_sub[] = {
>         U_BOOT_CMD_MKENT(load, 7, 0, do_sandbox_load, "", ""),
>         U_BOOT_CMD_MKENT(ls, 3, 0, do_sandbox_ls, "", ""),
>         U_BOOT_CMD_MKENT(save, 6, 0, do_sandbox_save, "", ""),
> +       U_BOOT_CMD_MKENT(bind, 3, 0, do_sandbox_bind, "", ""),
> +       U_BOOT_CMD_MKENT(info, 3, 0, do_sandbox_info, "", ""),
>  };
>
>  static int do_sandbox(cmd_tbl_t *cmdtp, int flag, int argc,
> @@ -70,4 +122,6 @@ U_BOOT_CMD(
>         "sb ls host <filename>                      - list files on host\n"
>         "sb save host <dev> <filename> <addr> <bytes> [<offset>] - "
>                 "save a file to host\n"
> +       "sb bind <dev> [<filename>] - bind \"host\" device to file\n"
> +       "sb info [<dev>]            - show device binding & info"
>  );
> diff --git a/disk/part.c b/disk/part.c
> index d73625c..5906aef 100644
> --- a/disk/part.c
> +++ b/disk/part.c
> @@ -59,6 +59,9 @@ static const struct block_drvr block_drvr[] = {
>  #if defined(CONFIG_SYSTEMACE)
>         { .name = "ace", .get_dev = systemace_get_dev, },
>  #endif
> +#if defined(CONFIG_SANDBOX)
> +       { .name = "host", .get_dev = host_get_dev, },
> +#endif
>         { },
>  };
>
> @@ -302,6 +305,9 @@ static void print_part_header (const char *type, block_dev_desc_t * dev_desc)
>         case IF_TYPE_MMC:
>                 puts ("MMC");
>                 break;
> +       case IF_TYPE_HOST:
> +               puts("HOST");
> +               break;
>         default:
>                 puts ("UNKNOWN");
>                 break;
> @@ -462,23 +468,6 @@ int get_device_and_partition(const char *ifname, const char *dev_part_str,
>         int part;
>         disk_partition_t tmpinfo;
>
> -       /*
> -        * For now, we have a special case for sandbox, since there is no
> -        * real block device support.
> -        */
> -       if (0 == strcmp(ifname, "host")) {
> -               *dev_desc = NULL;
> -               info->start = info->size =  info->blksz = 0;
> -               info->bootable = 0;
> -               strcpy((char *)info->type, BOOT_PART_TYPE);
> -               strcpy((char *)info->name, "Sandbox host");
> -#ifdef CONFIG_PARTITION_UUIDS
> -               info->uuid[0] = 0;
> -#endif
> -
> -               return 0;
> -       }
> -
>         /* If no dev_part_str, use bootdevice environment variable */
>         if (!dev_part_str || !strlen(dev_part_str) ||
>             !strcmp(dev_part_str, "-"))
> diff --git a/drivers/block/Makefile b/drivers/block/Makefile
> index f1ebdcc..2d2fb55 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_SANDBOX) += sandbox.o

Alphabetic order so this should go up two.

>
>  COBJS  := $(COBJS-y)
>  SRCS   := $(COBJS:.o=.c)
> diff --git a/drivers/block/sandbox.c b/drivers/block/sandbox.c
> new file mode 100644
> index 0000000..90f7dca
> --- /dev/null
> +++ b/drivers/block/sandbox.c
> @@ -0,0 +1,127 @@
> +/*
> + * Copyright (C) 2013 Henrik Nordstrom <henrik at henriknordstrom.net>
> + *
> + * 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 <config.h>
> +#include <common.h>
> +#include <part.h>
> +#include <os.h>
> +#include <malloc.h>
> +#include <sandboxblockdev.h>

How about sandbox-blockdev.h

> +
> +static struct host_block_dev host_devices[CONFIG_HOST_MAX_DEVICES];
> +
> +static struct host_block_dev *
> +find_host_device(int dev)
> +{
> +       if (dev >= 0 && dev < CONFIG_HOST_MAX_DEVICES)
> +               return &host_devices[dev];
> +
> +       printf("Invalid host device number\n");

puts() when you don't have format args. Please fix globally.

> +       return NULL;
> +}
> +
> +static unsigned long host_block_read(int dev, unsigned long start,
> +                                    lbaint_t blkcnt, void *buffer)
> +{
> +       struct host_block_dev *host_dev = find_host_device(dev);
> +       if (os_lseek(host_dev->fd,
> +                    start * host_dev->blk_dev.blksz,
> +                    OS_SEEK_SET) == -1) {
> +               printf("ERROR: Invalid position\n");
> +               return -1;
> +       }
> +       ssize_t len = os_read(host_dev->fd, buffer,
> +                             blkcnt * host_dev->blk_dev.blksz);
> +       if (len >= 0)
> +               return len / host_dev->blk_dev.blksz;
> +       return -1;
> +}
> +
> +static unsigned long host_block_write(int dev, unsigned long start,
> +                                     lbaint_t blkcnt, const void *buffer)
> +{
> +       struct host_block_dev *host_dev = find_host_device(dev);
> +       if (os_lseek(host_dev->fd,
> +                    start * host_dev->blk_dev.blksz,
> +                    OS_SEEK_SET) == -1) {
> +               printf("ERROR: Invalid position\n");
> +               return -1;
> +       }
> +       ssize_t len = os_write(host_dev->fd, buffer, blkcnt *
> +                              host_dev->blk_dev.blksz);
> +       if (len >= 0)
> +               return len / host_dev->blk_dev.blksz;
> +       return -1;
> +}
> +
> +int host_dev_bind(int dev, char *filename)
> +{
> +       struct host_block_dev *host_dev = find_host_device(dev);
> +       if (host_dev->blk_dev.priv) {
> +               os_close(host_dev->fd);
> +               host_dev->blk_dev.priv = NULL;
> +       }
> +       if (host_dev->filename)
> +               free(host_dev->filename);
> +       if (filename && *filename) {
> +               host_dev->filename = strdup(filename);
> +       } else {
> +               host_dev->filename = NULL;
> +               return 0;
> +       }
> +
> +       host_dev->fd = os_open(host_dev->filename, OS_O_RDWR);
> +       if (host_dev->fd == -1) {
> +               printf("Failed to access host backing file '%s'\n",
> +                      host_dev->filename);
> +               return 1;

-ENOENT might be better (include errno.h)

> +       }
> +
> +       block_dev_desc_t *blk_dev = &host_dev->blk_dev;
> +       blk_dev->if_type = IF_TYPE_HOST;
> +       blk_dev->priv = host_dev;
> +       blk_dev->blksz = 512;
> +       blk_dev->lba = os_lseek(host_dev->fd, 0, OS_SEEK_END) / blk_dev->blksz;
> +       blk_dev->block_read = host_block_read;
> +       blk_dev->block_write = host_block_write;
> +       blk_dev->dev = dev;
> +       blk_dev->part_type = PART_TYPE_UNKNOWN;
> +       init_part(blk_dev);
> +
> +       return 0;
> +}
> +
> +block_dev_desc_t *host_get_dev(int dev)
> +{
> +       struct host_block_dev *host_dev = find_host_device(dev);
> +
> +       if (!host_dev)
> +               return NULL;
> +
> +       if (!host_dev->blk_dev.priv) {
> +               printf("Not bound to a backing file\n");
> +               return NULL;
> +       }
> +
> +       block_dev_desc_t *blk_dev = &host_dev->blk_dev;
> +       return blk_dev;
> +}
> diff --git a/include/config_fallbacks.h b/include/config_fallbacks.h
> index e59ee96..9ea8c09 100644
> --- a/include/config_fallbacks.h
> +++ b/include/config_fallbacks.h
> @@ -49,7 +49,8 @@
>         defined(CONFIG_CMD_USB) || \
>         defined(CONFIG_CMD_PART) || \
>         defined(CONFIG_MMC) || \
> -       defined(CONFIG_SYSTEMACE)
> +       defined(CONFIG_SYSTEMACE) || \
> +       defined(CONFIG_SANDBOX)
>  #define HAVE_BLOCK_DEVICE
>  #endif
>
> diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h
> index 788207d..844a49f 100644
> --- a/include/configs/sandbox.h
> +++ b/include/configs/sandbox.h
> @@ -38,6 +38,11 @@
>  #define CONFIG_CMD_FAT
>  #define CONFIG_CMD_EXT4
>  #define CONFIG_CMD_EXT4_WRITE
> +#define CONFIG_CMD_PART
> +#define CONFIG_DOS_PARTITION
> +#define CONFIG_PARTITION_UUIDS
> +#define CONFIG_EFI_PARTITION
> +#define CONFIG_HOST_MAX_DEVICES 4
>
>  #define CONFIG_SYS_VSNPRINTF
>
> @@ -45,6 +50,7 @@
>  #define CONFIG_SANDBOX_GPIO
>  #define CONFIG_SANDBOX_GPIO_COUNT      20
>
> +
>  /*
>   * Size of malloc() pool, although we don't actually use this yet.
>   */
> diff --git a/include/part.h b/include/part.h
> index f7c7cc5..a29d615 100644
> --- a/include/part.h
> +++ b/include/part.h
> @@ -74,6 +74,7 @@ typedef struct block_dev_desc {
>  #define IF_TYPE_MMC            6
>  #define IF_TYPE_SD             7
>  #define IF_TYPE_SATA           8
> +#define IF_TYPE_HOST           9
>
>  /* Part types */
>  #define PART_TYPE_UNKNOWN      0x00
> @@ -118,6 +119,7 @@ block_dev_desc_t* usb_stor_get_dev(int dev);
>  block_dev_desc_t* mmc_get_dev(int dev);
>  block_dev_desc_t* systemace_get_dev(int dev);
>  block_dev_desc_t* mg_disk_get_dev(int dev);
> +block_dev_desc_t *host_get_dev(int dev);
>
>  /* disk/part.c */
>  int get_partition_info (block_dev_desc_t * dev_desc, int part, disk_partition_t *info);
> @@ -139,6 +141,7 @@ static inline block_dev_desc_t* usb_stor_get_dev(int dev) { return NULL; }
>  static inline block_dev_desc_t* mmc_get_dev(int dev) { return NULL; }
>  static inline block_dev_desc_t* systemace_get_dev(int dev) { return NULL; }
>  static inline block_dev_desc_t* mg_disk_get_dev(int dev) { return NULL; }
> +static inline block_dev_desc_t *host_get_dev(int dev) { return NULL; }
>
>  static inline int get_partition_info (block_dev_desc_t * dev_desc, int part,
>         disk_partition_t *info) { return -1; }
> diff --git a/include/sandboxblockdev.h b/include/sandboxblockdev.h
> new file mode 100644
> index 0000000..8723062
> --- /dev/null
> +++ b/include/sandboxblockdev.h
> @@ -0,0 +1,29 @@
> +/*
> + * Copyright (c) 2013, Henrik Nordstrom <henrik at henriknordstrom.net>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * 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., 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +#ifndef __SANDBOX_BLOCK_DEV__
> +#define __SANDBOX_BLOCK_DEV__
> +
> +struct host_block_dev {
> +       block_dev_desc_t blk_dev;
> +       char *filename;
> +       int fd;
> +};
> +
> +int host_dev_bind(int dev, char *filename);

Please add a comment as to what this function does, describing the
meaning and valid values for each argument.

> +
> +#endif
> --
> 1.8.1.4
>
>
>

For testing, I tried:

=>sb bind 0 chromiumos_test_image.bin
=>sb info
dev       blocks path
  0      4956096 chromiumos_test_image.bin
  1 Not bound to a backing file
  2 Not bound to a backing file
  3 Not bound to a backing file
=>part list host 0
(works fine)
=>ext4load host 0:3
Segmentation fault (core dumped)

(not sure why that is...maybe a bug?)

FAT works though:

=>fatls host 0:c
            u-boot/
  3451512   vmlinuz.uimg.a
  3250192   vmlinuz.a

2 file(s), 1 dir(s)

=>

Regards,
Simon


More information about the U-Boot mailing list