[U-Boot] [PATCH v3 1/4] Add run_command_list() to run a list of commands

[U-Boot] [PATCH v3 1/4] Add run_command_list() to run a list of commands

[Simon Glass]

This new function runs a list of commands separated by semicolon or newline.
We move this out of cmd_source so that it can be used by other code. The
PXE code also uses the new function.

Suggested-by: Michael Walle <michael@walle.cc>
Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v3:
- Added a few more comments on the need for malloc()
- Change PXE's printf() to a debug()
- Fix bug in built-in run_command()
- Fix commit message to mention ; and \n
- Move run_command_list() comment to header file

 common/cmd_pxe.c    |   20 ++----------
 common/cmd_source.c |   49 +----------------------------
 common/main.c       |   85 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/common.h    |   13 ++++++++
 4 files changed, 102 insertions(+), 65 deletions(-)

diff --git a/common/cmd_pxe.c b/common/cmd_pxe.c
index b3c1f67..6032a0f 100644
--- a/common/cmd_pxe.c
+++ b/common/cmd_pxe.c
@@ -515,33 +515,19 @@ static void label_print(void *data)
  */
 static int label_localboot(struct pxe_label *label)
 {
-	char *localcmd, *dupcmd;
-	int ret;
+	char *localcmd;
 
 	localcmd = from_env("localcmd");
 
 	if (!localcmd)
 		return -ENOENT;
 
-	/*
-	 * dup the command to avoid any issues with the version of it existing
-	 * in the environment changing during the execution of the command.
-	 */
-	dupcmd = strdup(localcmd);
-
-	if (!dupcmd)
-		return -ENOMEM;
-
 	if (label->append)
 		setenv("bootargs", label->append);
 
-	printf("running: %s\n", dupcmd);
-
-	ret = run_command(dupcmd, 0);
+	debug("running: %s\n", localcmd);
 
-	free(dupcmd);
-
-	return ret;
+	return run_command_list(localcmd, strlen(localcmd), 0);
 }
 
 /*
diff --git a/common/cmd_source.c b/common/cmd_source.c
index 32fff5c..c4cde98 100644
--- a/common/cmd_source.c
+++ b/common/cmd_source.c
@@ -39,9 +39,6 @@
 #if defined(CONFIG_8xx)
 #include <mpc8xx.h>
 #endif
-#ifdef CONFIG_SYS_HUSH_PARSER
-#include <hush.h>
-#endif
 
 int
 source (ulong addr, const char *fit_uname)
@@ -49,8 +46,6 @@ source (ulong addr, const char *fit_uname)
 	ulong		len;
 	image_header_t	*hdr;
 	ulong		*data;
-	char		*cmd;
-	int		rcode = 0;
 	int		verify;
 #if defined(CONFIG_FIT)
 	const void*	fit_hdr;
@@ -151,49 +146,7 @@ source (ulong addr, const char *fit_uname)
 	}
 
 	debug ("** Script length: %ld\n", len);
-
-	if ((cmd = malloc (len + 1)) == NULL) {
-		return 1;
-	}
-
-	/* make sure cmd is null terminated */
-	memmove (cmd, (char *)data, len);
-	*(cmd + len) = 0;
-
-#ifdef CONFIG_SYS_HUSH_PARSER /*?? */
-	rcode = parse_string_outer (cmd, FLAG_PARSE_SEMICOLON);
-#else
-	{
-		char *line = cmd;
-		char *next = cmd;
-
-		/*
-		 * break into individual lines,
-		 * and execute each line;
-		 * terminate on error.
-		 */
-		while (*next) {
-			if (*next == '\n') {
-				*next = '\0';
-				/* run only non-empty commands */
-				if (*line) {
-					debug ("** exec: \"%s\"\n",
-						line);
-					if (run_command(line, 0) < 0) {
-						rcode = 1;
-						break;
-					}
-				}
-				line = next + 1;
-			}
-			++next;
-		}
-		if (rcode == 0 && *line)
-			rcode = (run_command(line, 0) >= 0);
-	}
-#endif
-	free (cmd);
-	return rcode;
+	return run_command_list((char *)data, len, 0);
 }
 
 /**************************************************/
diff --git a/common/main.c b/common/main.c
index db181d3..8c5115d 100644
--- a/common/main.c
+++ b/common/main.c
@@ -30,6 +30,7 @@
 #include <common.h>
 #include <watchdog.h>
 #include <command.h>
+#include <malloc.h>
 #include <version.h>
 #ifdef CONFIG_MODEM_SUPPORT
 #include <malloc.h>		/* for free() prototype */
@@ -1373,6 +1374,90 @@ int run_command(const char *cmd, int flag)
 #endif
 }
 
+#ifndef CONFIG_SYS_HUSH_PARSER
+/**
+ * Execute a list of command separated by ; or \n using the built-in parser.
+ *
+ * This function cannot take a const char * for the command, since if it
+ * finds newlines in the string, it replaces them with \0.
+ *
+ * @param cmd	String containing list of commands
+ * @param flag	Execution flags (CMD_FLAG_...)
+ * @return 0 on success, or != 0 on error.
+ */
+static int builtin_run_command_list(char *cmd, int flag)
+{
+	char *line, *next;
+	int rcode = 0;
+
+	/*
+	 * Break into individual lines, and execute each line; terminate on
+	 * error.
+	 */
+	line = next = cmd;
+	while (*next) {
+		if (*next == '\n') {
+			*next = '\0';
+			/* run only non-empty commands */
+			if (*line) {
+				debug("** exec: \"%s\"\n", line);
+				if (builtin_run_command(line, 0) < 0) {
+					rcode = 1;
+					break;
+				}
+			}
+			line = next + 1;
+		}
+		++next;
+	}
+	if (rcode == 0 && *line)
+		rcode = (builtin_run_command(line, 0) >= 0);
+
+	return rcode;
+}
+#endif
+
+int run_command_list(const char *cmd, int len, int flag)
+{
+	int need_buff = 1;
+	char *buff = (char *)cmd;	/* cast away const */
+	int rcode = 0;
+
+	if (len == -1) {
+		len = strlen(cmd);
+#ifdef CONFIG_SYS_HUSH_PARSER
+		/* hush will never change our string */
+		need_buff = 0;
+#else
+		/* the built-in parser will change our string if it sees \n */
+		need_buff = strchr(cmd, '\n') != NULL;
+#endif
+	}
+	if (need_buff) {
+		buff = malloc(len + 1);
+		if (!buff)
+			return 1;
+		memcpy(buff, cmd, len);
+		buff[len] = '\0';
+	}
+#ifdef CONFIG_SYS_HUSH_PARSER
+	rcode = parse_string_outer(buff, FLAG_PARSE_SEMICOLON);
+#else
+	/*
+	 * This function will overwrite any \n it sees with a \0, which
+	 * is why it can't work with a const char *. Here we are making
+	 * using of internal knowledge of this function, to avoid always
+	 * doing a malloc() which is actually required only in a case that
+	 * is pretty rare.
+	 */
+	rcode = builtin_run_command_list(buff, flag);
+	if (need_buff)
+		free(buff);
+#endif
+
+	return rcode;
+}
+
 /****************************************************************************/
 
 #if defined(CONFIG_CMD_RUN)
diff --git a/include/common.h b/include/common.h
index 74d9704..c4ae451 100644
--- a/include/common.h
+++ b/include/common.h
@@ -268,6 +268,19 @@ int	print_buffer (ulong addr, void* data, uint width, uint count, uint linelen);
 /* common/main.c */
 void	main_loop	(void);
 int run_command(const char *cmd, int flag);
+
+/**
+ * Run a list of commands separated by ; or even \0
+ *
+ * Note that if 'len' is not -1, then the command does not need to be nul
+ * terminated, Memory will be allocated for the command in that case.
+ *
+ * @param cmd	List of commands to run, each separated bu semicolon
+ * @param len	Length of commands excluding terminator if known (-1 if not)
+ * @param flag	Execution flags (CMD_FLAG_...)
+ * @return 0 on success, or != 0 on error.
+ */
+int run_command_list(const char *cmd, int len, int flag);
 int	readline	(const char *const prompt);
 int	readline_into_buffer(const char *const prompt, char *buffer,
 			int timeout);
-- 
1.7.7.3

[U-Boot] [PATCH v3 2/4] Allow newlines within command environment vars

[Simon Glass]

Any environment variable can hold commands to be executed by the 'run'
command. The environment variables preboot, bootcmd and menucmd have
special code for triggering execution in certain circumstances.

We adjust these calls to use run_command_list() instead of run_command().
This change permits these variables to have embedded newlines so that
they work the same as the 'source' command.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v2:
- Update commit message to be less confusing

Changes in v3:
- Remove unneeded len parameter in calls to run_command_list

 common/main.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/common/main.c b/common/main.c
index 8c5115d..578977b 100644
--- a/common/main.c
+++ b/common/main.c
@@ -334,7 +334,7 @@ void main_loop (void)
 		int prev = disable_ctrlc(1);	/* disable Control C checking */
 # endif
 
-		run_command(p, 0);
+		run_command_list(p, -1, 0);
 
 # ifdef CONFIG_AUTOBOOT_KEYED
 		disable_ctrlc(prev);	/* restore Control C checking */
@@ -382,7 +382,7 @@ void main_loop (void)
 		int prev = disable_ctrlc(1);	/* disable Control C checking */
 # endif
 
-		run_command(s, 0);
+		run_command_list(s, -1, 0);
 
 # ifdef CONFIG_AUTOBOOT_KEYED
 		disable_ctrlc(prev);	/* restore Control C checking */
@@ -393,7 +393,7 @@ void main_loop (void)
 	if (menukey == CONFIG_MENUKEY) {
 		s = getenv("menucmd");
 		if (s)
-			run_command(s, 0);
+			run_command_list(s, -1, 0);
 	}
 #endif /* CONFIG_MENUKEY */
 #endif /* CONFIG_BOOTDELAY */
-- 
1.7.7.3

[U-Boot] [PATCH v3 3/4] sandbox: Use the new run_command()

[Simon Glass]

Now that run_command() handles both parsers, clean up sandbox to use it.
This fixes a build error.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v3:
- Add new patch to clean up sandbox's run_command() usage

 arch/sandbox/cpu/start.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/arch/sandbox/cpu/start.c b/arch/sandbox/cpu/start.c
index 6c3e8eb..7603bf9 100644
--- a/arch/sandbox/cpu/start.c
+++ b/arch/sandbox/cpu/start.c
@@ -90,13 +90,7 @@ int sandbox_main_loop_init(void)
 
 	/* Execute command if required */
 	if (state->cmd) {
-		/* TODO: redo this when cmd tidy-up series lands */
-#ifdef CONFIG_SYS_HUSH_PARSER
 		run_command(state->cmd, 0);
-#else
-		parse_string_outer(state->cmd, FLAG_PARSE_SEMICOLON |
-				    FLAG_EXIT_FROM_LOOP);
-#endif
 		os_exit(state->exit_type);
 	}
 
-- 
1.7.7.3

[U-Boot] [PATCH v3 4/4] sandbox: Add basic test for command execution

[Simon Glass]

Since run_command() and run_command_list() are important and a little
confusing, add some basic tests to check that the behaviour is correct.

Note: I am not sure that this should be committed, nor where it should go
in the source tree. Comments welcome.

To run the unit tests use the ut_cmd command available in sandbox:

make sandbox_config
make
./u-boot -c ut_cmd

(To test both hush and built-in parsers, you need to manually change
CONFIG_SYS_HUSH_PARSER in include/configs/sandbox.h and build/run again)

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v3:
- Add some unit tests for run_command() and run_command_list()

 Makefile          |    1 +
 test/Makefile     |   45 ++++++++++++++++++++++++++++
 test/command_ut.c |   85 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 131 insertions(+), 0 deletions(-)
 create mode 100644 test/Makefile
 create mode 100644 test/command_ut.c

diff --git a/Makefile b/Makefile
index 4ddf8d6..21a0f69 100644
--- a/Makefile
+++ b/Makefile
@@ -301,6 +301,7 @@ LIBS += common/libcommon.o
 LIBS += lib/libfdt/libfdt.o
 LIBS += api/libapi.o
 LIBS += post/libpost.o
+LIBS += test/libtest.o
 
 ifneq ($(CONFIG_AM33XX)$(CONFIG_OMAP34XX)$(CONFIG_OMAP44XX)$(CONFIG_OMAP54XX),)
 LIBS += $(CPUDIR)/omap-common/libomap-common.o
diff --git a/test/Makefile b/test/Makefile
new file mode 100644
index 0000000..83594f3
--- /dev/null
+++ b/test/Makefile
@@ -0,0 +1,45 @@
+#
+# (C) Copyright 2012 The Chromium Authors
+#
+# 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 $(TOPDIR)/config.mk
+
+LIB	= $(obj)libtest.o
+
+COBJS-$(CONFIG_SANDBOX) += command_ut.o
+
+COBJS	:= $(sort $(COBJS-y))
+SRCS	:= $(COBJS:.o=.c)
+OBJS	:= $(addprefix $(obj),$(COBJS))
+
+all:	$(LIB) $(XOBJS)
+
+$(LIB): $(obj).depend $(OBJS)
+	$(call cmd_link_o_target, $(OBJS))
+
+#########################################################################
+
+# defines $(obj).depend target
+include $(SRCTREE)/rules.mk
+
+sinclude $(obj).depend
+
+#########################################################################
diff --git a/test/command_ut.c b/test/command_ut.c
new file mode 100644
index 0000000..dfddc22
--- /dev/null
+++ b/test/command_ut.c
@@ -0,0 +1,85 @@
+/*
+ * Copyright (c) 2012, The Chromium Authors
+ *
+ * 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
+ */
+
+#define DEBUG
+
+#include <common.h>
+
+static const char test_cmd[] = "setenv list 1\n setenv list ${list}2; "
+		"setenv list ${list}3\0"
+		"setenv list ${list}4";
+
+static int do_ut_cmd(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+	printf("%s: Testing commands\n", __func__);
+	run_command("env default -f", 0);
+
+	/* run a single command */
+	run_command("setenv single 1", 0);
+	assert(!strcmp("1", getenv("single")));
+
+	/* make sure that compound statements work */
+#ifdef CONFIG_SYS_HUSH_PARSER
+	run_command("if test -n ${single} ; then setenv check 1; fi", 0);
+	assert(!strcmp("1", getenv("check")));
+	run_command("setenv check", 0);
+#endif
+
+	/* commands separated by ; */
+	run_command_list("setenv list 1; setenv list ${list}1", -1, 0);
+	assert(!strcmp("11", getenv("list")));
+
+	/* commands separated by \n */
+	run_command_list("setenv list 1\n setenv list ${list}1", -1, 0);
+	assert(!strcmp("11", getenv("list")));
+
+	/* command followed by \n and nothing else */
+	run_command_list("setenv list 1${list}\n", -1, 0);
+	assert(!strcmp("111", getenv("list")));
+
+	/* three commands in a row */
+	run_command_list("setenv list 1\n setenv list ${list}2; "
+		"setenv list ${list}3", -1, 0);
+	assert(!strcmp("123", getenv("list")));
+
+	/* a command string with \0 in it. Stuff after \0 should be ignored */
+	run_command("setenv list", 0);
+	run_command_list(test_cmd, sizeof(test_cmd), 0);
+	assert(!strcmp("123", getenv("list")));
+
+	/*
+	 * a command list where we limit execution to only the first command
+	 * using the length parameter.
+	 */
+	run_command_list("setenv list 1\n setenv list ${list}2; "
+		"setenv list ${list}3", strlen("setenv list 1"), 0);
+	assert(!strcmp("1", getenv("list")));
+
+	printf("%s: Everything went swimmingly\n", __func__);
+	return 0;
+}
+
+U_BOOT_CMD(
+	ut_cmd,	5,	1,	do_ut_cmd,
+	"Very basic test of command parsers",
+	""
+);
-- 
1.7.7.3

[U-Boot] [PATCH v3 1/4] Add run_command_list() to run a list of commands

[Mike Frysinger]

On Saturday 31 March 2012 03:30:55 Simon Glass wrote:
> --- a/common/cmd_pxe.c
> +++ b/common/cmd_pxe.c
> 
> +	return run_command_list(localcmd, strlen(localcmd), 0);

should be -1 instead of strlen()

> +int run_command_list(const char *cmd, int len, int flag)
> +{
> +	int need_buff = 1;
> +	char *buff = (char *)cmd;	/* cast away const */
> +	int rcode = 0;
> +
> +	if (len == -1) {
> +		len = strlen(cmd);
> +#ifdef CONFIG_SYS_HUSH_PARSER
> +		/* hush will never change our string */
> +		need_buff = 0;
> +#else
> +		/* the built-in parser will change our string if it sees \n */
> +		need_buff = strchr(cmd, '\n') != NULL;
> +#endif
> +	}

we have memchr(), so you should be able to split the len==-1 and the need_buff 
logic into two sep steps

also, should you handle the case where '\n' is the very last char ?  or not 
bother ?
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120401/846a2a5e/attachment.pgp>

[U-Boot] [PATCH v3 4/4] sandbox: Add basic test for command execution

[Mike Frysinger]

On Saturday 31 March 2012 03:30:58 Simon Glass wrote:
> --- /dev/null
> +++ b/test/Makefile
>
> +include $(TOPDIR)/config.mk
> +
> +LIB	= $(obj)libtest.o
> +
> +COBJS-$(CONFIG_SANDBOX) += command_ut.o
> +
> +COBJS	:= $(sort $(COBJS-y))

that sort actually needed ?

> +SRCS	:= $(COBJS:.o=.c)
> +OBJS	:= $(addprefix $(obj),$(COBJS))
> +
> +all:	$(LIB) $(XOBJS)

$(XOBJS) is dead code

> --- /dev/null
> +++ b/test/command_ut.c
>
> +#define DEBUG

should comment why you've always defined this

> +static const char test_cmd[] = "setenv list 1\n setenv list ${list}2; "
> +		"setenv list ${list}3\0"
> +		"setenv list ${list}4";

i'd put the first string on a new line too to make it easier to read

> +static int do_ut_cmd(cmd_tbl_t *cmdtp, int flag, int argc, char * const

what is "ut" supposed to stand for ?
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120401/c96a3750/attachment.pgp>

[U-Boot] [PATCH v3 3/4] sandbox: Use the new run_command()

[Mike Frysinger]

On Saturday 31 March 2012 03:30:57 Simon Glass wrote:
> Now that run_command() handles both parsers, clean up sandbox to use it.
> This fixes a build error.

Acked-by: Mike Frysinger <vapier@gentoo.org>
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120401/25f6088a/attachment.pgp>

[U-Boot] [PATCH v3 1/4] Add run_command_list() to run a list of commands

[Simon Glass]

Hi Mike,

On Sun, Apr 1, 2012 at 12:48 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Saturday 31 March 2012 03:30:55 Simon Glass wrote:
>> --- a/common/cmd_pxe.c
>> +++ b/common/cmd_pxe.c
>>
>> + ? ? return run_command_list(localcmd, strlen(localcmd), 0);
>
> should be -1 instead of strlen()

done

>
>> +int run_command_list(const char *cmd, int len, int flag)
>> +{
>> + ? ? int need_buff = 1;
>> + ? ? char *buff = (char *)cmd; ? ? ? /* cast away const */
>> + ? ? int rcode = 0;
>> +
>> + ? ? if (len == -1) {
>> + ? ? ? ? ? ? len = strlen(cmd);
>> +#ifdef CONFIG_SYS_HUSH_PARSER
>> + ? ? ? ? ? ? /* hush will never change our string */
>> + ? ? ? ? ? ? need_buff = 0;
>> +#else
>> + ? ? ? ? ? ? /* the built-in parser will change our string if it sees \n */
>> + ? ? ? ? ? ? need_buff = strchr(cmd, '\n') != NULL;
>> +#endif
>> + ? ? }
>
> we have memchr(), so you should be able to split the len==-1 and the need_buff
> logic into two sep steps

Are you saying that we should do this check if len is not -1 also?

I am only doing it when len is not specified, since if a length is
provided we must allocate. This is used by the source command, which
does not necessary have a nul-terminated string.

 * Note that if 'len' is not -1, then the command does not need to be nul
 * terminated, Memory will be allocated for the command in that case.

Sorry if I am missing your point.

>
> also, should you handle the case where '\n' is the very last char ? ?or not
> bother ?

Do you mean not allocate in that case? I don't think it is a common
situation is it?

> -mike

Regards,
Simon

[U-Boot] [PATCH v3 4/4] sandbox: Add basic test for command execution

[Simon Glass]

Hi Mike,

On Sun, Apr 1, 2012 at 12:53 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Saturday 31 March 2012 03:30:58 Simon Glass wrote:
>> --- /dev/null
>> +++ b/test/Makefile
>>
>> +include $(TOPDIR)/config.mk
>> +
>> +LIB ?= $(obj)libtest.o
>> +
>> +COBJS-$(CONFIG_SANDBOX) += command_ut.o
>> +
>> +COBJS ? ? ? ?:= $(sort $(COBJS-y))

Not yet :-) I like what it does to object files, but it is pointless
at least for now. Will drop it.

>
> that sort actually needed ?
>
>> +SRCS := $(COBJS:.o=.c)
>> +OBJS := $(addprefix $(obj),$(COBJS))
>> +
>> +all: $(LIB) $(XOBJS)
>
> $(XOBJS) is dead code

Removed

>
>> --- /dev/null
>> +++ b/test/command_ut.c
>>
>> +#define DEBUG
>
> should comment why you've always defined this

done

>
>> +static const char test_cmd[] = "setenv list 1\n setenv list ${list}2; "
>> + ? ? ? ? ? ? "setenv list ${list}3\0"
>> + ? ? ? ? ? ? "setenv list ${list}4";
>
> i'd put the first string on a new line too to make it easier to read

Done

>
>> +static int do_ut_cmd(cmd_tbl_t *cmdtp, int flag, int argc, char * const
>
> what is "ut" supposed to stand for ?

Unit test, as normally needed by code that is utterly tortuous.

> -mike

Regards,
Simon

[U-Boot] [PATCH v3 1/4] Add run_command_list() to run a list of commands

[Mike Frysinger]

On Wednesday 04 April 2012 03:12:27 Simon Glass wrote:
> On Sun, Apr 1, 2012 at 12:48 PM, Mike Frysinger wrote:
> > On Saturday 31 March 2012 03:30:55 Simon Glass wrote:
> >> --- a/common/cmd_pxe.c
> >> +++ b/common/cmd_pxe.c
> >> 
> >> +int run_command_list(const char *cmd, int len, int flag)
> >> +{
> >> +     int need_buff = 1;
> >> +     char *buff = (char *)cmd;       /* cast away const */
> >> +     int rcode = 0;
> >> +
> >> +     if (len == -1) {
> >> +             len = strlen(cmd);
> >> +#ifdef CONFIG_SYS_HUSH_PARSER
> >> +             /* hush will never change our string */
> >> +             need_buff = 0;
> >> +#else
> >> +             /* the built-in parser will change our string if it sees
> >> \n */ +             need_buff = strchr(cmd, '\n') != NULL;
> >> +#endif
> >> +     }
> > 
> > we have memchr(), so you should be able to split the len==-1 and the
> > need_buff logic into two sep steps
> 
> Are you saying that we should do this check if len is not -1 also?
> 
> I am only doing it when len is not specified, since if a length is
> provided we must allocate. This is used by the source command, which
> does not necessary have a nul-terminated string.

looks to me like it should be:
	int need_buff;

	if (len == -1)
		len = strlen(cmd);
#ifdef CONFIG_SYS_HUSH_PARSER
	need_buff = 0;
#else
	need_buff = memchr(cmd, '\n', len) != NULL;
#endif
	if (need_buff) {
		...
	}

> > also, should you handle the case where '\n' is the very last char ?  or
> > not bother ?
> 
> Do you mean not allocate in that case? I don't think it is a common
> situation is it?

i don't know how commands get passed down (as i've never poked that area 
before) which is why i'm asking
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120408/a54f35ff/attachment.pgp>

[U-Boot] [PATCH v3 4/4] sandbox: Add basic test for command execution

[Mike Frysinger]

On Wednesday 04 April 2012 03:18:30 Simon Glass wrote:
> On Sun, Apr 1, 2012 at 12:53 PM, Mike Frysinger wrote:
> > On Saturday 31 March 2012 03:30:58 Simon Glass wrote:
> >> +static int do_ut_cmd(cmd_tbl_t *cmdtp, int flag, int argc, char * const
> > 
> > what is "ut" supposed to stand for ?
> 
> Unit test, as normally needed by code that is utterly tortuous.

i'd use "unit test" in the cmd help string then
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120408/4d1efa41/attachment.pgp>

[U-Boot] [PATCH v3 1/4] Add run_command_list() to run a list of commands

[Simon Glass]

Hi Mike,

On Sun, Apr 8, 2012 at 1:39 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Wednesday 04 April 2012 03:12:27 Simon Glass wrote:
>> On Sun, Apr 1, 2012 at 12:48 PM, Mike Frysinger wrote:
>> > On Saturday 31 March 2012 03:30:55 Simon Glass wrote:
>> >> --- a/common/cmd_pxe.c
>> >> +++ b/common/cmd_pxe.c
>> >>
>> >> +int run_command_list(const char *cmd, int len, int flag)
>> >> +{
>> >> + ? ? int need_buff = 1;
>> >> + ? ? char *buff = (char *)cmd; ? ? ? /* cast away const */
>> >> + ? ? int rcode = 0;
>> >> +
>> >> + ? ? if (len == -1) {
>> >> + ? ? ? ? ? ? len = strlen(cmd);
>> >> +#ifdef CONFIG_SYS_HUSH_PARSER
>> >> + ? ? ? ? ? ? /* hush will never change our string */
>> >> + ? ? ? ? ? ? need_buff = 0;
>> >> +#else
>> >> + ? ? ? ? ? ? /* the built-in parser will change our string if it sees
>> >> \n */ + ? ? ? ? ? ? need_buff = strchr(cmd, '\n') != NULL;
>> >> +#endif
>> >> + ? ? }
>> >
>> > we have memchr(), so you should be able to split the len==-1 and the
>> > need_buff logic into two sep steps
>>
>> Are you saying that we should do this check if len is not -1 also?
>>
>> I am only doing it when len is not specified, since if a length is
>> provided we must allocate. This is used by the source command, which
>> does not necessary have a nul-terminated string.
>
> looks to me like it should be:
> ? ? ? ?int need_buff;
>
> ? ? ? ?if (len == -1)
> ? ? ? ? ? ? ? ?len = strlen(cmd);
> #ifdef CONFIG_SYS_HUSH_PARSER
> ? ? ? ?need_buff = 0;
> #else
> ? ? ? ?need_buff = memchr(cmd, '\n', len) != NULL;
> #endif
> ? ? ? ?if (need_buff) {
> ? ? ? ? ? ? ? ?...
> ? ? ? ?}
>

This logic is different. For example it won't nul terminate a
terminate a command string that is passed in with no terminator. I
think this is required by the 'source' command.

>> > also, should you handle the case where '\n' is the very last char ? ?or
>> > not bother ?
>>
>> Do you mean not allocate in that case? I don't think it is a common
>> situation is it?
>
> i don't know how commands get passed down (as i've never poked that area
> before) which is why i'm asking

Well within the normal command parses run_command() is called. They
split up the commands for us. We only need run_command_list() when we
think we might have a raw list of commands, on which no processing has
been done.

Regards,
Simon

> -mike

[U-Boot] [PATCH v3 3/4] sandbox: Use the new run_command()

[Wolfgang Denk]

Dear Simon Glass,

In message <1333179058-19598-3-git-send-email-sjg@chromium.org> you wrote:
> Now that run_command() handles both parsers, clean up sandbox to use it.
> This fixes a build error.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> Changes in v3:
> - Add new patch to clean up sandbox's run_command() usage
> 
>  arch/sandbox/cpu/start.c |    6 ------
>  1 files changed, 0 insertions(+), 6 deletions(-)

Applied, thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
If the facts don't fit the theory, change the facts.
                                                   -- Albert Einstein

[U-Boot] [PATCH v3 1/4] Add run_command_list() to run a list of commands

[Wolfgang Denk]

Dear Simon Glass,

In message <1333179058-19598-1-git-send-email-sjg@chromium.org> you wrote:
> This new function runs a list of commands separated by semicolon or newline.
> We move this out of cmd_source so that it can be used by other code. The
> PXE code also uses the new function.
> 
> Suggested-by: Michael Walle <michael@walle.cc>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> Changes in v3:
> - Added a few more comments on the need for malloc()
> - Change PXE's printf() to a debug()
> - Fix bug in built-in run_command()
> - Fix commit message to mention ; and \n
> - Move run_command_list() comment to header file
> 
>  common/cmd_pxe.c    |   20 ++----------
>  common/cmd_source.c |   49 +----------------------------
>  common/main.c       |   85 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/common.h    |   13 ++++++++
>  4 files changed, 102 insertions(+), 65 deletions(-)

Applied, thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Swap read error.  You lose your mind.

[U-Boot] [PATCH v3 2/4] Allow newlines within command environment vars

[Wolfgang Denk]

Dear Simon Glass,

In message <1333179058-19598-2-git-send-email-sjg@chromium.org> you wrote:
> Any environment variable can hold commands to be executed by the 'run'
> command. The environment variables preboot, bootcmd and menucmd have
> special code for triggering execution in certain circumstances.
> 
> We adjust these calls to use run_command_list() instead of run_command().
> This change permits these variables to have embedded newlines so that
> they work the same as the 'source' command.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> Changes in v2:
> - Update commit message to be less confusing
> 
> Changes in v3:
> - Remove unneeded len parameter in calls to run_command_list
> 
>  common/main.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)

Applied, thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The human mind treats a new idea the way the body  treats  a  strange
protein - it rejects it.                                 - P. Medawar

[U-Boot] [PATCH v3 4/4] sandbox: Add basic test for command execution

[Wolfgang Denk]

Dear Simon Glass,

In message <1333179058-19598-4-git-send-email-sjg@chromium.org> you wrote:
> Since run_command() and run_command_list() are important and a little
> confusing, add some basic tests to check that the behaviour is correct.
> 
> Note: I am not sure that this should be committed, nor where it should go
> in the source tree. Comments welcome.
> 
> To run the unit tests use the ut_cmd command available in sandbox:
> 
> make sandbox_config
> make
> ./u-boot -c ut_cmd
> 
> (To test both hush and built-in parsers, you need to manually change
> CONFIG_SYS_HUSH_PARSER in include/configs/sandbox.h and build/run again)
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> Changes in v3:
> - Add some unit tests for run_command() and run_command_list()
> 
>  Makefile          |    1 +
>  test/Makefile     |   45 ++++++++++++++++++++++++++++
>  test/command_ut.c |   85 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 131 insertions(+), 0 deletions(-)
>  create mode 100644 test/Makefile
>  create mode 100644 test/command_ut.c

Applied, thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
It usually takes more than three weeks to prepare  a  good  impromptu
speech.                                                  - Mark Twain