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

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

[Henrik Nordström]

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; }
-- 
1.8.1.4

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

[Simon Glass]

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

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

[Henrik Nordström]

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:
> 
> http://patchwork.ozlabs.org/patch/211049/

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/9b5f9b762a6e901f599b7ae1c9946806a4422929.patch

will submit a new copy to the mailinglist after adding documentation &
testsuite.

Regards
Henrik

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

[Henrik Nordström]

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

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

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

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

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

[Henrik Nordström]

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

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

[Simon Glass]

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

[U-Boot] sandbox: block driver using host file/device as backing store, crash in ext4

[Henrik Nordström]

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 at entry=0x643980 <fs_partition>) at dev.c:56

#0  ext4fs_set_blk_dev (rbdd=0x6434a0 <host_devices>, 
    info=info at 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

[U-Boot] sandbox: block driver using host file/device as backing store, crash in ext4

[Simon Glass]

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 at entry=0x643980 <fs_partition>) at dev.c:56
>
> #0  ext4fs_set_blk_dev (rbdd=0x6434a0 <host_devices>,
>     info=info at 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

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

[Simon Glass]

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

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

[Henrik Nordström]

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

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

[Simon Glass]

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