[U-Boot] [PATCH] sandbox: block driver using host file/device as backing store
A simple "host" block driver using any host file/device as backing store.
The mapping is established using the "sb bind" command
Signed-off-by: Henrik Nordstrom henrik@henriknordstrom.net --- common/cmd_sandbox.c | 10 +++- disk/part.c | 20 ++----- drivers/block/Makefile | 1 + drivers/block/sandbox.c | 130 +++++++++++++++++++++++++++++++++++++++++++++ include/config_fallbacks.h | 3 +- include/configs/sandbox.h | 2 + include/part.h | 3 ++ 7 files changed, 150 insertions(+), 19 deletions(-) create mode 100644 drivers/block/sandbox.c
diff --git a/common/cmd_sandbox.c b/common/cmd_sandbox.c index 206a486..492d569 100644 --- a/common/cmd_sandbox.c +++ b/common/cmd_sandbox.c @@ -32,9 +32,16 @@ static int do_sandbox_ls(cmd_tbl_t *cmdtp, int flag, int argc, return do_ls(cmdtp, flag, argc, argv, FS_TYPE_SANDBOX); }
+static int do_sandbox_bind(cmd_tbl_t *cmdtp, int flag, int argc, + char * const argv[]) +{ + return host_dev_bind(atoi(argv[1]), argv[2]); +} + static cmd_tbl_t cmd_sandbox_sub[] = { U_BOOT_CMD_MKENT(load, 3, 0, do_sandbox_load, "", ""), U_BOOT_CMD_MKENT(ls, 3, 0, do_sandbox_ls, "", ""), + U_BOOT_CMD_MKENT(bind, 3, 0, do_sandbox_bind, "", ""), };
static int do_sandbox(cmd_tbl_t *cmdtp, int flag, int argc, @@ -59,5 +66,6 @@ U_BOOT_CMD( sb, 6, 1, do_sandbox, "Miscellaneous sandbox commands", "load host <addr> <filename> [<bytes> <offset>] - load a file from host\n" - "sb ls host <filename> - save a file to host" + "ls host <filename> - save a file to host\n" + "bind dev <filename> - bind "host" device to file" ); diff --git a/disk/part.c b/disk/part.c index d73625c..648839b 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 { }, };
@@ -462,23 +465,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
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..185cee8 --- /dev/null +++ b/drivers/block/sandbox.c @@ -0,0 +1,130 @@ +/* + * Copyright (C) 2013 Henrik Nordstrom henrik@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> + +#ifndef CONFIG_HOST_MAX_DEVICES +#define CONFIG_HOST_MAX_DEVICES 4 +#endif + +static struct host_block_dev { + block_dev_desc_t blk_dev; + char *filename; + int fd; +} 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"); + 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; +} + +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->filename) { + printf("Not bound to a backing file\n"); + return NULL; + } + + block_dev_desc_t *blk_dev = &host_dev->blk_dev; + if (!host_dev->blk_dev.priv) { + 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 NULL; + } + 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 blk_dev; +} diff --git a/include/config_fallbacks.h b/include/config_fallbacks.h index 269b5c2..33affd7 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 406da43..b73fbac 100644 --- a/include/configs/sandbox.h +++ b/include/configs/sandbox.h @@ -39,6 +39,8 @@ #define CONFIG_CMD_EXT4 #define CONFIG_CMD_EXT4_WRITE
+#define CONFIG_DOS_PARTITION + #define CONFIG_SYS_VSNPRINTF
#define CONFIG_CMD_GPIO diff --git a/include/part.h b/include/part.h index c58a734..40a34c8 100644 --- a/include/part.h +++ b/include/part.h @@ -65,6 +65,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 @@ -109,6 +110,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); @@ -130,6 +132,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; }
Hi Henrik,
On Tue, May 14, 2013 at 4:36 PM, Henrik Nordström henrik@henriknordstrom.net wrote:
A simple "host" block driver using any host file/device as backing store.
The mapping is established using the "sb bind" command
Signed-off-by: Henrik Nordstrom henrik@henriknordstrom.net
common/cmd_sandbox.c | 10 +++- disk/part.c | 20 ++----- drivers/block/Makefile | 1 + drivers/block/sandbox.c | 130 +++++++++++++++++++++++++++++++++++++++++++++ include/config_fallbacks.h | 3 +- include/configs/sandbox.h | 2 + include/part.h | 3 ++ 7 files changed, 150 insertions(+), 19 deletions(-) create mode 100644 drivers/block/sandbox.c
This is cool, a great addition to sandbox. Thanks for doing it.
Some high-level comments:
Configuring for sandbox board... sandbox.c: In function ‘host_dev_bind’: sandbox.c:90:3: warning: implicit declaration of function ‘free’ [-Wimplicit-function-declaration] cmd_sandbox.c: In function ‘do_sandbox_bind’: cmd_sandbox.c:44:2: warning: implicit declaration of function ‘host_dev_bind’ [-Wimplicit-function-declaration] cmd_sandbox.c:44:2: warning: implicit declaration of function ‘atoi’ [-Wimplicit-function-declaration]
- should not have any warnings
=>sb bind Segmentation fault (core dumped)
- should print a nice error if an arg is missing
=>sb bind fred =>
- should print an error if the file is not found
- should create an example of how to use this
- suggest a test script in test/sandbox which sets up a loopback device containing a partition table and ext2 filesystem (for example, then runs U-Boot sandbox and lists and reads a file. Example test scripts you might copy are:
http://patchwork.ozlabs.org/patch/228876/
and this rather more complicated one:
http://patchwork.ozlabs.org/patch/211049/
Also please enable partitions and EFI support so we get more functionality:
#define CONFIG_PARTITION_UUIDS #define CONFIG_CMD_PART #define CONFIG_EFI_PARTITION
Here is the test I did:
$ ./x/u-boot
U-Boot 2013.04-00139-g5a35ef9 (May 15 2013 - 05:47:23)
DRAM: 128 MiB Using default environment
In: serial Out: serial Err: serial =>sb bind dev chromiumos_test_image.bin =>part list host 0
Partition Map for UNKNOWN device 0 -- Partition Type: EFI
Part Start LBA End LBA Name Attributes Type UUID Partition UUID 1 0x002b2000 0x004b1fff "STATE" attrs: 0x0000000000000000 type: ebd0a0a2-b9e5-4433-87c0-68b6b72699c7 uuid: 8e484e16-9146-a540-aa05-4a01d5b2708e 2 0x00005000 0x0000cfff "KERN-A" attrs: 0x01ff000000000000 type: fe3a2a5d-4f32-41a7-b725-accc3285a309 uuid: 3c2cce96-351d-1442-b443-180979877d50 3 0x00046000 0x002b1fff "ROOT-A" attrs: 0x0000000000000000 type: 3cb8e202-3b7e-47dd-8a3c-7ff2a13cfcec uuid: ae059dce-752f-934a-a9f5-2e2ae329a763 4 0x0000d000 0x00014fff "KERN-B" attrs: 0x00f0000000000000 type: fe3a2a5d-4f32-41a7-b725-accc3285a309 uuid: bdc023ba-749e-6a41-8328-994a67193adf 5 0x00045000 0x00045fff "ROOT-B" attrs: 0x0000000000000000 type: 3cb8e202-3b7e-47dd-8a3c-7ff2a13cfcec uuid: 8055a9ee-41e9-f443-8f6e-aff694b92d01 6 0x00004040 0x00004040 "KERN-C" attrs: 0x00f0000000000000 type: fe3a2a5d-4f32-41a7-b725-accc3285a309 uuid: 551febf2-23f5-6047-811b-3ba38498e99d 7 0x00004041 0x00004041 "ROOT-C" attrs: 0x0000000000000000 type: 3cb8e202-3b7e-47dd-8a3c-7ff2a13cfcec uuid: c26b8ef3-8d80-974e-8871-62b4ef521dda 8 0x00015000 0x0001cfff "OEM" attrs: 0x0000000000000000 type: ebd0a0a2-b9e5-4433-87c0-68b6b72699c7 uuid: 9bc99b8d-00a8-9d48-9897-c328a06299cd 9 0x00004042 0x00004042 "reserved" attrs: 0x0000000000000000 type: 2e0a753d-9e48-43b0-8337-b15192cb1b5e uuid: 796ef203-c110-dc41-acd1-aea3c1a9d040 10 0x00004043 0x00004043 "reserved" attrs: 0x0000000000000000 type: 2e0a753d-9e48-43b0-8337-b15192cb1b5e uuid: 3e086568-db49-6a42-8022-6ce0e4cfb989 11 0x00000040 0x0000403f "RWFW" attrs: 0x0000000000000000 type: cab6e88e-abf3-4102-a07a-d4bb9be3c1d3 uuid: eefcac05-3fba-0c4a-a519-8db04a10cd1f 12 0x0003d000 0x00044fff "EFI-SYSTEM" attrs: 0x0000000000000000 type: c12a7328-f81f-11d2-ba4b-00a0c93ec93b uuid: 6ddce429-6592-8144-a1c2-d4ac818fbe3e =>reset
Regards, Simon
ons 2013-05-15 klockan 10:42 -0700 skrev Simon Glass:
Some high-level comments:
Configuring for sandbox board... sandbox.c: In function ‘host_dev_bind’:
- should not have any warnings
Shame on me :)
Fixed now, and should have before submitting for review.
=>sb bind Segmentation fault (core dumped)
- should print a nice error if an arg is missing
Right. done.
And correct usage is sb bind 0 file (devices 0-3 supported, arbitrary limit of 4 currently)
have corrected the help message to reflect this.
=>sb bind fred =>
- should print an error if the file is not found
In this version it opens the file on access and you get the error then. Have now moved that to the bind operation which gives immediate feedback.
Initially I thought of having some predefined device backing names but it's better to always use the sb bind command to establish the backing name.
Have also added and "sb info" command to show the bound devices.
- should create an example of how to use this
Yes. Is there any existing documentation for sandbox somewhere to add to?
- suggest a test script in test/sandbox which sets up a loopback
device containing a partition table and ext2 filesystem (for example, then runs U-Boot sandbox and lists and reads a file. Example test scripts you might copy are:
http://patchwork.ozlabs.org/patch/228876/
and this rather more complicated one:
Ok. I'll look into this.
Also please enable partitions and EFI support so we get more functionality:
#define CONFIG_PARTITION_UUIDS #define CONFIG_CMD_PART #define CONFIG_EFI_PARTITION
Ok, can do this. Didn't want to enable too much as part of the block driver patch.
Done.
Current version of the patch is at https://github.com/hno/u-boot/commit/9b5f9b762a6e901f599b7ae1c9946806a442292...
will submit a new copy to the mailinglist after adding documentation & testsuite.
Regards Henrik
Version 2 of this patch addressing the code comments by Simon and adding some small missing pieces.
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@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); + 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; + int min_dev = 0; + int max_dev = CONFIG_HOST_MAX_DEVICES - 1; + if (argc >= 2) { + char *ep; + char *dev_str = argv[1]; + int dev = simple_strtoul(dev_str, &ep, 16); + if (*ep) { + printf("** Bad device specification %s **\n", dev_str); + return CMD_RET_USAGE; + } + min_dev = dev; + max_dev = dev; + } + int dev; + 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; + struct host_block_dev *host_dev = blk_dev->priv; + 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
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@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> + +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"); + 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; + } + + 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@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); + +#endif
Hi Henrik,
On Wed, May 15, 2013 at 2:54 PM, Henrik Nordström henrik@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@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@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@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
ons 2013-05-15 klockan 15:20 -0700 skrev Simon Glass:
Hi Henrik,
On Wed, May 15, 2013 at 2:54 PM, Henrik Nordström henrik@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.
Just looked into it and looks nice. Giving it a try for next version.
Took a little while to get started, mostly because it crashes & burns when not finding an matching alias for sandbox.
blank line here, please fix globally (a blank line between declarations and code).
Ok.
+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
Ok. Also restrucured do_sandbox_bind a little to match this. There one of the declarations depends on this being checked first.
Move to top of function. Try to collect your declarations within a block when it's easy to do so.
Ok.
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.
Yes. And it then relies on the driver to print an error.
struct host_block_dev *host_dev = blk_dev->priv;Maybe leave the assignment here but put the declaration at the start of the block.
Yes.
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.
up seven. sorted on filename here not CONFIG_..
+#include <sandboxblockdev.h>
How about sandbox-blockdev.h
Yee.
puts() when you don't have format args. Please fix globally.
Ok.
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)
or maybe just -errno?
and handle the error in do_sandbox_bind().
+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.
Ok.
=>ext4load host 0:3 Segmentation fault (core dumped)
Doesn't ext2load expect a address & filename to load?
Regads Henrik
Hi Henrik,
On Wed, May 15, 2013 at 6:09 PM, Henrik Nordström henrik@henriknordstrom.net wrote:
ons 2013-05-15 klockan 15:20 -0700 skrev Simon Glass:
Hi Henrik,
On Wed, May 15, 2013 at 2:54 PM, Henrik Nordström henrik@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.
Just looked into it and looks nice. Giving it a try for next version.
Took a little while to get started, mostly because it crashes & burns when not finding an matching alias for sandbox.
blank line here, please fix globally (a blank line between declarations and code).
Ok.
+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
Ok. Also restrucured do_sandbox_bind a little to match this. There one of the declarations depends on this being checked first.
Move to top of function. Try to collect your declarations within a block when it's easy to do so.
Ok.
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.
Yes. And it then relies on the driver to print an error.
struct host_block_dev *host_dev = blk_dev->priv;Maybe leave the assignment here but put the declaration at the start of the block.
Yes.
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.
up seven. sorted on filename here not CONFIG_..
+#include <sandboxblockdev.h>
How about sandbox-blockdev.h
Yee.
puts() when you don't have format args. Please fix globally.
Ok.
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)
or maybe just -errno?
and handle the error in do_sandbox_bind().
+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.
Ok.
=>ext4load host 0:3 Segmentation fault (core dumped)
Doesn't ext2load expect a address & filename to load?
Sorry I meant:
=>ext4ls host 0:3 Segmentation fault (core dumped)
It may not be your code, but I think the segfault is there.
Regads Henrik
Regards, Simon
tor 2013-05-16 klockan 21:53 -0700 skrev Simon Glass:
Sorry I meant:
=>ext4ls host 0:3 Segmentation fault (core dumped)
It may not be your code, but I think the segfault is there.
It's crashing in ext4 for me.
#0 ext4fs_set_blk_dev (rbdd=0x6434a0 <host_devices>, info=info@entry=0x643980 <fs_partition>) at dev.c:56
#0 ext4fs_set_blk_dev (rbdd=0x6434a0 <host_devices>, info=info@entry=0x643980 <fs_partition>) at dev.c:56 56 get_fs()->total_sect = (info->size * info->blksz) >> 57 get_fs()->dev_desc->log2blksz; 58 get_fs()->dev_desc = rbdd;
p(gdb) p get_fs() $2 = (struct ext_filesystem *) 0x644410 <ext_fs> (gdb) p $2->dev_desc $5 = (block_dev_desc_t *) 0x0
Looks like a generic ext4 bug in the new block size handling code.
I think it just needs to be shuffled around a bit so the dev_desc is assigned before, not after.
Regards Henrik
Hi Henrik,
On Sat, May 18, 2013 at 7:24 AM, Henrik Nordström henrik@henriknordstrom.net wrote:
tor 2013-05-16 klockan 21:53 -0700 skrev Simon Glass:
Sorry I meant:
=>ext4ls host 0:3 Segmentation fault (core dumped)
It may not be your code, but I think the segfault is there.
It's crashing in ext4 for me.
#0 ext4fs_set_blk_dev (rbdd=0x6434a0 <host_devices>, info=info@entry=0x643980 <fs_partition>) at dev.c:56
#0 ext4fs_set_blk_dev (rbdd=0x6434a0 <host_devices>, info=info@entry=0x643980 <fs_partition>) at dev.c:56 56 get_fs()->total_sect = (info->size * info->blksz) >> 57 get_fs()->dev_desc->log2blksz; 58 get_fs()->dev_desc = rbdd;
p(gdb) p get_fs() $2 = (struct ext_filesystem *) 0x644410 <ext_fs> (gdb) p $2->dev_desc $5 = (block_dev_desc_t *) 0x0
Looks like a generic ext4 bug in the new block size handling code.
I think it just needs to be shuffled around a bit so the dev_desc is assigned before, not after.
Well that's a great bug to find.
It isn't related to your patch though - if you have time it would be nice to get a separate patch to fix that bug.
Regards Henrik
Regards, Simon
HI Henrik,
On Thu, May 16, 2013 at 9:53 PM, Simon Glass sjg@chromium.org wrote:
Hi Henrik,
On Wed, May 15, 2013 at 6:09 PM, Henrik Nordström henrik@henriknordstrom.net wrote:
ons 2013-05-15 klockan 15:20 -0700 skrev Simon Glass:
Hi Henrik,
On Wed, May 15, 2013 at 2:54 PM, Henrik Nordström henrik@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.
Just looked into it and looks nice. Giving it a try for next version.
Did you send a new version? I might have missed it.
Regards, Simon
sön 2013-06-16 klockan 07:50 -0700 skrev Simon Glass:
Did you send a new version? I might have missed it.
Not yet. Work got in a bit of a panic mode and then been ill for a while. Not forgotten.
Regards Henrik
Hi Henrik,
On Sun, Jun 16, 2013 at 7:39 PM, Henrik Nordström < henrik@henriknordstrom.net> wrote:
sön 2013-06-16 klockan 07:50 -0700 skrev Simon Glass:
Did you send a new version? I might have missed it.
Not yet. Work got in a bit of a panic mode and then been ill for a while. Not forgotten.
FYI I think there the blksz field needs to be set also, in drivers/block/sandbox.c. I am using:
... blk_dev->priv = host_dev; blk_dev->log2blksz = 9; blk_dev->blksz = 1 << blk_dev->log2blksz; blk_dev->lba = os_lseek(host_dev->fd, 0, OS_SEEK_END) / blk_dev->blksz; ...
Regards, Simon
participants (2)
-
Henrik Nordström -
Simon Glass