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

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

[Simon Glass]

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

Suggested-by: Michael Walle <michael@walle.cc>
Signed-off-by: Simon Glass <sjg@chromium.org>
---
 common/cmd_pxe.c    |   20 ++------------
 common/cmd_source.c |   49 +----------------------------------
 common/main.c       |   72 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/common.h    |    1 +
 4 files changed, 77 insertions(+), 65 deletions(-)

diff --git a/common/cmd_pxe.c b/common/cmd_pxe.c
index 8a68fa1..4b18d0b 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);
+	printf("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..87f2855 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,77 @@ int run_command(const char *cmd, int flag)
 #endif
 }
 
+#ifndef CONFIG_SYS_HUSH_PARSER
+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 && *cmd)
+		rcode = (builtin_run_command(cmd, 0) >= 0);
+
+	return rcode;
+}
+#endif
+
+/*
+ * Run a list of commands separated by ;
+ *
+ * Note that if 'len' is not -1, then the command may not be \0 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 command 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 need_buff = 1;
+	char *buff = (char *)cmd;
+	int rcode = 0;
+
+	if (len == -1) {
+		len = strlen(cmd);
+		need_buff = strchr(cmd, '\n') != NULL;
+	}
+	if (need_buff) {
+		buff = malloc(len + 1);
+		if (!buff)
+			return 1;
+		memcpy(buff, cmd, len + 1);
+	}
+#ifdef CONFIG_SYS_HUSH_PARSER
+	rcode = parse_string_outer(buff, FLAG_PARSE_SEMICOLON);
+#else
+	rcode = builtin_run_command_list(buff, flag);
+#endif
+	if (need_buff)
+		free(buff);
+
+	return rcode;
+}
+
 /****************************************************************************/
 
 #if defined(CONFIG_CMD_RUN)
diff --git a/include/common.h b/include/common.h
index 0bda049..cc5d61c 100644
--- a/include/common.h
+++ b/include/common.h
@@ -261,6 +261,7 @@ 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);
+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 2/2] Allow newlines within command environment vars

[Simon Glass]

The environment variables preboot, bootcmd and menucmd can hold a command
to execute. This change permits these variables to have newlines so that
they work the same as the 'source' command.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 common/main.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/common/main.c b/common/main.c
index 87f2855..48d6b12 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, strlen(p), 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, strlen(s), 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, strlen(s), 0);
 	}
 #endif /* CONFIG_MENUKEY */
 #endif /* CONFIG_BOOTDELAY */
-- 
1.7.7.3

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

[Wolfgang Denk]

Dear Simon Glass,

In message <1329286030-32560-2-git-send-email-sjg@chromium.org> you wrote:
> The environment variables preboot, bootcmd and menucmd can hold a command
> to execute. This change permits these variables to have newlines so that
> they work the same as the 'source' command.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>  common/main.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)

Please squash this into the patch that introduces run_command_list()
[assuming this is the patch which breaks behaviour?]

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 word "fit", as I understand it, means "appropriate to a purpose",
and I would say the body of the Dean is supremely appropriate to  the
purpose of sitting around all day and eating big heavy meals.
                                 - Terry Pratchett, _Moving Pictures_

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

[Simon Glass]

Hi Wolfgang,

On Tue, Feb 14, 2012 at 11:31 PM, Wolfgang Denk <wd@denx.de> wrote:

> Dear Simon Glass,
>
> In message <1329286030-32560-2-git-send-email-sjg@chromium.org> you wrote:
> > The environment variables preboot, bootcmd and menucmd can hold a command
> > to execute. This change permits these variables to have newlines so that
> > they work the same as the 'source' command.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >  common/main.c |    6 +++---
> >  1 files changed, 3 insertions(+), 3 deletions(-)
>
> Please squash this into the patch that introduces run_command_list()
> [assuming this is the patch which breaks behaviour?]
>

The first patch is just a clean-up and should not change any behaviour.
This second patch changes the behaviour of the named env variables.

I will combine them.

Regards,
Simon

>
> 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 word "fit", as I understand it, means "appropriate to a purpose",
> and I would say the body of the Dean is supremely appropriate to  the
> purpose of sitting around all day and eating big heavy meals.
>                                 - Terry Pratchett, _Moving Pictures_
>

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

[Michael Walle]

Am Mittwoch 15 Februar 2012, 07:07:09 schrieb Simon Glass:
> This new function runs a list of commands separated by semicolon. We
> move this out of cmd_source so that it can be used by other code. The
> PXE also uses the new function.
> 
> Suggested-by: Michael Walle <michael@walle.cc>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>  common/cmd_pxe.c    |   20 ++------------
>  common/cmd_source.c |   49 +----------------------------------
>  common/main.c       |   72
> +++++++++++++++++++++++++++++++++++++++++++++++++++ include/common.h    | 
>   1 +
>  4 files changed, 77 insertions(+), 65 deletions(-)
> 
[..snip..]

> diff --git a/common/main.c b/common/main.c
> index db181d3..87f2855 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,77 @@ int run_command(const char *cmd, int flag)
>  #endif
>  }
> 
> +#ifndef CONFIG_SYS_HUSH_PARSER
> +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 && *cmd)
> +		rcode = (builtin_run_command(cmd, 0) >= 0);
> +
> +	return rcode;
> +}
> +#endif
> +
> +/*
> + * Run a list of commands separated by ;
mh, where is the command string seperated by ';' if no hush is used?

> + *
> + * Note that if 'len' is not -1, then the command may not be \0
> 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 command 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 need_buff = 1;
> +	char *buff = (char *)cmd;
mhhh, builtin_run_command_list will modify buff. what do you think about 
always copying the buffer if no hush parser is used?

> +	int rcode = 0;
> +
> +	if (len == -1) {
> +		len = strlen(cmd);
> +		need_buff = strchr(cmd, '\n') != NULL;
> +	}
> +	if (need_buff) {
> +		buff = malloc(len + 1);
> +		if (!buff)
> +			return 1;
> +		memcpy(buff, cmd, len + 1);
> +	}
> +#ifdef CONFIG_SYS_HUSH_PARSER
> +	rcode = parse_string_outer(buff, FLAG_PARSE_SEMICOLON);
> +#else
> +	rcode = builtin_run_command_list(buff, flag);
> +#endif
> +	if (need_buff)
> +		free(buff);
> +
> +	return rcode;
> +}
> +
>  /*************************************************************************
> ***/
> 
>  #if defined(CONFIG_CMD_RUN)
> diff --git a/include/common.h b/include/common.h
> index 0bda049..cc5d61c 100644
> --- a/include/common.h
> +++ b/include/common.h
> @@ -261,6 +261,7 @@ 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);
> +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);

-- 
Michael

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

[Simon Glass]

Hi Michael,

On Wed, Feb 15, 2012 at 11:38 AM, Michael Walle <michael@walle.cc> wrote:
> Am Mittwoch 15 Februar 2012, 07:07:09 schrieb Simon Glass:
>> This new function runs a list of commands separated by semicolon. We
>> move this out of cmd_source so that it can be used by other code. The
>> PXE also uses the new function.
>>
>> Suggested-by: Michael Walle <michael@walle.cc>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>> ?common/cmd_pxe.c ? ?| ? 20 ++------------
>> ?common/cmd_source.c | ? 49 +----------------------------------
>> ?common/main.c ? ? ? | ? 72
>> +++++++++++++++++++++++++++++++++++++++++++++++++++ include/common.h ? ?|
>> ? 1 +
>> ?4 files changed, 77 insertions(+), 65 deletions(-)
>>
> [..snip..]
>
>> diff --git a/common/main.c b/common/main.c
>> index db181d3..87f2855 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,77 @@ int run_command(const char *cmd, int flag)
>> ?#endif
>> ?}
>>
>> +#ifndef CONFIG_SYS_HUSH_PARSER
>> +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 && *cmd)
>> + ? ? ? ? ? ? rcode = (builtin_run_command(cmd, 0) >= 0);
>> +
>> + ? ? return rcode;
>> +}
>> +#endif
>> +
>> +/*
>> + * Run a list of commands separated by ;
> mh, where is the command string seperated by ';' if no hush is used?

In that case it is handled by builtin_run_command() already.

>
>> + *
>> + * Note that if 'len' is not -1, then the command may not be \0
>> 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 command 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 need_buff = 1;
>> + ? ? char *buff = (char *)cmd;
> mhhh, builtin_run_command_list will modify buff. what do you think about
> always copying the buffer if no hush parser is used?

Yes we could do - how would that help?

>
>> + ? ? int rcode = 0;
>> +
>> + ? ? if (len == -1) {
>> + ? ? ? ? ? ? len = strlen(cmd);
>> + ? ? ? ? ? ? need_buff = strchr(cmd, '\n') != NULL;
>> + ? ? }
>> + ? ? if (need_buff) {
>> + ? ? ? ? ? ? buff = malloc(len + 1);
>> + ? ? ? ? ? ? if (!buff)
>> + ? ? ? ? ? ? ? ? ? ? return 1;
>> + ? ? ? ? ? ? memcpy(buff, cmd, len + 1);
>> + ? ? }
>> +#ifdef CONFIG_SYS_HUSH_PARSER
>> + ? ? rcode = parse_string_outer(buff, FLAG_PARSE_SEMICOLON);
>> +#else
>> + ? ? rcode = builtin_run_command_list(buff, flag);
>> +#endif
>> + ? ? if (need_buff)
>> + ? ? ? ? ? ? free(buff);
>> +
>> + ? ? return rcode;
>> +}
>> +
>> ?/*************************************************************************
>> ***/
>>
>> ?#if defined(CONFIG_CMD_RUN)
>> diff --git a/include/common.h b/include/common.h
>> index 0bda049..cc5d61c 100644
>> --- a/include/common.h
>> +++ b/include/common.h
>> @@ -261,6 +261,7 @@ 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);
>> +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);
>
> --
> Michael

Regards,
Simon

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

[Wolfgang Denk]

Dear Simon Glass,

In message <CAPnjgZ0XueZukodaNSdQSYvuevyiH5Z-=3-2pkcC7OekddG-JA@mail.gmail.com> you wrote:
>
> > In message <1329286030-32560-2-git-send-email-sjg@chromium.org> you wrote:
> > > The environment variables preboot, bootcmd and menucmd can hold a command
> > > to execute. This change permits these variables to have newlines so that
> > > they work the same as the 'source' command.
> > >
> > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > ---
> > >  common/main.c |    6 +++---
> > >  1 files changed, 3 insertions(+), 3 deletions(-)
> >
> > Please squash this into the patch that introduces run_command_list()
> > [assuming this is the patch which breaks behaviour?]
> >
> 
> The first patch is just a clean-up and should not change any behaviour.
> This second patch changes the behaviour of the named env variables.

So which commit introduced the breakage, then?

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
A stone was placed at a ford in a river with the inscription:
"When this stone is covered it is dangerous to ford here."

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

[Michael Walle]

Am Mittwoch 15 Februar 2012, 23:23:19 schrieb Simon Glass:
> Hi Michael,
> 
> On Wed, Feb 15, 2012 at 11:38 AM, Michael Walle <michael@walle.cc> wrote:
> > Am Mittwoch 15 Februar 2012, 07:07:09 schrieb Simon Glass:
> >> This new function runs a list of commands separated by semicolon. We
> >> move this out of cmd_source so that it can be used by other code. The
> >> PXE also uses the new function.
> >> 
> >> Suggested-by: Michael Walle <michael@walle.cc>
> >> Signed-off-by: Simon Glass <sjg@chromium.org>
> >> ---
> >>  common/cmd_pxe.c    |   20 ++------------
> >>  common/cmd_source.c |   49 +----------------------------------
> >>  common/main.c       |   72
> >> +++++++++++++++++++++++++++++++++++++++++++++++++++ include/common.h  
> >>  | 1 +
> >>  4 files changed, 77 insertions(+), 65 deletions(-)
> > 
> > [..snip..]
> > 
> >> diff --git a/common/main.c b/common/main.c
> >> index db181d3..87f2855 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,77 @@ int run_command(const char *cmd, int flag)
> >>  #endif
> >>  }
> >> 
> >> +#ifndef CONFIG_SYS_HUSH_PARSER
> >> +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 && *cmd)
> >> +             rcode = (builtin_run_command(cmd, 0) >= 0);
> >> +
> >> +     return rcode;
> >> +}
> >> +#endif
> >> +
> >> +/*
> >> + * Run a list of commands separated by ;
> > 
> > mh, where is the command string seperated by ';' if no hush is used?
> 
> In that case it is handled by builtin_run_command() already.
> 
> >> + *
> >> + * Note that if 'len' is not -1, then the command may not be \0
> >> 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 command 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 need_buff = 1;
> >> +     char *buff = (char *)cmd;
> > 
> > mhhh, builtin_run_command_list will modify buff. what do you think about
> > always copying the buffer if no hush parser is used?
> 
> Yes we could do - how would that help?
ah sorry, i hadn't understood your code. too late here ;)

copying the buffer everytime (in case no hush is available):
 - removes the cast, which seems very odd on the first look
 - makes run_command_list() more robust against furture changes in
   builtin_run_command_list(), eg. if it modifies the string not only if
   '\n' is found in the cmd.

at least you should add a comment to keep the check for need_buff in sync with 
builtin_run_command_list().


-- 
Michael

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

[Simon Glass]

Hi Michael,

On Wed, Feb 15, 2012 at 3:55 PM, Michael Walle <michael@walle.cc> wrote:
> Am Mittwoch 15 Februar 2012, 23:23:19 schrieb Simon Glass:
>> Hi Michael,
>>
>> On Wed, Feb 15, 2012 at 11:38 AM, Michael Walle <michael@walle.cc> wrote:
>> > Am Mittwoch 15 Februar 2012, 07:07:09 schrieb Simon Glass:
>> >> This new function runs a list of commands separated by semicolon. We
>> >> move this out of cmd_source so that it can be used by other code. The
>> >> PXE also uses the new function.
>> >>
>> >> Suggested-by: Michael Walle <michael@walle.cc>
>> >> Signed-off-by: Simon Glass <sjg@chromium.org>
>> >> ---
>> >> ?common/cmd_pxe.c ? ?| ? 20 ++------------
>> >> ?common/cmd_source.c | ? 49 +----------------------------------
>> >> ?common/main.c ? ? ? | ? 72
>> >> +++++++++++++++++++++++++++++++++++++++++++++++++++ include/common.h
>> >> ?| 1 +
>> >> ?4 files changed, 77 insertions(+), 65 deletions(-)
>> >
>> > [..snip..]
>> >
>> >> diff --git a/common/main.c b/common/main.c
>> >> index db181d3..87f2855 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,77 @@ int run_command(const char *cmd, int flag)
>> >> ?#endif
>> >> ?}
>> >>
>> >> +#ifndef CONFIG_SYS_HUSH_PARSER
>> >> +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 && *cmd)
>> >> + ? ? ? ? ? ? rcode = (builtin_run_command(cmd, 0) >= 0);
>> >> +
>> >> + ? ? return rcode;
>> >> +}
>> >> +#endif
>> >> +
>> >> +/*
>> >> + * Run a list of commands separated by ;
>> >
>> > mh, where is the command string seperated by ';' if no hush is used?
>>
>> In that case it is handled by builtin_run_command() already.
>>
>> >> + *
>> >> + * Note that if 'len' is not -1, then the command may not be \0
>> >> 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 command 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 need_buff = 1;
>> >> + ? ? char *buff = (char *)cmd;
>> >
>> > mhhh, builtin_run_command_list will modify buff. what do you think about
>> > always copying the buffer if no hush parser is used?
>>
>> Yes we could do - how would that help?
> ah sorry, i hadn't understood your code. too late here ;)
>
> copying the buffer everytime (in case no hush is available):
> ?- removes the cast, which seems very odd on the first look

I can add a comment.

> ?- makes run_command_list() more robust against furture changes in
> ? builtin_run_command_list(), eg. if it modifies the string not only if
> ? '\n' is found in the cmd.

Yes that's true, and I wasn't able to pass 'cmd' which would solve the
problem at compile time.

>
> at least you should add a comment to keep the check for need_buff in sync with
> builtin_run_command_list().

Yes, will do that. I am a little reluctant to always malloc() since it
introduces a malloc() even in the trivial case where we want to
execute the contents of a simple env variable with no newline.

>
>
> --
> Michael

Regards,
Simon

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

[Simon Glass]

Hi Wolfgang,

On Wed, Feb 15, 2012 at 2:49 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <CAPnjgZ0XueZukodaNSdQSYvuevyiH5Z-=3-2pkcC7OekddG-JA@mail.gmail.com> you wrote:
>>
>> > In message <1329286030-32560-2-git-send-email-sjg@chromium.org> you wrote:
>> > > The environment variables preboot, bootcmd and menucmd can hold a command
>> > > to execute. This change permits these variables to have newlines so that
>> > > they work the same as the 'source' command.
>> > >
>> > > Signed-off-by: Simon Glass <sjg@chromium.org>
>> > > ---
>> > > ?common/main.c | ? ?6 +++---
>> > > ?1 files changed, 3 insertions(+), 3 deletions(-)
>> >
>> > Please squash this into the patch that introduces run_command_list()
>> > [assuming this is the patch which breaks behaviour?]
>> >
>>
>> The first patch is just a clean-up and should not change any behaviour.
>> This second patch changes the behaviour of the named env variables.
>
> So which commit introduced the breakage, then?

Which breakage are you referring to? The intent of these two patches
is to add a new feature (proposed by Michael Walle <michael@walle.cc>
who also provided a patch) on top of the command refactor series.

Sorry, there is some confusion here but I'm not sure what it is.

Regards,
Simon
>
> 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
> A stone was placed at a ford in a river with the inscription:
> "When this stone is covered it is dangerous to ford here."

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

[Wolfgang Denk]

Dear Simon Glass,

In message <CAPnjgZ1BMMrN-+bHFr9GcH-5wF4WSz_7Qa53C_cHPLEbL2ycqA@mail.gmail.com> you wrote:
...
> >> > > The environment variables preboot, bootcmd and menucmd can hold a command
> >> > > to execute. This change permits these variables to have newlines so that
> >> > > they work the same as the 'source' command.
...
> >> The first patch is just a clean-up and should not change any behaviour.
> >> This second patch changes the behaviour of the named env variables.
> >
> > So which commit introduced the breakage, then?
>
> Which breakage are you referring to? The intent of these two patches
> is to add a new feature (proposed by Michael Walle <michael@walle.cc>
> who also provided a patch) on top of the command refactor series.
>
> Sorry, there is some confusion here but I'm not sure what it is.

Guess I'm confused.

I don't understand in which way the variables listed here (preboot,
bootcmd and menucmd) are special - actually all variables can hold
commands, that then can be executed using the "run" command.

If newline is a valid command separator (and I think it is), this
should work for _all_ variable.



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
Backed up the system lately?

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

[Simon Glass]

Hi Wolfgang,

On Tue, Mar 6, 2012 at 7:26 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <CAPnjgZ1BMMrN-+bHFr9GcH-5wF4WSz_7Qa53C_cHPLEbL2ycqA@mail.gmail.com> you wrote:
> ...
>> >> > > The environment variables preboot, bootcmd and menucmd can hold a command
>> >> > > to execute. This change permits these variables to have newlines so that
>> >> > > they work the same as the 'source' command.
> ...
>> >> The first patch is just a clean-up and should not change any behaviour.
>> >> This second patch changes the behaviour of the named env variables.
>> >
>> > So which commit introduced the breakage, then?
>>
>> Which breakage are you referring to? The intent of these two patches
>> is to add a new feature (proposed by Michael Walle <michael@walle.cc>
>> who also provided a patch) on top of the command refactor series.
>>
>> Sorry, there is some confusion here but I'm not sure what it is.
>
> Guess I'm confused.
>
> I don't understand in which way the variables listed here (preboot,
> bootcmd and menucmd) are special - actually all variables can hold
> commands, that then can be executed using the "run" command.
>
> If newline is a valid command separator (and I think it is), this
> should work for _all_ variable.

OK I see. It is my commit message that is confusing, sorry. I didn't
mean that only those env variables can hold a command, just that this
patch affects the code which makes those environment variables work.

I will update the commit message and resend this patch.

Regards,
Simon

>
>
>
> 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
> Backed up the system lately?

[U-Boot] [PATCH v2 2/2] 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

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

diff --git a/common/main.c b/common/main.c
index 87f2855..48d6b12 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, strlen(p), 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, strlen(s), 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, strlen(s), 0);
 	}
 #endif /* CONFIG_MENUKEY */
 #endif /* CONFIG_BOOTDELAY */
-- 
1.7.7.3

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

[Wolfgang Denk]

Dear Simon Glass,

In message <1329286030-32560-1-git-send-email-sjg@chromium.org> you wrote:
> This new function runs a list of commands separated by semicolon. We
> move this out of cmd_source so that it can be used by other code. The
> PXE also uses the new function.

Separated by semicolon?  What about newline here?

> +	printf("running: %s\n", localcmd);

Should this be a debug() ?

> +	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 && *cmd)
> +		rcode = (builtin_run_command(cmd, 0) >= 0);

This looks wrong to me.  There you are re-executing the original
command.  Shoudl this not be

	if (rcode == 0 && *line)
		rcode = (run_command(line, 0) >= 0);

??

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
Computers are not intelligent.  They only think they are.

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

[Wolfgang Denk]

Dear Simon Glass,

In message <1329286030-32560-2-git-send-email-sjg@chromium.org> you wrote:
> The environment variables preboot, bootcmd and menucmd can hold a command
> to execute. This change permits these variables to have newlines so that
> they work the same as the 'source' command.

This comment needs to be fixed. Not only these variables are effected,
and from the patch it is not visible what it has to do with newlines.

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
Man did not weave the web of life; he  is  merely  a  strand  in  it.
Whatever he does to the web, he does to himself.     - Seattle [1854]

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

[Simon Glass]

Hi Wolfgang,

On Wed, Mar 7, 2012 at 3:34 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <1329286030-32560-1-git-send-email-sjg@chromium.org> you wrote:
>> This new function runs a list of commands separated by semicolon. We
>> move this out of cmd_source so that it can be used by other code. The
>> PXE also uses the new function.

This took a bit longer to get back to than I expected.

>
> Separated by semicolon? ?What about newline here?
>
>> + ? ? printf("running: %s\n", localcmd);
>
> Should this be a debug() ?

I'm not sure - the old code has printf() but I don't know why. I will change it.

>
>> + ? ? 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 && *cmd)
>> + ? ? ? ? ? ? rcode = (builtin_run_command(cmd, 0) >= 0);
>
> This looks wrong to me. ?There you are re-executing the original
> command. ?Shoudl this not be
>
> ? ? ? ?if (rcode == 0 && *line)
> ? ? ? ? ? ? ? ?rcode = (run_command(line, 0) >= 0);
>
> ??

Yes you are right of course. I created a few tests and found a few
other nits as well, so will send a new series that tidies all this up.

Regards,
Simon

>
> 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
> Computers are not intelligent. ?They only think they are.