[U-Boot] [PATCH V3 01/12] fs: fix generic save command implementation

[U-Boot] [PATCH V3 01/12] fs: fix generic save command implementation

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

Fix a few issues with the generic "save" shell command, and fs_write()
function.

1) fstypes[].write wasn't filled in for some file-systems, and isn't
   checked when used, which could cause crashes/... if executing save
   on e.g. fat/ext filesystems.

2) fs_write() requires the length argument to be non-zero, since it needs
   to know exactly how many bytes to write. Adjust the comments and code
   according to this.

3) fs_write() wasn't prototyped in <fs.h> like other generic functions;
   other code should be able to call this directly rather than invoking
   the "save" shell command.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
Acked-by: Simon Glass <sjg@chromium.org>
---
v3: No change.
v2: No change.
---
 fs/fs.c      |  9 +++------
 include/fs.h | 10 ++++++++++
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/fs/fs.c b/fs/fs.c
index be1855d1291f..9c2ef6b6597c 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -75,6 +75,7 @@ static struct fstype_info fstypes[] = {
 		.close = fat_close,
 		.ls = file_fat_ls,
 		.read = fat_read_file,
+		.write = fs_write_unsupported,
 	},
 #endif
 #ifdef CONFIG_FS_EXT4
@@ -84,6 +85,7 @@ static struct fstype_info fstypes[] = {
 		.close = ext4fs_close,
 		.ls = ext4fs_ls,
 		.read = ext4_read_file,
+		.write = fs_write_unsupported,
 	},
 #endif
 #ifdef CONFIG_SANDBOX
@@ -212,16 +214,11 @@ int fs_write(const char *filename, ulong addr, int offset, int len)
 	void *buf;
 	int ret;
 
-	/*
-	 * We don't actually know how many bytes are being read, since len==0
-	 * means read the whole file.
-	 */
 	buf = map_sysmem(addr, len);
 	ret = info->write(filename, buf, offset, len);
 	unmap_sysmem(buf);
 
-	/* If we requested a specific number of bytes, check we got it */
-	if (ret >= 0 && len && ret != len) {
+	if (ret >= 0 && ret != len) {
 		printf("** Unable to write file %s **\n", filename);
 		ret = -1;
 	}
diff --git a/include/fs.h b/include/fs.h
index 7d9403ed8758..97b0094e954b 100644
--- a/include/fs.h
+++ b/include/fs.h
@@ -55,6 +55,16 @@ int fs_ls(const char *dirname);
 int fs_read(const char *filename, ulong addr, int offset, int len);
 
 /*
+ * Write file "filename" to the partition previously set by fs_set_blk_dev(),
+ * from address "addr", starting at byte offset "offset", and writing "len"
+ * bytes. "offset" may be 0 to write to the start of the file. Note that not
+ * all filesystem types support offset!=0.
+ *
+ * Returns number of bytes read on success. Returns <= 0 on error.
+ */
+int fs_write(const char *filename, ulong addr, int offset, int len);
+
+/*
  * Common implementation for various filesystem commands, optionally limited
  * to a specific filesystem type via the fstype parameter.
  */
-- 
1.8.1.5

[U-Boot] [PATCH V3 02/12] fs: implement infra-structure for an 'exists' function

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

This could be used in scripts such as:

if test -e mmc 0:1 /boot/boot.scr; then
    load mmc 0:1 ${scriptaddr} /boot/boot.scr
    source ${scriptaddr}
fi

rather than:

if load mmc 0:1 ${scriptaddr} /boot/boot.scr; then
    source ${scriptaddr}
fi

This prevents errors being printed by attempts to load non-existent
files, which can be important when checking for a large set of files,
such as /boot/boot.scr.uimg, /boot/boot.scr, /boot/extlinux.conf,
/boot.scr.uimg, /boot.scr, /extlinux.conf.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
Acked-by: Simon Glass <sjg@chromium.org>
---
v3:
* Remove addition of "exists" command; it's implemented in a separate
  patch later, as an operator in the "test" command.
* Invert return value of fs_exists()/file_exists() so it returns a
  logical value; the mapping to shell command return value is performed
  (later) in the test command.
v2: No change.
---
 fs/fs.c      | 32 ++++++++++++++++++++++++++++++++
 include/fs.h |  9 +++++++++
 2 files changed, 41 insertions(+)

diff --git a/fs/fs.c b/fs/fs.c
index 9c2ef6b6597c..8fe2403a46ae 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -41,6 +41,11 @@ static inline int fs_ls_unsupported(const char *dirname)
 	return -1;
 }
 
+static inline int fs_exists_unsupported(const char *filename)
+{
+	return 0;
+}
+
 static inline int fs_read_unsupported(const char *filename, void *buf,
 				      int offset, int len)
 {
@@ -62,6 +67,7 @@ struct fstype_info {
 	int (*probe)(block_dev_desc_t *fs_dev_desc,
 		     disk_partition_t *fs_partition);
 	int (*ls)(const char *dirname);
+	int (*exists)(const char *filename);
 	int (*read)(const char *filename, void *buf, int offset, int len);
 	int (*write)(const char *filename, void *buf, int offset, int len);
 	void (*close)(void);
@@ -74,6 +80,7 @@ static struct fstype_info fstypes[] = {
 		.probe = fat_set_blk_dev,
 		.close = fat_close,
 		.ls = file_fat_ls,
+		.exists = fs_exists_unsupported,
 		.read = fat_read_file,
 		.write = fs_write_unsupported,
 	},
@@ -84,6 +91,7 @@ static struct fstype_info fstypes[] = {
 		.probe = ext4fs_probe,
 		.close = ext4fs_close,
 		.ls = ext4fs_ls,
+		.exists = fs_exists_unsupported,
 		.read = ext4_read_file,
 		.write = fs_write_unsupported,
 	},
@@ -94,6 +102,7 @@ static struct fstype_info fstypes[] = {
 		.probe = sandbox_fs_set_blk_dev,
 		.close = sandbox_fs_close,
 		.ls = sandbox_fs_ls,
+		.exists = fs_exists_unsupported,
 		.read = fs_read_sandbox,
 		.write = fs_write_sandbox,
 	},
@@ -103,6 +112,7 @@ static struct fstype_info fstypes[] = {
 		.probe = fs_probe_unsupported,
 		.close = fs_close_unsupported,
 		.ls = fs_ls_unsupported,
+		.exists = fs_exists_unsupported,
 		.read = fs_read_unsupported,
 		.write = fs_write_unsupported,
 	},
@@ -184,6 +194,19 @@ int fs_ls(const char *dirname)
 	return ret;
 }
 
+int fs_exists(const char *filename)
+{
+	int ret;
+
+	struct fstype_info *info = fs_get_info(fs_type);
+
+	ret = info->exists(filename);
+
+	fs_close();
+
+	return ret;
+}
+
 int fs_read(const char *filename, ulong addr, int offset, int len)
 {
 	struct fstype_info *info = fs_get_info(fs_type);
@@ -309,6 +332,15 @@ int do_ls(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
 	return 0;
 }
 
+int file_exists(const char *dev_type, const char *dev_part, const char *file,
+		int fstype)
+{
+	if (fs_set_blk_dev(dev_type, dev_part, fstype))
+		return 0;
+
+	return fs_exists(file);
+}
+
 int do_save(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
 		int fstype)
 {
diff --git a/include/fs.h b/include/fs.h
index 97b0094e954b..26de0539f7d9 100644
--- a/include/fs.h
+++ b/include/fs.h
@@ -44,6 +44,13 @@ int fs_set_blk_dev(const char *ifname, const char *dev_part_str, int fstype);
 int fs_ls(const char *dirname);
 
 /*
+ * Determine whether a file exists
+ *
+ * Returns 1 if the file exists, 0 if it doesn't exist.
+ */
+int fs_exists(const char *filename);
+
+/*
  * Read file "filename" from the partition previously set by fs_set_blk_dev(),
  * to address "addr", starting at byte offset "offset", and reading "len"
  * bytes. "offset" may be 0 to read from the start of the file. "len" may be
@@ -72,6 +79,8 @@ int do_load(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
 		int fstype);
 int do_ls(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
 		int fstype);
+int file_exists(const char *dev_type, const char *dev_part, const char *file,
+		int fstype);
 int do_save(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[],
 		int fstype);
 
-- 
1.8.1.5

[U-Boot] [PATCH V3 03/12] fs: don't pass NULL dev_desc to most filesystems

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

FAT and ext4 expect that the passed in block device descriptor not be
NULL. This causes problems on sandbox, where get_device_and_partition()
succeeds for the "host" device, yet passes back a NULL device descriptor.
Add special handling for this situation, so that the generic filesystem
commands operate as expected on sandbox.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
v3: New patch.
---
 fs/fs.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/fs/fs.c b/fs/fs.c
index 8fe2403a46ae..653e597c3cc9 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -64,6 +64,7 @@ static inline void fs_close_unsupported(void)
 
 struct fstype_info {
 	int fstype;
+	bool null_dev_desc_ok;
 	int (*probe)(block_dev_desc_t *fs_dev_desc,
 		     disk_partition_t *fs_partition);
 	int (*ls)(const char *dirname);
@@ -77,6 +78,7 @@ static struct fstype_info fstypes[] = {
 #ifdef CONFIG_FS_FAT
 	{
 		.fstype = FS_TYPE_FAT,
+		.null_dev_desc_ok = false,
 		.probe = fat_set_blk_dev,
 		.close = fat_close,
 		.ls = file_fat_ls,
@@ -88,6 +90,7 @@ static struct fstype_info fstypes[] = {
 #ifdef CONFIG_FS_EXT4
 	{
 		.fstype = FS_TYPE_EXT,
+		.null_dev_desc_ok = false,
 		.probe = ext4fs_probe,
 		.close = ext4fs_close,
 		.ls = ext4fs_ls,
@@ -99,6 +102,7 @@ static struct fstype_info fstypes[] = {
 #ifdef CONFIG_SANDBOX
 	{
 		.fstype = FS_TYPE_SANDBOX,
+		.null_dev_desc_ok = true,
 		.probe = sandbox_fs_set_blk_dev,
 		.close = sandbox_fs_close,
 		.ls = sandbox_fs_ls,
@@ -109,6 +113,7 @@ static struct fstype_info fstypes[] = {
 #endif
 	{
 		.fstype = FS_TYPE_ANY,
+		.null_dev_desc_ok = true,
 		.probe = fs_probe_unsupported,
 		.close = fs_close_unsupported,
 		.ls = fs_ls_unsupported,
@@ -162,6 +167,9 @@ int fs_set_blk_dev(const char *ifname, const char *dev_part_str, int fstype)
 				fstype != info->fstype)
 			continue;
 
+		if (!fs_dev_desc && !info->null_dev_desc_ok)
+			continue;
+
 		if (!info->probe(fs_dev_desc, &fs_partition)) {
 			fs_type = info->fstype;
 			return 0;
-- 
1.8.1.5

[U-Boot] [PATCH V3 04/12] cmd_test: use table lookup for parsing

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

do_test() currently uses strcmp() twice to determine which operator is
present; once to determine how many arguments the operator needs, then
a second time to actually decode the operator and implement it.

Rewrite the code so that a table lookup is used to translate the operator
string to an integer, and use a more efficient switch statement to decode
and execute the operator.

This approach also acts as enablement for the following patches.

This patch should introduce no behavioural change.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
v3: New patch.
---
 common/cmd_test.c | 177 +++++++++++++++++++++++++++++++++---------------------
 1 file changed, 110 insertions(+), 67 deletions(-)

diff --git a/common/cmd_test.c b/common/cmd_test.c
index bacc3684069b..69b1b4cee69a 100644
--- a/common/cmd_test.c
+++ b/common/cmd_test.c
@@ -17,10 +17,48 @@
 #include <common.h>
 #include <command.h>
 
+#define OP_INVALID	0
+#define OP_OR		2
+#define OP_AND		3
+#define OP_STR_EMPTY	4
+#define OP_STR_NEMPTY	5
+#define OP_STR_EQ	6
+#define OP_STR_NEQ	7
+#define OP_STR_LT	8
+#define OP_STR_GT	9
+#define OP_INT_EQ	10
+#define OP_INT_NEQ	11
+#define OP_INT_LT	12
+#define OP_INT_LE	13
+#define OP_INT_GT	14
+#define OP_INT_GE	15
+
+const struct {
+	int arg;
+	const char *str;
+	int op;
+	int adv;
+} op_adv[] = {
+	{0, "-o", OP_OR, 1},
+	{0, "-a", OP_AND, 1},
+	{0, "-z", OP_STR_EMPTY, 2},
+	{0, "-n", OP_STR_NEMPTY, 2},
+	{1, "=", OP_STR_EQ, 3},
+	{1, "!=", OP_STR_NEQ, 3},
+	{1, "<", OP_STR_LT, 3},
+	{1, ">", OP_STR_GT, 3},
+	{1, "-eq", OP_INT_EQ, 3},
+	{1, "-ne", OP_INT_NEQ, 3},
+	{1, "-lt", OP_INT_LT, 3},
+	{1, "-le", OP_INT_LE, 3},
+	{1, "-gt", OP_INT_GT, 3},
+	{1, "-ge", OP_INT_GE, 3},
+};
+
 static int do_test(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
 	char * const *ap;
-	int left, adv, expr, last_expr, neg, last_cmp;
+	int i, op, left, adv, expr, last_expr, neg, last_cmp;
 
 	/* args? */
 	if (argc < 3)
@@ -45,83 +83,88 @@ static int do_test(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		neg = 0;
 
 	expr = -1;
-	last_cmp = -1;
+	last_cmp = OP_INVALID;
 	last_expr = -1;
 	while (left > 0) {
-
-		if (strcmp(ap[0], "-o") == 0 || strcmp(ap[0], "-a") == 0)
-			adv = 1;
-		else if (strcmp(ap[0], "-z") == 0 || strcmp(ap[0], "-n") == 0)
-			adv = 2;
-		else
-			adv = 3;
-
-		if (left < adv) {
+		for (i = 0; i < ARRAY_SIZE(op_adv); i++) {
+			if (left <= op_adv[i].arg)
+				continue;
+			if (!strcmp(ap[op_adv[i].arg], op_adv[i].str)) {
+				op = op_adv[i].op;
+				adv = op_adv[i].adv;
+				break;
+			}
+		}
+		if (i == ARRAY_SIZE(op_adv)) {
 			expr = 1;
 			break;
 		}
-
-		if (adv == 1) {
-			if (strcmp(ap[0], "-o") == 0) {
-				last_expr = expr;
-				last_cmp = 0;
-			} else if (strcmp(ap[0], "-a") == 0) {
-				last_expr = expr;
-				last_cmp = 1;
-			} else {
-				expr = 1;
-				break;
-			}
+		if (left < adv) {
+			expr = 1;
+			break;
 		}
 
-		if (adv == 2) {
-			if (strcmp(ap[0], "-z") == 0)
-				expr = strlen(ap[1]) == 0 ? 1 : 0;
-			else if (strcmp(ap[0], "-n") == 0)
-				expr = strlen(ap[1]) == 0 ? 0 : 1;
-			else {
-				expr = 1;
-				break;
-			}
-
-			if (last_cmp == 0)
-				expr = last_expr || expr;
-			else if (last_cmp == 1)
-				expr = last_expr && expr;
-			last_cmp = -1;
+		switch (op) {
+		case OP_STR_EMPTY:
+			expr = strlen(ap[1]) == 0 ? 1 : 0;
+			break;
+		case OP_STR_NEMPTY:
+			expr = strlen(ap[1]) == 0 ? 0 : 1;
+			break;
+		case OP_STR_EQ:
+			expr = strcmp(ap[0], ap[2]) == 0;
+			break;
+		case OP_STR_NEQ:
+			expr = strcmp(ap[0], ap[2]) != 0;
+			break;
+		case OP_STR_LT:
+			expr = strcmp(ap[0], ap[2]) < 0;
+			break;
+		case OP_STR_GT:
+			expr = strcmp(ap[0], ap[2]) > 0;
+			break;
+		case OP_INT_EQ:
+			expr = simple_strtol(ap[0], NULL, 10) ==
+					simple_strtol(ap[2], NULL, 10);
+			break;
+		case OP_INT_NEQ:
+			expr = simple_strtol(ap[0], NULL, 10) !=
+					simple_strtol(ap[2], NULL, 10);
+			break;
+		case OP_INT_LT:
+			expr = simple_strtol(ap[0], NULL, 10) <
+					simple_strtol(ap[2], NULL, 10);
+			break;
+		case OP_INT_LE:
+			expr = simple_strtol(ap[0], NULL, 10) <=
+					simple_strtol(ap[2], NULL, 10);
+			break;
+		case OP_INT_GT:
+			expr = simple_strtol(ap[0], NULL, 10) >
+					simple_strtol(ap[2], NULL, 10);
+			break;
+		case OP_INT_GE:
+			expr = simple_strtol(ap[0], NULL, 10) >=
+					simple_strtol(ap[2], NULL, 10);
+			break;
 		}
 
-		if (adv == 3) {
-			if (strcmp(ap[1], "=") == 0)
-				expr = strcmp(ap[0], ap[2]) == 0;
-			else if (strcmp(ap[1], "!=") == 0)
-				expr = strcmp(ap[0], ap[2]) != 0;
-			else if (strcmp(ap[1], ">") == 0)
-				expr = strcmp(ap[0], ap[2]) > 0;
-			else if (strcmp(ap[1], "<") == 0)
-				expr = strcmp(ap[0], ap[2]) < 0;
-			else if (strcmp(ap[1], "-eq") == 0)
-				expr = simple_strtol(ap[0], NULL, 10) == simple_strtol(ap[2], NULL, 10);
-			else if (strcmp(ap[1], "-ne") == 0)
-				expr = simple_strtol(ap[0], NULL, 10) != simple_strtol(ap[2], NULL, 10);
-			else if (strcmp(ap[1], "-lt") == 0)
-				expr = simple_strtol(ap[0], NULL, 10) < simple_strtol(ap[2], NULL, 10);
-			else if (strcmp(ap[1], "-le") == 0)
-				expr = simple_strtol(ap[0], NULL, 10) <= simple_strtol(ap[2], NULL, 10);
-			else if (strcmp(ap[1], "-gt") == 0)
-				expr = simple_strtol(ap[0], NULL, 10) > simple_strtol(ap[2], NULL, 10);
-			else if (strcmp(ap[1], "-ge") == 0)
-				expr = simple_strtol(ap[0], NULL, 10) >= simple_strtol(ap[2], NULL, 10);
-			else {
-				expr = 1;
-				break;
-			}
-
-			if (last_cmp == 0)
+		switch (op) {
+		case OP_OR:
+			last_expr = expr;
+			last_cmp = OP_OR;
+			break;
+		case OP_AND:
+			last_expr = expr;
+			last_cmp = OP_AND;
+			break;
+		default:
+			if (last_cmp == OP_OR)
 				expr = last_expr || expr;
-			else if (last_cmp == 1)
+			else if (last_cmp == OP_AND)
 				expr = last_expr && expr;
-			last_cmp = -1;
+			last_cmp = OP_INVALID;
+			break;
 		}
 
 		ap += adv; left -= adv;
-- 
1.8.1.5

[U-Boot] [PATCH V3 05/12] cmd_test: check for binary operators before unary

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

This better mirrors the behaviour of bash, for example:

$ if test -z = -z; then echo yes; else echo no; fi
yes

This is parsed as a string comparison of "-z" and "-z", since the check
for the binary "=" operator occurs first. Without this change, the
command would be parsed as a -z test of "-", followed by a syntax error;
a trailing -z without and operand.

This is a behavioural change, but I believe any commands affected were
previously invalid or bizarely formed.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
v3: New patch.
---
 common/cmd_test.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/common/cmd_test.c b/common/cmd_test.c
index 69b1b4cee69a..e65dd531877e 100644
--- a/common/cmd_test.c
+++ b/common/cmd_test.c
@@ -39,10 +39,6 @@ const struct {
 	int op;
 	int adv;
 } op_adv[] = {
-	{0, "-o", OP_OR, 1},
-	{0, "-a", OP_AND, 1},
-	{0, "-z", OP_STR_EMPTY, 2},
-	{0, "-n", OP_STR_NEMPTY, 2},
 	{1, "=", OP_STR_EQ, 3},
 	{1, "!=", OP_STR_NEQ, 3},
 	{1, "<", OP_STR_LT, 3},
@@ -53,6 +49,10 @@ const struct {
 	{1, "-le", OP_INT_LE, 3},
 	{1, "-gt", OP_INT_GT, 3},
 	{1, "-ge", OP_INT_GE, 3},
+	{0, "-o", OP_OR, 1},
+	{0, "-a", OP_AND, 1},
+	{0, "-z", OP_STR_EMPTY, 2},
+	{0, "-n", OP_STR_NEMPTY, 2},
 };
 
 static int do_test(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
-- 
1.8.1.5

[U-Boot] [PATCH V3 06/12] cmd_test: implement ! on sub-expressions

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

Currently, ! can only be parsed as the first operator in an expression.
This prevents the following from working:

$ if test ! ! 1 -eq 1; then echo yes; else echo no; fi
yes
$ if test ! 1 -eq 2 -a ! 3 -eq 4; then echo yes; else echo no; fi
yes

Fix this by parsing ! like any other operator, and and handling it
similarly to -a and -o.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
v3: New patch.
---
 common/cmd_test.c | 43 ++++++++++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/common/cmd_test.c b/common/cmd_test.c
index e65dd531877e..b927d09eb3e0 100644
--- a/common/cmd_test.c
+++ b/common/cmd_test.c
@@ -18,6 +18,7 @@
 #include <command.h>
 
 #define OP_INVALID	0
+#define OP_NOT		1
 #define OP_OR		2
 #define OP_AND		3
 #define OP_STR_EMPTY	4
@@ -49,6 +50,7 @@ const struct {
 	{1, "-le", OP_INT_LE, 3},
 	{1, "-gt", OP_INT_GT, 3},
 	{1, "-ge", OP_INT_GE, 3},
+	{0, "!", OP_NOT, 1},
 	{0, "-o", OP_OR, 1},
 	{0, "-a", OP_AND, 1},
 	{0, "-z", OP_STR_EMPTY, 2},
@@ -58,7 +60,7 @@ const struct {
 static int do_test(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
 	char * const *ap;
-	int i, op, left, adv, expr, last_expr, neg, last_cmp;
+	int i, op, left, adv, expr, last_expr, last_unop, last_binop;
 
 	/* args? */
 	if (argc < 3)
@@ -73,17 +75,11 @@ static int do_test(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	}
 #endif
 
-	last_expr = 0;
-	left = argc - 1; ap = argv + 1;
-	if (left > 0 && strcmp(ap[0], "!") == 0) {
-		neg = 1;
-		ap++;
-		left--;
-	} else
-		neg = 0;
-
+	left = argc - 1;
+	ap = argv + 1;
 	expr = -1;
-	last_cmp = OP_INVALID;
+	last_unop = OP_INVALID;
+	last_binop = OP_INVALID;
 	last_expr = -1;
 	while (left > 0) {
 		for (i = 0; i < ARRAY_SIZE(op_adv); i++) {
@@ -152,27 +148,36 @@ static int do_test(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		switch (op) {
 		case OP_OR:
 			last_expr = expr;
-			last_cmp = OP_OR;
+			last_binop = OP_OR;
 			break;
 		case OP_AND:
 			last_expr = expr;
-			last_cmp = OP_AND;
+			last_binop = OP_AND;
+			break;
+		case OP_NOT:
+			if (last_unop == OP_NOT)
+				last_unop = OP_INVALID;
+			else
+				last_unop = OP_NOT;
 			break;
 		default:
-			if (last_cmp == OP_OR)
+			if (last_unop == OP_NOT) {
+				expr = !expr;
+				last_unop = OP_INVALID;
+			}
+
+			if (last_binop == OP_OR)
 				expr = last_expr || expr;
-			else if (last_cmp == OP_AND)
+			else if (last_binop == OP_AND)
 				expr = last_expr && expr;
-			last_cmp = OP_INVALID;
+			last_binop = OP_INVALID;
+
 			break;
 		}
 
 		ap += adv; left -= adv;
 	}
 
-	if (neg)
-		expr = !expr;
-
 	expr = !expr;
 
 	debug (": returns %d\n", expr);
-- 
1.8.1.5

[U-Boot] [PATCH V3 07/12] cmd_test: evaluate to false without any arguments

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

This emulates bash:
$ if test; then echo yes; else echo no; fi
no

Currently, the code sets expr = -1 in this case, which gets mapped to
0 (true) at the end of do_test() by the logical -> shell exit code
conversion.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
v3: New patch.
---
 common/cmd_test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/cmd_test.c b/common/cmd_test.c
index b927d09eb3e0..4c2f967c6dc0 100644
--- a/common/cmd_test.c
+++ b/common/cmd_test.c
@@ -77,7 +77,7 @@ static int do_test(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 
 	left = argc - 1;
 	ap = argv + 1;
-	expr = -1;
+	expr = 0;
 	last_unop = OP_INVALID;
 	last_binop = OP_INVALID;
 	last_expr = -1;
-- 
1.8.1.5

[U-Boot] [PATCH V3 08/12] cmd_test: implement -e test for file existence

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

This is much like a regular shell's -e operator, except that it takes
multiple arguments to specify the device type and  device/partition ID
in addition to the usual filename:

if test -e mmc 0:1 /boot/boot.scr; then echo yes; else echo no; fi

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
v3: New patch.
---
 common/cmd_test.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/common/cmd_test.c b/common/cmd_test.c
index 4c2f967c6dc0..c93fe7823100 100644
--- a/common/cmd_test.c
+++ b/common/cmd_test.c
@@ -16,6 +16,7 @@
 
 #include <common.h>
 #include <command.h>
+#include <fs.h>
 
 #define OP_INVALID	0
 #define OP_NOT		1
@@ -33,6 +34,7 @@
 #define OP_INT_LE	13
 #define OP_INT_GT	14
 #define OP_INT_GE	15
+#define OP_FILE_EXISTS	16
 
 const struct {
 	int arg;
@@ -55,6 +57,7 @@ const struct {
 	{0, "-a", OP_AND, 1},
 	{0, "-z", OP_STR_EMPTY, 2},
 	{0, "-n", OP_STR_NEMPTY, 2},
+	{0, "-e", OP_FILE_EXISTS, 4},
 };
 
 static int do_test(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
@@ -143,6 +146,9 @@ static int do_test(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 			expr = simple_strtol(ap[0], NULL, 10) >=
 					simple_strtol(ap[2], NULL, 10);
 			break;
+		case OP_FILE_EXISTS:
+			expr = file_exists(ap[1], ap[2], ap[3], FS_TYPE_ANY);
+			break;
 		}
 
 		switch (op) {
-- 
1.8.1.5

[U-Boot] [PATCH V3 09/12] sandbox: implement exists() function

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

This hooks into the generic "file exists" support added in an earlier
patch, and provides an implementation for the sandbox test environment.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
Acked-by: Simon Glass <sjg@chromium.org>
---
v3:
* Remove "sb exists" command; it's part of "test" now.
* Invert return value of exists(), per change to previous patch.
v2: No change.
---
 fs/fs.c                | 2 +-
 fs/sandbox/sandboxfs.c | 8 ++++++++
 include/sandboxfs.h    | 1 +
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/fs/fs.c b/fs/fs.c
index 653e597c3cc9..81ff1959fae5 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -106,7 +106,7 @@ static struct fstype_info fstypes[] = {
 		.probe = sandbox_fs_set_blk_dev,
 		.close = sandbox_fs_close,
 		.ls = sandbox_fs_ls,
-		.exists = fs_exists_unsupported,
+		.exists = sandbox_fs_exists,
 		.read = fs_read_sandbox,
 		.write = fs_write_sandbox,
 	},
diff --git a/fs/sandbox/sandboxfs.c b/fs/sandbox/sandboxfs.c
index dd028da8e32b..85079788c990 100644
--- a/fs/sandbox/sandboxfs.c
+++ b/fs/sandbox/sandboxfs.c
@@ -72,6 +72,14 @@ int sandbox_fs_ls(const char *dirname)
 	return 0;
 }
 
+int sandbox_fs_exists(const char *filename)
+{
+	ssize_t sz;
+
+	sz = os_get_filesize(filename);
+	return sz >= 0;
+}
+
 void sandbox_fs_close(void)
 {
 }
diff --git a/include/sandboxfs.h b/include/sandboxfs.h
index 8ea8cb7e2e62..a51ad13044e1 100644
--- a/include/sandboxfs.h
+++ b/include/sandboxfs.h
@@ -25,6 +25,7 @@ long sandbox_fs_read_at(const char *filename, unsigned long pos,
 
 void sandbox_fs_close(void);
 int sandbox_fs_ls(const char *dirname);
+int sandbox_fs_exists(const char *filename);
 int fs_read_sandbox(const char *filename, void *buf, int offset, int len);
 int fs_write_sandbox(const char *filename, void *buf, int offset, int len);
 
-- 
1.8.1.5

[U-Boot] [PATCH V3 10/12] sandbox: enable CONFIG_CMD_FS_GENERIC

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

Since the generic ls command no longer segfaults sandbox, enabling it.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
v3: New patch.
---
 include/configs/sandbox.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h
index a6d55822b82e..e77d06bcd3ed 100644
--- a/include/configs/sandbox.h
+++ b/include/configs/sandbox.h
@@ -42,6 +42,7 @@
 #define CONFIG_CMD_PART
 #define CONFIG_DOS_PARTITION
 #define CONFIG_HOST_MAX_DEVICES 4
+#define CONFIG_CMD_FS_GENERIC
 
 #define CONFIG_SYS_VSNPRINTF
 
-- 
1.8.1.5

[U-Boot] [PATCH V3 11/12] ext4: implement exists() for ext4fs

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

This hooks into the generic "file exists" support added in an earlier
patch, and provides an implementation for the ext4 filesystem.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
Acked-by: Simon Glass <sjg@chromium.org>
---
v3: Invert return value of exists(), per change to previous patch.
v2: No change.
---
 fs/ext4/ext4fs.c | 8 ++++++++
 fs/fs.c          | 2 +-
 include/ext4fs.h | 1 +
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/ext4fs.c b/fs/ext4/ext4fs.c
index 735b2564175b..417ce7b63bf0 100644
--- a/fs/ext4/ext4fs.c
+++ b/fs/ext4/ext4fs.c
@@ -174,6 +174,14 @@ int ext4fs_ls(const char *dirname)
 	return 0;
 }
 
+int ext4fs_exists(const char *filename)
+{
+	int file_len;
+
+	file_len = ext4fs_open(filename);
+	return file_len >= 0;
+}
+
 int ext4fs_read(char *buf, unsigned len)
 {
 	if (ext4fs_root == NULL || ext4fs_file == NULL)
diff --git a/fs/fs.c b/fs/fs.c
index 81ff1959fae5..e1e4fcc236d6 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -94,7 +94,7 @@ static struct fstype_info fstypes[] = {
 		.probe = ext4fs_probe,
 		.close = ext4fs_close,
 		.ls = ext4fs_ls,
-		.exists = fs_exists_unsupported,
+		.exists = ext4fs_exists,
 		.read = ext4_read_file,
 		.write = fs_write_unsupported,
 	},
diff --git a/include/ext4fs.h b/include/ext4fs.h
index 242938039662..aacb147de24b 100644
--- a/include/ext4fs.h
+++ b/include/ext4fs.h
@@ -134,6 +134,7 @@ int ext4fs_read(char *buf, unsigned len);
 int ext4fs_mount(unsigned part_length);
 void ext4fs_close(void);
 int ext4fs_ls(const char *dirname);
+int ext4fs_exists(const char *filename);
 void ext4fs_free_node(struct ext2fs_node *node, struct ext2fs_node *currroot);
 int ext4fs_devread(lbaint_t sector, int byte_offset, int byte_len, char *buf);
 void ext4fs_set_blk_dev(block_dev_desc_t *rbdd, disk_partition_t *info);
-- 
1.8.1.5

[U-Boot] [PATCH V3 12/12] fat: implement exists() for FAT fs

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

This hooks into the generic "file exists" support added in an earlier
patch, and provides an implementation for the FAT filesystem.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
v3:
* s/ext/fat/ in the commit description too:-(
* Invert return value of exists(), per change to previous patch.
v2: s/ext/fat/ in the commit subject.
---
 fs/fat/fat.c  | 18 ++++++++++++++----
 fs/fs.c       |  2 +-
 include/fat.h |  1 +
 3 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index b41d62e3c386..54f42eae0d05 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -808,7 +808,7 @@ __u8 do_fat_read_at_block[MAX_CLUSTSIZE]
 
 long
 do_fat_read_at(const char *filename, unsigned long pos, void *buffer,
-	       unsigned long maxsize, int dols)
+	       unsigned long maxsize, int dols, int dogetsize)
 {
 	char fnamecopy[2048];
 	boot_sector bs;
@@ -1152,7 +1152,10 @@ rootdir_done:
 			subname = nextname;
 	}
 
-	ret = get_contents(mydata, dentptr, pos, buffer, maxsize);
+	if (dogetsize)
+		ret = FAT2CPU32(dentptr->size);
+	else
+		ret = get_contents(mydata, dentptr, pos, buffer, maxsize);
 	debug("Size: %d, got: %ld\n", FAT2CPU32(dentptr->size), ret);
 
 exit:
@@ -1163,7 +1166,7 @@ exit:
 long
 do_fat_read(const char *filename, void *buffer, unsigned long maxsize, int dols)
 {
-	return do_fat_read_at(filename, 0, buffer, maxsize, dols);
+	return do_fat_read_at(filename, 0, buffer, maxsize, dols, 0);
 }
 
 int file_fat_detectfs(void)
@@ -1233,11 +1236,18 @@ int file_fat_ls(const char *dir)
 	return do_fat_read(dir, NULL, 0, LS_YES);
 }
 
+int fat_exists(const char *filename)
+{
+	int sz;
+	sz = do_fat_read_at(filename, 0, NULL, 0, LS_NO, 1);
+	return sz >= 0;
+}
+
 long file_fat_read_at(const char *filename, unsigned long pos, void *buffer,
 		      unsigned long maxsize)
 {
 	printf("reading %s\n", filename);
-	return do_fat_read_at(filename, pos, buffer, maxsize, LS_NO);
+	return do_fat_read_at(filename, pos, buffer, maxsize, LS_NO, 0);
 }
 
 long file_fat_read(const char *filename, void *buffer, unsigned long maxsize)
diff --git a/fs/fs.c b/fs/fs.c
index e1e4fcc236d6..0b919f286038 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -82,7 +82,7 @@ static struct fstype_info fstypes[] = {
 		.probe = fat_set_blk_dev,
 		.close = fat_close,
 		.ls = file_fat_ls,
-		.exists = fs_exists_unsupported,
+		.exists = fat_exists,
 		.read = fat_read_file,
 		.write = fs_write_unsupported,
 	},
diff --git a/include/fat.h b/include/fat.h
index 2c951e7d79c6..c8eb7ccd2904 100644
--- a/include/fat.h
+++ b/include/fat.h
@@ -188,6 +188,7 @@ file_read_func		file_fat_read;
 int file_cd(const char *path);
 int file_fat_detectfs(void);
 int file_fat_ls(const char *dir);
+int fat_exists(const char *filename);
 long file_fat_read_at(const char *filename, unsigned long pos, void *buffer,
 		      unsigned long maxsize);
 long file_fat_read(const char *filename, void *buffer, unsigned long maxsize);
-- 
1.8.1.5

[U-Boot] [PATCH V3 02/12] fs: implement infra-structure for an 'exists' function

[Simon Glass]

On 27 January 2014 13:49, Stephen Warren <swarren@wwwdotorg.org> wrote:
> From: Stephen Warren <swarren@nvidia.com>
>
> This could be used in scripts such as:
>
> if test -e mmc 0:1 /boot/boot.scr; then
>     load mmc 0:1 ${scriptaddr} /boot/boot.scr
>     source ${scriptaddr}
> fi
>
> rather than:
>
> if load mmc 0:1 ${scriptaddr} /boot/boot.scr; then
>     source ${scriptaddr}
> fi
>
> This prevents errors being printed by attempts to load non-existent
> files, which can be important when checking for a large set of files,
> such as /boot/boot.scr.uimg, /boot/boot.scr, /boot/extlinux.conf,
> /boot.scr.uimg, /boot.scr, /extlinux.conf.
>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> Acked-by: Simon Glass <sjg@chromium.org>

nit in case you re-issue the series, infrastructure is one word.

Regards,
Simon

[U-Boot] [PATCH V3 03/12] fs: don't pass NULL dev_desc to most filesystems

[Simon Glass]

Hi Stephen,

On 27 January 2014 13:49, Stephen Warren <swarren@wwwdotorg.org> wrote:
> From: Stephen Warren <swarren@nvidia.com>
>
> FAT and ext4 expect that the passed in block device descriptor not be
> NULL. This causes problems on sandbox, where get_device_and_partition()
> succeeds for the "host" device, yet passes back a NULL device descriptor.
> Add special handling for this situation, so that the generic filesystem
> commands operate as expected on sandbox.
>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
> v3: New patch.
> ---
>  fs/fs.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/fs/fs.c b/fs/fs.c
> index 8fe2403a46ae..653e597c3cc9 100644
> --- a/fs/fs.c
> +++ b/fs/fs.c
> @@ -64,6 +64,7 @@ static inline void fs_close_unsupported(void)
>
>  struct fstype_info {
>         int fstype;
> +       bool null_dev_desc_ok;

I suggest adding a comment for this field.

>         int (*probe)(block_dev_desc_t *fs_dev_desc,
>                      disk_partition_t *fs_partition);
>         int (*ls)(const char *dirname);
> @@ -77,6 +78,7 @@ static struct fstype_info fstypes[] = {
>  #ifdef CONFIG_FS_FAT
>         {
>                 .fstype = FS_TYPE_FAT,
> +               .null_dev_desc_ok = false,
>                 .probe = fat_set_blk_dev,
>                 .close = fat_close,
>                 .ls = file_fat_ls,
> @@ -88,6 +90,7 @@ static struct fstype_info fstypes[] = {
>  #ifdef CONFIG_FS_EXT4
>         {
>                 .fstype = FS_TYPE_EXT,
> +               .null_dev_desc_ok = false,
>                 .probe = ext4fs_probe,
>                 .close = ext4fs_close,
>                 .ls = ext4fs_ls,
> @@ -99,6 +102,7 @@ static struct fstype_info fstypes[] = {
>  #ifdef CONFIG_SANDBOX
>         {
>                 .fstype = FS_TYPE_SANDBOX,
> +               .null_dev_desc_ok = true,
>                 .probe = sandbox_fs_set_blk_dev,
>                 .close = sandbox_fs_close,
>                 .ls = sandbox_fs_ls,
> @@ -109,6 +113,7 @@ static struct fstype_info fstypes[] = {
>  #endif
>         {
>                 .fstype = FS_TYPE_ANY,
> +               .null_dev_desc_ok = true,
>                 .probe = fs_probe_unsupported,
>                 .close = fs_close_unsupported,
>                 .ls = fs_ls_unsupported,
> @@ -162,6 +167,9 @@ int fs_set_blk_dev(const char *ifname, const char *dev_part_str, int fstype)
>                                 fstype != info->fstype)
>                         continue;
>
> +               if (!fs_dev_desc && !info->null_dev_desc_ok)
> +                       continue;
> +
>                 if (!info->probe(fs_dev_desc, &fs_partition)) {
>                         fs_type = info->fstype;
>                         return 0;
> --
> 1.8.1.5
>

Regards,
Simon

[U-Boot] [PATCH V3 04/12] cmd_test: use table lookup for parsing

[Simon Glass]

Hi Stephen,

On 27 January 2014 13:49, Stephen Warren <swarren@wwwdotorg.org> wrote:
> From: Stephen Warren <swarren@nvidia.com>
>
> do_test() currently uses strcmp() twice to determine which operator is
> present; once to determine how many arguments the operator needs, then
> a second time to actually decode the operator and implement it.
>
> Rewrite the code so that a table lookup is used to translate the operator
> string to an integer, and use a more efficient switch statement to decode
> and execute the operator.
>
> This approach also acts as enablement for the following patches.
>
> This patch should introduce no behavioural change.
>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
> v3: New patch.

Wow that's some interesting code...it took me a while to understand
both the old and the new code. It looks correct to me but I wonder if
it is deserving of some tests? Something like test/command_ut.c might
show a simple way to run some tests.

Regards,
Simon

[U-Boot] [PATCH V3 08/12] cmd_test: implement -e test for file existence

[Simon Glass]

Hi Stephen,

On 27 January 2014 13:49, Stephen Warren <swarren@wwwdotorg.org> wrote:
> From: Stephen Warren <swarren@nvidia.com>
>
> This is much like a regular shell's -e operator, except that it takes
> multiple arguments to specify the device type and  device/partition ID
> in addition to the usual filename:
>
> if test -e mmc 0:1 /boot/boot.scr; then echo yes; else echo no; fi
>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>

That's a really clever implementation.

Regards,
Simon

[U-Boot] [PATCH V3 10/12] sandbox: enable CONFIG_CMD_FS_GENERIC

[Simon Glass]

On 27 January 2014 13:50, Stephen Warren <swarren@wwwdotorg.org> wrote:
> From: Stephen Warren <swarren@nvidia.com>
>
> Since the generic ls command no longer segfaults sandbox, enabling it.
>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>

Acked-by: Simon Glass <sjg@chromium.org>

> ---
> v3: New patch.
> ---
>  include/configs/sandbox.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h
> index a6d55822b82e..e77d06bcd3ed 100644
> --- a/include/configs/sandbox.h
> +++ b/include/configs/sandbox.h
> @@ -42,6 +42,7 @@
>  #define CONFIG_CMD_PART
>  #define CONFIG_DOS_PARTITION
>  #define CONFIG_HOST_MAX_DEVICES 4
> +#define CONFIG_CMD_FS_GENERIC
>
>  #define CONFIG_SYS_VSNPRINTF
>
> --
> 1.8.1.5
>

[U-Boot] [PATCH V3 12/12] fat: implement exists() for FAT fs

[Simon Glass]

On 27 January 2014 13:50, Stephen Warren <swarren@wwwdotorg.org> wrote:
> From: Stephen Warren <swarren@nvidia.com>
>
> This hooks into the generic "file exists" support added in an earlier
> patch, and provides an implementation for the FAT filesystem.
>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>

Acked-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH V3 04/12] cmd_test: use table lookup for parsing

[Stephen Warren]

On 01/31/2014 05:03 PM, Simon Glass wrote:
> Hi Stephen,
> 
> On 27 January 2014 13:49, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> do_test() currently uses strcmp() twice to determine which operator is
>> present; once to determine how many arguments the operator needs, then
>> a second time to actually decode the operator and implement it.
>>
>> Rewrite the code so that a table lookup is used to translate the operator
>> string to an integer, and use a more efficient switch statement to decode
>> and execute the operator.
>>
>> This approach also acts as enablement for the following patches.
>>
>> This patch should introduce no behavioural change.
>>
>> Signed-off-by: Stephen Warren <swarren@nvidia.com>
>> ---
>> v3: New patch.
> 
> Wow that's some interesting code...it took me a while to understand
> both the old and the new code. It looks correct to me but I wonder if
> it is deserving of some tests? Something like test/command_ut.c might
> show a simple way to run some tests.

OK, I'll send V4 of this series with your minor issues addressed, and
I'll send a separate follow-on series which adds the unit tests, just so
it doesn't delay or cause revisions to the main series.

[U-Boot] [PATCH V3 04/12] cmd_test: use table lookup for parsing

[Simon Glass]

Hi Stephen,

On 3 February 2014 13:19, Stephen Warren <swarren@wwwdotorg.org> wrote:
>
> On 01/31/2014 05:03 PM, Simon Glass wrote:
> > Hi Stephen,
> >
> > On 27 January 2014 13:49, Stephen Warren <swarren@wwwdotorg.org> wrote:
> >> From: Stephen Warren <swarren@nvidia.com>
> >>
> >> do_test() currently uses strcmp() twice to determine which operator is
> >> present; once to determine how many arguments the operator needs, then
> >> a second time to actually decode the operator and implement it.
> >>
> >> Rewrite the code so that a table lookup is used to translate the operator
> >> string to an integer, and use a more efficient switch statement to decode
> >> and execute the operator.
> >>
> >> This approach also acts as enablement for the following patches.
> >>
> >> This patch should introduce no behavioural change.
> >>
> >> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> >> ---
> >> v3: New patch.
> >
> > Wow that's some interesting code...it took me a while to understand
> > both the old and the new code. It looks correct to me but I wonder if
> > it is deserving of some tests? Something like test/command_ut.c might
> > show a simple way to run some tests.
>
> OK, I'll send V4 of this series with your minor issues addressed, and
> I'll send a separate follow-on series which adds the unit tests, just so
> it doesn't delay or cause revisions to the main series.


Sounds good, thanks.

Regards,
Simon