[U-Boot] [RFC 0/2 v2] Remove CONFIG_SYS_MAXARGS

[U-Boot] [RFC 0/2 v2] Remove CONFIG_SYS_MAXARGS

[John Schmoller]

The first patch removes CONFIG_SYS_MAXARGS, replacing the staticly defined
array with a malloc'd array of the appropriate size. When a function has no
upper argument limit (ie, was set to CONFIG_SYS_MAXARGS), it is now set to 0
to indicate this fact.  Argument count is now unlimited, within reason and
malloc buffer size.

The second patch removes cmdtp->maxargs and moves the checks to the individual
command functions.  Since most functions do bounds checking anyway, it's a
a fairly cheap task (sometimes free) to remove this bounds check in
common/main.c.  In addition, it's more intuitive (in my opinion) if all bounds
checking is done in only one place for each function.  The second patch also
creates a CMD_ERR_USAGE return value, which prints usage when returned.  The
overall effect of this patch is to reduce code size by an average of 200-250
bytes and, I feel, make things a bit cleaner.

I'm looking for comments on these two patches as they are quite invasive, and
will definitly cause problems for those people who maintain their own code
out-of-tree.  They may also require an additional amount of testing.

Changes since v1:
 - Made 2/2 a subset of the changes of the real patch so it can be submitted
 to the mailing list and isn't too large.
 - Found a change in 2/2 that should have been in 1/2.

John Schmoller (2):
  cmd: Remove CONFIG_SYS_MAXARGS
  command: Remove maxargs from command structure

 board/amcc/makalu/cmd_pll.c                   |    2 +-
 board/esd/du440/du440.c                       |    4 +-
 board/fads/fads.h                             |    1 -
 board/freescale/common/pixis.c                |    4 +-
 board/freescale/mpc8610hpcd/mpc8610hpcd_diu.c |    2 +-
 board/pxa255_idp/pxa_idp.c                    |    2 +-
 common/cmd_boot.c                             |    2 +-
 common/cmd_bootm.c                            |    4 +-
 common/cmd_diag.c                             |    2 +-
 common/cmd_display.c                          |    2 +-
 common/cmd_echo.c                             |    2 +-
 common/cmd_eeprom.c                           |    7 +-
 common/cmd_elf.c                              |   18 ++++--
 common/cmd_help.c                             |    4 +-
 common/cmd_i2c.c                              |   75 +++++++++---------------
 common/cmd_mp.c                               |    2 +-
 common/cmd_nand.c                             |    2 +-
 common/cmd_nvedit.c                           |    8 +-
 common/cmd_onenand.c                          |    2 +-
 common/cmd_test.c                             |    6 +-
 common/command.c                              |   37 ++-----------
 common/hush.c                                 |   10 +--
 common/kgdb.c                                 |    2 +-
 common/lcd.c                                  |    5 +-
 common/main.c                                 |   58 ++++++++++++-------
 cpu/arm_cortexa8/mx51/clock.c                 |    2 +-
 cpu/mpc512x/diu.c                             |    2 +-
 cpu/mpc512x/iim.c                             |    2 +-
 include/command.h                             |   22 ++++---
 include/common.h                              |    4 +-
 30 files changed, 137 insertions(+), 158 deletions(-)

[U-Boot] [RFC 1/2 v2] cmd: Remove CONFIG_SYS_MAXARGS

[John Schmoller]

CONFIG_SYS_MAXARGS is an arbitrary limit on the number of arguments
which can be input at the command line. Remove that arbitrary limit.
Setting maxargs in the cmdtp to 0 will result in the ability to enter
as many arguments as you can hold in CONFIG_SYS_CBSIZE.

Signed-off-by: John Schmoller <jschmoller@xes-inc.com>
---
Since v1:
 - Moved a change from 2/2 to this patch so both compile cleanly independently.

 board/amcc/makalu/cmd_pll.c                   |    2 +-
 board/esd/du440/du440.c                       |    4 +-
 board/fads/fads.h                             |    1 -
 board/freescale/common/pixis.c                |    4 +-
 board/freescale/mpc8610hpcd/mpc8610hpcd_diu.c |    2 +-
 board/pxa255_idp/pxa_idp.c                    |    2 +-
 common/cmd_boot.c                             |    2 +-
 common/cmd_bootm.c                            |    4 +-
 common/cmd_diag.c                             |    2 +-
 common/cmd_display.c                          |    2 +-
 common/cmd_echo.c                             |    2 +-
 common/cmd_help.c                             |    4 +-
 common/cmd_mp.c                               |    2 +-
 common/cmd_nand.c                             |    2 +-
 common/cmd_nvedit.c                           |    8 ++--
 common/cmd_onenand.c                          |    2 +-
 common/cmd_test.c                             |    6 ++--
 common/command.c                              |   37 +++-------------------
 common/hush.c                                 |    5 ++-
 common/kgdb.c                                 |    2 +-
 common/main.c                                 |   41 +++++++++++++++++++------
 cpu/arm_cortexa8/mx51/clock.c                 |    2 +-
 cpu/mpc512x/diu.c                             |    2 +-
 cpu/mpc512x/iim.c                             |    2 +-
 include/command.h                             |    2 +-
 include/common.h                              |    4 ++-
 26 files changed, 72 insertions(+), 76 deletions(-)

diff --git a/board/amcc/makalu/cmd_pll.c b/board/amcc/makalu/cmd_pll.c
index 9bae67e..5cadb4a 100644
--- a/board/amcc/makalu/cmd_pll.c
+++ b/board/amcc/makalu/cmd_pll.c
@@ -236,7 +236,7 @@ ret:
 }
 
 U_BOOT_CMD(
-	pllalter, CONFIG_SYS_MAXARGS, 1,        do_pll_alter,
+	pllalter, 0, 1,        do_pll_alter,
 	"change pll frequence",
 	"pllalter <selection>      - change pll frequence \n\n\
 	** New freq take effect after reset. ** \n\
diff --git a/board/esd/du440/du440.c b/board/esd/du440/du440.c
index 111cce5..0c3d976 100644
--- a/board/esd/du440/du440.c
+++ b/board/esd/du440/du440.c
@@ -841,7 +841,7 @@ int do_time(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 	return ret;
 }
 U_BOOT_CMD(
-	time,	CONFIG_SYS_MAXARGS,	1,	do_time,
+	time,	0,	1,	do_time,
 	"run command and output execution time",
 	""
 );
@@ -891,7 +891,7 @@ int do_gfxdemo(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 	return 0;
 }
 U_BOOT_CMD(
-	gfxdemo,	CONFIG_SYS_MAXARGS,	1,	do_gfxdemo,
+	gfxdemo,	0,	1,	do_gfxdemo,
 	"demo",
 	""
 );
diff --git a/board/fads/fads.h b/board/fads/fads.h
index 4ab4b26..c7d2b2d 100644
--- a/board/fads/fads.h
+++ b/board/fads/fads.h
@@ -140,7 +140,6 @@
 #define	CONFIG_SYS_CBSIZE		256		/* Console I/O Buffer Size	*/
 #endif
 #define	CONFIG_SYS_PBSIZE (CONFIG_SYS_CBSIZE + sizeof(CONFIG_SYS_PROMPT) + 16) /* Print Buffer Size	*/
-#define	CONFIG_SYS_MAXARGS		16		/* max number of command args	*/
 #define CONFIG_SYS_BARGSIZE		CONFIG_SYS_CBSIZE	/* Boot Argument Buffer Size	*/
 
 #define CONFIG_SYS_LOAD_ADDR		0x00100000
diff --git a/board/freescale/common/pixis.c b/board/freescale/common/pixis.c
index 7210512..b4b5d78 100644
--- a/board/freescale/common/pixis.c
+++ b/board/freescale/common/pixis.c
@@ -354,7 +354,7 @@ int pixis_set_sgmii(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 }
 
 U_BOOT_CMD(
-	pixis_set_sgmii, CONFIG_SYS_MAXARGS, 1, pixis_set_sgmii,
+	pixis_set_sgmii, 0, 1, pixis_set_sgmii,
 	"pixis_set_sgmii"
 	" - Enable or disable SGMII mode for a given TSEC \n",
 	"\npixis_set_sgmii [TSEC num] <on|off|switch>\n"
@@ -550,7 +550,7 @@ pixis_reset_cmd(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 
 
 U_BOOT_CMD(
-	pixis_reset, CONFIG_SYS_MAXARGS, 1, pixis_reset_cmd,
+	pixis_reset, 0, 1, pixis_reset_cmd,
 	"Reset the board using the FPGA sequencer",
 	"    pixis_reset\n"
 	"    pixis_reset [altbank]\n"
diff --git a/board/freescale/mpc8610hpcd/mpc8610hpcd_diu.c b/board/freescale/mpc8610hpcd/mpc8610hpcd_diu.c
index 4186a2e..6b8fd28 100644
--- a/board/freescale/mpc8610hpcd/mpc8610hpcd_diu.c
+++ b/board/freescale/mpc8610hpcd/mpc8610hpcd_diu.c
@@ -138,7 +138,7 @@ int mpc8610diu_init_show_bmp(cmd_tbl_t *cmdtp,
 }
 
 U_BOOT_CMD(
-	diufb, CONFIG_SYS_MAXARGS, 1, mpc8610diu_init_show_bmp,
+	diufb, 0, 1, mpc8610diu_init_show_bmp,
 	"Init or Display BMP file",
 	"init\n    - initialize DIU\n"
 	"addr\n    - display bmp at address 'addr'"
diff --git a/board/pxa255_idp/pxa_idp.c b/board/pxa255_idp/pxa_idp.c
index 05e30ec..72ef9e6 100644
--- a/board/pxa255_idp/pxa_idp.c
+++ b/board/pxa255_idp/pxa_idp.c
@@ -128,7 +128,7 @@ int do_idpcmd(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 	return 0;
 }
 
-U_BOOT_CMD(idpcmd, CONFIG_SYS_MAXARGS, 0, do_idpcmd,
+U_BOOT_CMD(idpcmd, 0, 0, do_idpcmd,
 	   "custom IDP command",
 	   "no args at this time"
 );
diff --git a/common/cmd_boot.c b/common/cmd_boot.c
index bfc1db2..1aa6081 100644
--- a/common/cmd_boot.c
+++ b/common/cmd_boot.c
@@ -63,7 +63,7 @@ int do_go (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 /* -------------------------------------------------------------------- */
 
 U_BOOT_CMD(
-	go, CONFIG_SYS_MAXARGS, 1,	do_go,
+	go, 0, 1,	do_go,
 	"start application at address 'addr'",
 	"addr [arg ...]\n    - start application at address 'addr'\n"
 	"      passing 'arg' as arguments"
diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
index 23ab0c4..55f5a42 100644
--- a/common/cmd_bootm.c
+++ b/common/cmd_bootm.c
@@ -984,7 +984,7 @@ static void *boot_get_kernel (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]
 }
 
 U_BOOT_CMD(
-	bootm,	CONFIG_SYS_MAXARGS,	1,	do_bootm,
+	bootm,	0,	1,	do_bootm,
 	"boot application image from memory",
 	"[addr [arg ...]]\n    - boot application image stored in memory\n"
 	"\tpassing arguments 'arg ...'; when booting a Linux kernel,\n"
@@ -1133,7 +1133,7 @@ static int image_info (ulong addr)
 }
 
 U_BOOT_CMD(
-	iminfo,	CONFIG_SYS_MAXARGS,	1,	do_iminfo,
+	iminfo,	0,	1,	do_iminfo,
 	"print header information for application image",
 	"addr [addr ...]\n"
 	"    - print header information for application image starting at\n"
diff --git a/common/cmd_diag.c b/common/cmd_diag.c
index 0436c49..c39b16c 100644
--- a/common/cmd_diag.c
+++ b/common/cmd_diag.c
@@ -65,7 +65,7 @@ int do_diag (cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
 /***************************************************/
 
 U_BOOT_CMD(
-	diag,	CONFIG_SYS_MAXARGS,	0,	do_diag,
+	diag,	0,	0,	do_diag,
 	"perform board diagnostics",
 	     "    - print list of available tests\n"
 	"diag [test1 [test2]]\n"
diff --git a/common/cmd_display.c b/common/cmd_display.c
index 3422395..6851329 100644
--- a/common/cmd_display.c
+++ b/common/cmd_display.c
@@ -70,7 +70,7 @@ int do_display (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 /***************************************************/
 
 U_BOOT_CMD(
-	display,	CONFIG_SYS_MAXARGS,	1,	do_display,
+	display,	0,	1,	do_display,
 	"display string on dot matrix display",
 	"[<string>]\n"
 	"    - with <string> argument: display <string> on dot matrix display\n"
diff --git a/common/cmd_echo.c b/common/cmd_echo.c
index 3ec4d48..0f96efb 100644
--- a/common/cmd_echo.c
+++ b/common/cmd_echo.c
@@ -51,7 +51,7 @@ int do_echo(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 }
 
 U_BOOT_CMD(
-	echo,	CONFIG_SYS_MAXARGS,	1,	do_echo,
+	echo,	0,	1,	do_echo,
 	"echo args to console",
 	"[args..]\n"
 	"    - echo args to console; \\c suppresses newline"
diff --git a/common/cmd_help.c b/common/cmd_help.c
index e860dfb..c729f79 100644
--- a/common/cmd_help.c
+++ b/common/cmd_help.c
@@ -32,7 +32,7 @@ int do_help(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
 }
 
 U_BOOT_CMD(
-	help,	CONFIG_SYS_MAXARGS,	1,	do_help,
+	help,	0,	1,	do_help,
 	"print command description/usage",
 	"\n"
 	"	- print brief description of all commands\n"
@@ -42,7 +42,7 @@ U_BOOT_CMD(
 
 /* This does not use the U_BOOT_CMD macro as ? can't be used in symbol names */
 cmd_tbl_t __u_boot_cmd_question_mark Struct_Section = {
-	"?",	CONFIG_SYS_MAXARGS,	1,	do_help,
+	"?",	0,	1,	do_help,
 	"alias for 'help'",
 #ifdef  CONFIG_SYS_LONGHELP
 	""
diff --git a/common/cmd_mp.c b/common/cmd_mp.c
index d78c209..b8ddbf3 100644
--- a/common/cmd_mp.c
+++ b/common/cmd_mp.c
@@ -84,7 +84,7 @@ cpu_cmd(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 #endif
 
 U_BOOT_CMD(
-	cpu, CONFIG_SYS_MAXARGS, 1, cpu_cmd,
+	cpu, 0, 1, cpu_cmd,
 	"Multiprocessor CPU boot manipulation and release",
 	    "<num> reset                 - Reset cpu <num>\n"
 	"cpu <num> status                - Status of cpu <num>\n"
diff --git a/common/cmd_nand.c b/common/cmd_nand.c
index 075a8af..430c115 100644
--- a/common/cmd_nand.c
+++ b/common/cmd_nand.c
@@ -475,7 +475,7 @@ usage:
 	return 1;
 }
 
-U_BOOT_CMD(nand, CONFIG_SYS_MAXARGS, 1, do_nand,
+U_BOOT_CMD(nand, 0, 1, do_nand,
 	"NAND sub-system",
 	"info - show available NAND devices\n"
 	"nand device [dev] - show or set current device\n"
diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c
index eb89e9e..6020e85 100644
--- a/common/cmd_nvedit.c
+++ b/common/cmd_nvedit.c
@@ -638,7 +638,7 @@ U_BOOT_CMD(
 #endif
 
 U_BOOT_CMD(
-	printenv, CONFIG_SYS_MAXARGS, 1,	do_printenv,
+	printenv, 0, 1,	do_printenv,
 	"print environment variables",
 	"\n    - print values of all environment variables\n"
 	"printenv name ...\n"
@@ -646,7 +646,7 @@ U_BOOT_CMD(
 );
 
 U_BOOT_CMD(
-	setenv, CONFIG_SYS_MAXARGS, 0,	do_setenv,
+	setenv, 0, 0,	do_setenv,
 	"set environment variables",
 	"name value ...\n"
 	"    - set environment variable 'name' to 'value ...'\n"
@@ -657,7 +657,7 @@ U_BOOT_CMD(
 #if defined(CONFIG_CMD_ASKENV)
 
 U_BOOT_CMD(
-	askenv,	CONFIG_SYS_MAXARGS,	1,	do_askenv,
+	askenv,	0,	1,	do_askenv,
 	"get environment variables from stdin",
 	"name [message] [size]\n"
 	"    - get environment variable 'name' from stdin (max 'size' chars)\n"
@@ -674,7 +674,7 @@ U_BOOT_CMD(
 #if defined(CONFIG_CMD_RUN)
 int do_run (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]);
 U_BOOT_CMD(
-	run,	CONFIG_SYS_MAXARGS,	1,	do_run,
+	run,	0,	1,	do_run,
 	"run commands in an environment variable",
 	"var [...]\n"
 	"    - run the commands in the environment variable(s) 'var'"
diff --git a/common/cmd_onenand.c b/common/cmd_onenand.c
index 565257c..7701e21 100644
--- a/common/cmd_onenand.c
+++ b/common/cmd_onenand.c
@@ -481,7 +481,7 @@ usage:
 }
 
 U_BOOT_CMD(
-	onenand,	CONFIG_SYS_MAXARGS,	1,	do_onenand,
+	onenand,	0,	1,	do_onenand,
 	"OneNAND sub-system",
 	"info - show available OneNAND devices\n"
 	"onenand bad - show bad blocks\n"
diff --git a/common/cmd_test.c b/common/cmd_test.c
index d886f89..88ed2ba 100644
--- a/common/cmd_test.c
+++ b/common/cmd_test.c
@@ -145,7 +145,7 @@ int do_test(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 }
 
 U_BOOT_CMD(
-	test,	CONFIG_SYS_MAXARGS,	1,	do_test,
+	test,	0,	1,	do_test,
 	"minimal test like /bin/sh",
 	"[args..]"
 );
@@ -156,7 +156,7 @@ int do_false(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 }
 
 U_BOOT_CMD(
-	false,	CONFIG_SYS_MAXARGS,	1,	do_false,
+	false,	0,	1,	do_false,
 	"do nothing, unsuccessfully",
 	NULL
 );
@@ -167,7 +167,7 @@ int do_true(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 }
 
 U_BOOT_CMD(
-	true,	CONFIG_SYS_MAXARGS,	1,	do_true,
+	true,	0,	1,	do_true,
 	"do nothing, successfully",
 	NULL
 );
diff --git a/common/command.c b/common/command.c
index 0c66b7a..246ace0 100644
--- a/common/command.c
+++ b/common/command.c
@@ -27,6 +27,7 @@
 
 #include <common.h>
 #include <command.h>
+#include <malloc.h>
 
 /*
  * Use puts() instead of printf() to avoid printf buffer overflow
@@ -268,36 +269,6 @@ static int complete_cmdv(int argc, char *argv[], char last_char, int maxv, char
 	return n_found;
 }
 
-static int make_argv(char *s, int argvsz, char *argv[])
-{
-	int argc = 0;
-
-	/* split into argv */
-	while (argc < argvsz - 1) {
-
-		/* skip any white space */
-		while ((*s == ' ') || (*s == '\t'))
-			++s;
-
-		if (*s == '\0')	/* end of s, no more args	*/
-			break;
-
-		argv[argc++] = s;	/* begin of argument string	*/
-
-		/* find end of string */
-		while (*s && (*s != ' ') && (*s != '\t'))
-			++s;
-
-		if (*s == '\0')		/* end of s, no more args	*/
-			break;
-
-		*s++ = '\0';		/* terminate current arg	 */
-	}
-	argv[argc] = NULL;
-
-	return argc;
-}
-
 static void print_argv(const char *banner, const char *leader, const char *sep, int linemax, char *argv[])
 {
 	int ll = leader != NULL ? strlen(leader) : 0;
@@ -352,7 +323,7 @@ static char tmp_buf[CONFIG_SYS_CBSIZE];	/* copy of console I/O buffer	*/
 int cmd_auto_complete(const char *const prompt, char *buf, int *np, int *colp)
 {
 	int n = *np, col = *colp;
-	char *argv[CONFIG_SYS_MAXARGS + 1];		/* NULL terminated	*/
+	char **argv;		/* NULL terminated	*/
 	char *cmdv[20];
 	char *s, *t;
 	const char *sep;
@@ -373,7 +344,9 @@ int cmd_auto_complete(const char *const prompt, char *buf, int *np, int *colp)
 	strcpy(tmp_buf, buf);
 
 	/* separate into argv */
-	argc = make_argv(tmp_buf, sizeof(argv)/sizeof(argv[0]), argv);
+	argc = arg_count(tmp_buf);
+	argv = (char **)malloc(sizeof(char *) * argc);
+	parse_line(tmp_buf, argv, argc);
 
 	/* do the completion and return the possible completions */
 	i = complete_cmdv(argc, argv, last_char, sizeof(cmdv)/sizeof(cmdv[0]), cmdv);
diff --git a/common/hush.c b/common/hush.c
index 06c5ff8..1837a7f 100644
--- a/common/hush.c
+++ b/common/hush.c
@@ -1694,7 +1694,8 @@ static int run_pipe_real(struct pipe *pi)
 				}
 #endif
 				/* found - check max args */
-				if ((child->argc - i) > cmdtp->maxargs) {
+				if (cmdtp->maxargs &&
+				    (child->argc - i) > cmdtp->maxargs) {
 					cmd_usage(cmdtp);
 					return -1;
 				}
@@ -3627,7 +3628,7 @@ int do_showvar (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 }
 
 U_BOOT_CMD(
-	showvar, CONFIG_SYS_MAXARGS, 1,	do_showvar,
+	showvar, 0, 1,	do_showvar,
 	"print local hushshell variables",
 	"\n    - print values of all hushshell variables\n"
 	"showvar name ...\n"
diff --git a/common/kgdb.c b/common/kgdb.c
index 0531452..ac47c59 100644
--- a/common/kgdb.c
+++ b/common/kgdb.c
@@ -593,7 +593,7 @@ do_kgdb(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 }
 
 U_BOOT_CMD(
-	kgdb, CONFIG_SYS_MAXARGS, 1,	do_kgdb,
+	kgdb, 0, 1,	do_kgdb,
 	"enter gdb remote debug mode",
 	"[arg0 arg1 .. argN]\n"
 	"    - executes a breakpoint so that kgdb mode is\n"
diff --git a/common/main.c b/common/main.c
index 10d8904..64ec072 100644
--- a/common/main.c
+++ b/common/main.c
@@ -30,9 +30,7 @@
 #include <common.h>
 #include <watchdog.h>
 #include <command.h>
-#ifdef CONFIG_MODEM_SUPPORT
 #include <malloc.h>		/* for free() prototype */
-#endif
 
 #ifdef CONFIG_SYS_HUSH_PARSER
 #include <hush.h>
@@ -1112,15 +1110,35 @@ static char * delete_char (char *buffer, char *p, int *colp, int *np, int plen)
 }
 
 /****************************************************************************/
+int arg_count(char *line)
+{
+	int argc = 0;
+
+	while (*line != '\0') {
+
+		while ((*line == ' ') || (*line == '\t')) {
+			++line;
+		}
 
-int parse_line (char *line, char *argv[])
+		argc++;
+
+		/* find end of string */
+		while (*line && (*line != ' ') && (*line != '\t')) {
+			++line;
+		}
+	}
+
+	return argc;
+}
+
+int parse_line (char *line, char *argv[], uint argc)
 {
 	int nargs = 0;
 
 #ifdef DEBUG_PARSER
 	printf ("parse_line: \"%s\"\n", line);
 #endif
-	while (nargs < CONFIG_SYS_MAXARGS) {
+	while (nargs < argc) {
 
 		/* skip any white space */
 		while ((*line == ' ') || (*line == '\t')) {
@@ -1153,8 +1171,6 @@ int parse_line (char *line, char *argv[])
 		*line++ = '\0';		/* terminate current arg	 */
 	}
 
-	printf ("** Too many args (max. %d) **\n", CONFIG_SYS_MAXARGS);
-
 #ifdef DEBUG_PARSER
 	printf ("parse_line: nargs=%d\n", nargs);
 #endif
@@ -1298,8 +1314,9 @@ int run_command (const char *cmd, int flag)
 	char *sep;			/* end of token (separator) in cmdbuf */
 	char finaltoken[CONFIG_SYS_CBSIZE];
 	char *str = cmdbuf;
-	char *argv[CONFIG_SYS_MAXARGS + 1];	/* NULL terminated	*/
-	int argc, inquotes;
+	char **argv;
+	uint argc;
+	int inquotes;
 	int repeatable = 1;
 	int rc = 0;
 
@@ -1365,7 +1382,9 @@ int run_command (const char *cmd, int flag)
 		process_macros (token, finaltoken);
 
 		/* Extract arguments */
-		if ((argc = parse_line (finaltoken, argv)) == 0) {
+		argc = arg_count(finaltoken);
+		argv = (char **)malloc(sizeof(char *) * argc);
+		if (parse_line (finaltoken, argv, argc) == 0) {
 			rc = -1;	/* no command@all */
 			continue;
 		}
@@ -1378,7 +1397,7 @@ int run_command (const char *cmd, int flag)
 		}
 
 		/* found - check max args */
-		if (argc > cmdtp->maxargs) {
+		if (cmdtp->maxargs && argc > cmdtp->maxargs) {
 			cmd_usage(cmdtp);
 			rc = -1;
 			continue;
@@ -1407,6 +1426,8 @@ int run_command (const char *cmd, int flag)
 
 		repeatable &= cmdtp->repeatable;
 
+		free(argv);
+
 		/* Did the user stop this? */
 		if (had_ctrlc ())
 			return -1;	/* if stopped then not repeatable */
diff --git a/cpu/arm_cortexa8/mx51/clock.c b/cpu/arm_cortexa8/mx51/clock.c
index 38480ac..b594caf 100644
--- a/cpu/arm_cortexa8/mx51/clock.c
+++ b/cpu/arm_cortexa8/mx51/clock.c
@@ -288,7 +288,7 @@ int do_mx51_showclocks(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 /***************************************************/
 
 U_BOOT_CMD(
-	clockinfo,	CONFIG_SYS_MAXARGS,	1,	do_mx51_showclocks,
+	clockinfo,	0,	1,	do_mx51_showclocks,
 	"display mx51 clocks\n",
 	""
 );
diff --git a/cpu/mpc512x/diu.c b/cpu/mpc512x/diu.c
index a24f395..bbb8421 100644
--- a/cpu/mpc512x/diu.c
+++ b/cpu/mpc512x/diu.c
@@ -126,7 +126,7 @@ int mpc5121diu_init_show_bmp(cmd_tbl_t *cmdtp,
 }
 
 U_BOOT_CMD(
-	diufb, CONFIG_SYS_MAXARGS, 1, mpc5121diu_init_show_bmp,
+	diufb, 0, 1, mpc5121diu_init_show_bmp,
 	"Init or Display BMP file",
 	"init\n    - initialize DIU\n"
 	"addr\n    - display bmp at address 'addr'"
diff --git a/cpu/mpc512x/iim.c b/cpu/mpc512x/iim.c
index 8f2eb37..ae3de0b 100644
--- a/cpu/mpc512x/iim.c
+++ b/cpu/mpc512x/iim.c
@@ -374,7 +374,7 @@ int do_ads5121_fuse(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 }
 
 U_BOOT_CMD(
-	fuse, CONFIG_SYS_MAXARGS, 0, do_ads5121_fuse,
+	fuse, 0, 0, do_ads5121_fuse,
 	"   - Read, Sense, Override or Program Fuses\n",
 	"bank <n>		- sets active Fuse Bank to 0 or 1\n"
 	"			    no args shows current active bank\n"
diff --git a/include/command.h b/include/command.h
index 55caa6e..725df9f 100644
--- a/include/command.h
+++ b/include/command.h
@@ -45,7 +45,7 @@
 
 struct cmd_tbl_s {
 	char		*name;		/* Command Name			*/
-	int		maxargs;	/* maximum number of arguments	*/
+	unsigned int	maxargs;	/* maximum number of arguments	*/
 	int		repeatable;	/* autorepeat allowed?		*/
 					/* Implementation function	*/
 	int		(*cmd)(struct cmd_tbl_s *, int, int, char *[]);
diff --git a/include/common.h b/include/common.h
index a133e34..6c15c02 100644
--- a/include/common.h
+++ b/include/common.h
@@ -1,3 +1,4 @@
+
 /*
  * (C) Copyright 2000-2009
  * Wolfgang Denk, DENX Software Engineering, wd at denx.de.
@@ -226,7 +227,8 @@ void	main_loop	(void);
 int	run_command	(const char *cmd, int flag);
 int	readline	(const char *const prompt);
 int	readline_into_buffer	(const char *const prompt, char * buffer);
-int	parse_line (char *, char *[]);
+int	arg_count(char *);
+int	parse_line (char *, char *[], uint);
 void	init_cmd_timeout(void);
 void	reset_cmd_timeout(void);
 
-- 
1.6.0.4

[U-Boot] [RFC 2/2 v2] command: Remove maxargs from command structure

[John Schmoller]

Remove maxargs from the command structure and move bounds checking
to individual functions.

This is a subset of the changes created by the real patch, and is for
reviewing only, and not applying. The real patch is much too large for
the mailing list.

Signed-off-by: John Schmoller <jschmoller@xes-inc.com>
---
Since v1:
 - This patch is now a subset of the real patch. It is now small enough to
 be posted.

 common/cmd_eeprom.c |    7 ++---
 common/cmd_elf.c    |   18 ++++++++----
 common/cmd_i2c.c    |   75 +++++++++++++++++++--------------------------------
 common/hush.c       |   11 ++-----
 common/kgdb.c       |    2 +-
 common/lcd.c        |    5 +++-
 common/main.c       |   19 +++++--------
 include/command.h   |   22 +++++++++------
 8 files changed, 71 insertions(+), 88 deletions(-)

diff --git a/common/cmd_eeprom.c b/common/cmd_eeprom.c
index 519b510..b645f97 100644
--- a/common/cmd_eeprom.c
+++ b/common/cmd_eeprom.c
@@ -104,8 +104,7 @@ int do_eeprom ( cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
 		}
 	}
 
-	cmd_usage(cmdtp);
-	return 1;
+	return CMD_ERR_USAGE;
 }
 #endif
 
@@ -422,7 +421,7 @@ void eeprom_init  (void)
 
 #ifdef CONFIG_SYS_I2C_MULTI_EEPROMS
 U_BOOT_CMD(
-	eeprom,	6,	1,	do_eeprom,
+	eeprom,	1,	do_eeprom,
 	"EEPROM sub-system",
 	"read  devaddr addr off cnt\n"
 	"eeprom write devaddr addr off cnt\n"
@@ -430,7 +429,7 @@ U_BOOT_CMD(
 );
 #else /* One EEPROM */
 U_BOOT_CMD(
-	eeprom,	5,	1,	do_eeprom,
+	eeprom,	1,	do_eeprom,
 	"EEPROM sub-system",
 	"read  addr off cnt\n"
 	"eeprom write addr off cnt\n"
diff --git a/common/cmd_elf.c b/common/cmd_elf.c
index 63f6fe7..26fa346 100644
--- a/common/cmd_elf.c
+++ b/common/cmd_elf.c
@@ -64,10 +64,13 @@ int do_bootelf (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 	/* -------------------------------------------------- */
 	int rcode = 0;
 
-	if (argc < 2)
+	if (argc == 1) {
 		addr = load_addr;
-	else
+	} else if (argc == 2) {
 		addr = simple_strtoul (argv[1], NULL, 16);
+	} else {
+		return CMD_ERR_USAGE;
+	}
 
 	if (!valid_elf_image (addr))
 		return 1;
@@ -107,10 +110,13 @@ int do_bootvx (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 	 * If we don't know where the image is then we're done.
 	 */
 
-	if (argc < 2)
+	if (argc == 1) {
 		addr = load_addr;
-	else
+	} else if (argc == 2) {
 		addr = simple_strtoul (argv[1], NULL, 16);
+	} else {
+		return CMD_ERR_USAGE;
+	}
 
 #if defined(CONFIG_CMD_NET)
 	/* Check to see if we need to tftp the image ourselves before starting */
@@ -311,13 +317,13 @@ unsigned long load_elf_image (unsigned long addr)
 
 /* ====================================================================== */
 U_BOOT_CMD(
-	bootelf,      2,      0,      do_bootelf,
+	bootelf,      0,      do_bootelf,
 	"Boot from an ELF image in memory",
 	" [address] - load address of ELF image."
 );
 
 U_BOOT_CMD(
-	bootvx,      2,      0,      do_bootvx,
+	bootvx,      0,      do_bootvx,
 	"Boot vxWorks from an ELF image",
 	" [address] - load address of vxWorks ELF image."
 );
diff --git a/common/cmd_i2c.c b/common/cmd_i2c.c
index 62cbd33..d4d07e5 100644
--- a/common/cmd_i2c.c
+++ b/common/cmd_i2c.c
@@ -168,10 +168,8 @@ int do_i2c_md ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 	alen   = i2c_dp_last_alen;
 	length = i2c_dp_last_length;
 
-	if (argc < 3) {
-		cmd_usage(cmdtp);
-		return 1;
-	}
+	if (argc < 3)
+		return CMD_ERR_USAGE;
 
 	if ((flag & CMD_FLAG_REPEAT) == 0) {
 		/*
@@ -193,10 +191,8 @@ int do_i2c_md ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 		for (j = 0; j < 8; j++) {
 			if (argv[2][j] == '.') {
 				alen = argv[2][j+1] - '0';
-				if (alen > 4) {
-					cmd_usage(cmdtp);
-					return 1;
-				}
+				if (alen > 4)
+					return CMD_ERR_USAGE;
 				break;
 			} else if (argv[2][j] == '\0')
 				break;
@@ -269,10 +265,8 @@ int do_i2c_mw ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 	int	count;
 	int	j;
 
-	if ((argc < 4) || (argc > 5)) {
-		cmd_usage(cmdtp);
-		return 1;
-	}
+	if ((argc < 4) || (argc > 5))
+		return CMD_ERR_USAGE;
 
 	/*
 	 * Chip is always specified.
@@ -287,10 +281,8 @@ int do_i2c_mw ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 	for (j = 0; j < 8; j++) {
 		if (argv[2][j] == '.') {
 			alen = argv[2][j+1] - '0';
-			if (alen > 4) {
-				cmd_usage(cmdtp);
-				return 1;
-			}
+			if (alen > 4)
+				return CMD_ERR_USAGE;
 			break;
 		} else if (argv[2][j] == '\0')
 			break;
@@ -343,10 +335,8 @@ int do_i2c_crc (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 	ulong	err;
 	int	j;
 
-	if (argc < 4) {
-		cmd_usage(cmdtp);
-		return 1;
-	}
+	if (argc < 4)
+		return CMD_ERR_USAGE;
 
 	/*
 	 * Chip is always specified.
@@ -361,10 +351,8 @@ int do_i2c_crc (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 	for (j = 0; j < 8; j++) {
 		if (argv[2][j] == '.') {
 			alen = argv[2][j+1] - '0';
-			if (alen > 4) {
-				cmd_usage(cmdtp);
-				return 1;
-			}
+			if (alen > 4)
+				return CMD_ERR_USAGE;
 			break;
 		} else if (argv[2][j] == '\0')
 			break;
@@ -415,10 +403,8 @@ mod_i2c_mem(cmd_tbl_t *cmdtp, int incrflag, int flag, int argc, char *argv[])
 	int	j;
 	extern char console_buffer[];
 
-	if (argc != 3) {
-		cmd_usage(cmdtp);
-		return 1;
-	}
+	if (argc != 3)
+		return CMD_ERR_USAGE;
 
 #ifdef CONFIG_BOOT_RETRY_TIME
 	reset_cmd_timeout();	/* got a good command to get here */
@@ -451,10 +437,8 @@ mod_i2c_mem(cmd_tbl_t *cmdtp, int incrflag, int flag, int argc, char *argv[])
 		for (j = 0; j < 8; j++) {
 			if (argv[2][j] == '.') {
 				alen = argv[2][j+1] - '0';
-				if (alen > 4) {
-					cmd_usage(cmdtp);
-					return 1;
-				}
+				if (alen > 4)
+					return CMD_ERR_USAGE;
 				break;
 			} else if (argv[2][j] == '\0')
 				break;
@@ -589,10 +573,8 @@ int do_i2c_loop(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 	int	delay;
 	int	j;
 
-	if (argc < 3) {
-		cmd_usage(cmdtp);
-		return 1;
-	}
+	if (argc < 3)
+		return CMD_ERR_USAGE;
 
 	/*
 	 * Chip is always specified.
@@ -607,10 +589,8 @@ int do_i2c_loop(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 	for (j = 0; j < 8; j++) {
 		if (argv[2][j] == '.') {
 			alen = argv[2][j+1] - '0';
-			if (alen > 4) {
-				cmd_usage(cmdtp);
-				return 1;
-			}
+			if (alen > 4)
+				return CMD_ERR_USAGE;
 			break;
 		} else if (argv[2][j] == '\0')
 			break;
@@ -752,10 +732,9 @@ int do_sdram (cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
 		"32 MiB", "16 MiB", "8 MiB", "4 MiB"
 	};
 
-	if (argc < 2) {
-		cmd_usage(cmdtp);
-		return 1;
-	}
+	if (argc < 2)
+		return CMD_ERR_USAGE;
+
 	/*
 	 * Chip is always specified.
 	 */
@@ -1244,6 +1223,9 @@ int do_i2c_bus_speed(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
 
 int do_i2c(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
 {
+	if (argc > 6)
+		return CMD_ERR_USAGE;
+
 	/* Strip off leading 'i2c' command argument */
 	argc--;
 	argv++;
@@ -1280,14 +1262,13 @@ int do_i2c(cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
 	if (!strncmp(argv[0], "sd", 2))
 		return do_sdram(cmdtp, flag, argc, argv);
 #endif
-	cmd_usage(cmdtp);
-	return 0;
+	return CMD_ERR_USAGE;
 }
 
 /***************************************************/
 
 U_BOOT_CMD(
-	i2c, 6, 1, do_i2c,
+	i2c, 1, do_i2c,
 	"I2C sub-system",
 	"speed [speed] - show or set I2C bus speed\n"
 #if defined(CONFIG_I2C_MUX)
diff --git a/common/hush.c b/common/hush.c
index 1837a7f..682846e 100644
--- a/common/hush.c
+++ b/common/hush.c
@@ -1693,12 +1693,6 @@ static int run_pipe_real(struct pipe *pi)
 					flag |= CMD_FLAG_BOOTD;
 				}
 #endif
-				/* found - check max args */
-				if (cmdtp->maxargs &&
-				    (child->argc - i) > cmdtp->maxargs) {
-					cmd_usage(cmdtp);
-					return -1;
-				}
 #endif
 				child->argv+=i;  /* XXX horrible hack */
 #ifndef __U_BOOT__
@@ -1711,7 +1705,8 @@ static int run_pipe_real(struct pipe *pi)
 				if ( !cmdtp->repeatable )
 					flag_repeat = 0;
 
-
+				if (rcode == CMD_ERR_USAGE)
+					cmd_usage(cmdtp);
 #endif
 				child->argv-=i;  /* XXX restore hack so free() can work right */
 #ifndef __U_BOOT__
@@ -3628,7 +3623,7 @@ int do_showvar (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 }
 
 U_BOOT_CMD(
-	showvar, 0, 1,	do_showvar,
+	showvar, 1,	do_showvar,
 	"print local hushshell variables",
 	"\n    - print values of all hushshell variables\n"
 	"showvar name ...\n"
diff --git a/common/kgdb.c b/common/kgdb.c
index ac47c59..0152eec 100644
--- a/common/kgdb.c
+++ b/common/kgdb.c
@@ -593,7 +593,7 @@ do_kgdb(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 }
 
 U_BOOT_CMD(
-	kgdb, 0, 1,	do_kgdb,
+	kgdb, 1,	do_kgdb,
 	"enter gdb remote debug mode",
 	"[arg0 arg1 .. argN]\n"
 	"    - executes a breakpoint so that kgdb mode is\n"
diff --git a/common/lcd.c b/common/lcd.c
index db799db..e1643b5 100644
--- a/common/lcd.c
+++ b/common/lcd.c
@@ -348,6 +348,9 @@ int drv_lcd_init (void)
 /*----------------------------------------------------------------------*/
 static int lcd_clear (cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
 {
+	if (argc > 1)
+		return CMD_ERR_USAGE;
+
 #if LCD_BPP == LCD_MONOCHROME
 	/* Setting the palette */
 	lcd_initcolregs();
@@ -392,7 +395,7 @@ static int lcd_clear (cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
 }
 
 U_BOOT_CMD(
-	cls,	1,	1,	lcd_clear,
+	cls,	1,	lcd_clear,
 	"clear screen",
 	""
 );
diff --git a/common/main.c b/common/main.c
index 64ec072..340478c 100644
--- a/common/main.c
+++ b/common/main.c
@@ -1396,13 +1396,6 @@ int run_command (const char *cmd, int flag)
 			continue;
 		}
 
-		/* found - check max args */
-		if (cmdtp->maxargs && argc > cmdtp->maxargs) {
-			cmd_usage(cmdtp);
-			rc = -1;
-			continue;
-		}
-
 #if defined(CONFIG_CMD_BOOTD)
 		/* avoid "bootd" recursion */
 		if (cmdtp->cmd == do_bootd) {
@@ -1420,7 +1413,11 @@ int run_command (const char *cmd, int flag)
 #endif
 
 		/* OK - call function to do the command */
-		if ((cmdtp->cmd) (cmdtp, flag, argc, argv) != 0) {
+		rc = (cmdtp->cmd) (cmdtp, flag, argc, argv);
+		if (rc == CMD_ERR_USAGE)
+			cmd_usage(cmdtp);
+
+		if (rc != 0) {
 			rc = -1;
 		}
 
@@ -1443,10 +1440,8 @@ int do_run (cmd_tbl_t * cmdtp, int flag, int argc, char *argv[])
 {
 	int i;
 
-	if (argc < 2) {
-		cmd_usage(cmdtp);
-		return 1;
-	}
+	if (argc < 2)
+		return CMD_ERR_USAGE;
 
 	for (i=1; i<argc; ++i) {
 		char *arg;
diff --git a/include/command.h b/include/command.h
index 725df9f..833682e 100644
--- a/include/command.h
+++ b/include/command.h
@@ -45,7 +45,6 @@
 
 struct cmd_tbl_s {
 	char		*name;		/* Command Name			*/
-	unsigned int	maxargs;	/* maximum number of arguments	*/
 	int		repeatable;	/* autorepeat allowed?		*/
 					/* Implementation function	*/
 	int		(*cmd)(struct cmd_tbl_s *, int, int, char *[]);
@@ -105,23 +104,28 @@ extern int cmd_get_data_size(char* arg, int default_size);
 #define CMD_FLAG_REPEAT		0x0001	/* repeat last command		*/
 #define CMD_FLAG_BOOTD		0x0002	/* command is from bootd	*/
 
+/*
+ * Command Errors:
+ */
+#define CMD_ERR_USAGE		256
+
 #define Struct_Section  __attribute__ ((unused,section (".u_boot_cmd")))
 
 #ifdef  CONFIG_SYS_LONGHELP
 
-#define U_BOOT_CMD(name,maxargs,rep,cmd,usage,help) \
-cmd_tbl_t __u_boot_cmd_##name Struct_Section = {#name, maxargs, rep, cmd, usage, help}
+#define U_BOOT_CMD(name,rep,cmd,usage,help) \
+cmd_tbl_t __u_boot_cmd_##name Struct_Section = {#name, rep, cmd, usage, help}
 
-#define U_BOOT_CMD_MKENT(name,maxargs,rep,cmd,usage,help) \
-{#name, maxargs, rep, cmd, usage, help}
+#define U_BOOT_CMD_MKENT(name,rep,cmd,usage,help) \
+{#name, rep, cmd, usage, help}
 
 #else	/* no long help info */
 
-#define U_BOOT_CMD(name,maxargs,rep,cmd,usage,help) \
-cmd_tbl_t __u_boot_cmd_##name Struct_Section = {#name, maxargs, rep, cmd, usage}
+#define U_BOOT_CMD(name,rep,cmd,usage,help) \
+cmd_tbl_t __u_boot_cmd_##name Struct_Section = {#name, rep, cmd, usage}
 
-#define U_BOOT_CMD_MKENT(name,maxargs,rep,cmd,usage,help) \
-{#name, maxargs, rep, cmd, usage}
+#define U_BOOT_CMD_MKENT(name,rep,cmd,usage,help) \
+{#name, rep, cmd, usage}
 
 #endif	/* CONFIG_SYS_LONGHELP */
 
-- 
1.6.0.4

[U-Boot] [RFC 2/2 v2] command: Remove maxargs from command structure

[Kim Phillips]

On Fri, 12 Mar 2010 13:25:31 -0600
John Schmoller <jschmoller@xes-inc.com> wrote:

> +/*
> + * Command Errors:
> + */
> +#define CMD_ERR_USAGE		256
> +

can we just use something like -EINVAL instead of reinventing the error
codes wheel?

Thanks,

Kim

[U-Boot] [RFC 2/2 v2] command: Remove maxargs from command structure

[John Schmoller]

On Fri, 2010-03-12 at 14:58 -0600, Kim Phillips wrote:
> On Fri, 12 Mar 2010 13:25:31 -0600
> John Schmoller <jschmoller@xes-inc.com> wrote:
> 
> > +/*
> > + * Command Errors:
> > + */
> > +#define CMD_ERR_USAGE		256
> > +
> 
> can we just use something like -EINVAL instead of reinventing the error
> codes wheel?

The problem with a negative return value, at least as far as I can tell,
is that hush will print "exit not allowed from main input shell."
run_list_real() returns -2 if the called functions return is < -1, and
-2 means exit. If I'm misinterpreting something, let me know.  I don't
see any problem with using positive EINVAL, though, I didn't notice it
existed.

John

[U-Boot] [RFC 2/2 v2] command: Remove maxargs from command structure

[Kim Phillips]

On Fri, 12 Mar 2010 15:04:49 -0600
John Schmoller <jschmoller@xes-inc.com> wrote:

> On Fri, 2010-03-12 at 14:58 -0600, Kim Phillips wrote:
> > On Fri, 12 Mar 2010 13:25:31 -0600
> > John Schmoller <jschmoller@xes-inc.com> wrote:
> > 
> > > +/*
> > > + * Command Errors:
> > > + */
> > > +#define CMD_ERR_USAGE		256
> > > +
> > 
> > can we just use something like -EINVAL instead of reinventing the error
> > codes wheel?
> 
> The problem with a negative return value, at least as far as I can tell,
> is that hush will print "exit not allowed from main input shell."
> run_list_real() returns -2 if the called functions return is < -1, and
> -2 means exit. If I'm misinterpreting something, let me know.  I don't

maybe I'm missing something too, but it sounds like the
arbitrarily-chosen {-1, -2} error codes commit c26e454d introduced can
be renumerated, just not sure to what though (exit can't be 0?).

> see any problem with using positive EINVAL, though, I didn't notice it
> existed.

I don't see a problem with that either - my main concern here is to not
introduce a new CMD_ERR_ namespace unnecessarily.

Thanks,

Kim

[U-Boot] [RFC 0/2 v2] Remove CONFIG_SYS_MAXARGS

[Wolfgang Denk]

Dear John Schmoller,

In message <cover.1268416692.git.jschmoller@xes-inc.com> you wrote:
> The first patch removes CONFIG_SYS_MAXARGS, replacing the staticly defined
> array with a malloc'd array of the appropriate size. When a function has no
> upper argument limit (ie, was set to CONFIG_SYS_MAXARGS), it is now set to 0
> to indicate this fact.  Argument count is now unlimited, within reason and
> malloc buffer size.
> 
> The second patch removes cmdtp->maxargs and moves the checks to the individual
> command functions.  Since most functions do bounds checking anyway, it's a
> a fairly cheap task (sometimes free) to remove this bounds check in
> common/main.c.  In addition, it's more intuitive (in my opinion) if all bounds
> checking is done in only one place for each function.  The second patch also
> creates a CMD_ERR_USAGE return value, which prints usage when returned.  The
> overall effect of this patch is to reduce code size by an average of 200-250
> bytes and, I feel, make things a bit cleaner.
> 
> I'm looking for comments on these two patches as they are quite invasive, and
> will definitly cause problems for those people who maintain their own code
> out-of-tree.  They may also require an additional amount of testing.

Did you actually run MAKEALL after applying the patches?

I tried to get a feeling for the impact on the memory footprint, but
it doesn't work for me. I get tons of error messages like these:

...
cmd_bdinfo.c:389:1: error: macro "U_BOOT_CMD" passed 6 arguments, but takes just 5
cmd_bdinfo.c:385: warning: data definition has no type or storage class
cmd_bdinfo.c:385: warning: type defaults to 'int' in declaration of 'U_BOOT_CMD'
...
cmd_bootm.c:466:65: error: macro "U_BOOT_CMD_MKENT" passed 6 arguments, but takes just 5
cmd_bootm.c:466: error: 'U_BOOT_CMD_MKENT' undeclared here (not in a function)
cmd_bootm.c:467:67: error: macro "U_BOOT_CMD_MKENT" passed 6 arguments, but takes just 5
cmd_bootm.c:469:69: error: macro "U_BOOT_CMD_MKENT" passed 6 arguments, but takes just 5
cmd_bootm.c:474:72: error: macro "U_BOOT_CMD_MKENT" passed 6 arguments, but takes just 5
cmd_bootm.c:475:65: error: macro "U_BOOT_CMD_MKENT" passed 6 arguments, but takes just 5
cmd_bootm.c:476:66: error: macro "U_BOOT_CMD_MKENT" passed 6 arguments, but takes just 5
cmd_bootm.c:477:62: error: macro "U_BOOT_CMD_MKENT" passed 6 arguments, but takes just 5
cmd_bootm.c:1022:1: error: macro "U_BOOT_CMD" passed 6 arguments, but takes just 5
cmd_bootm.c:986: warning: data definition has no type or storage class
cmd_bootm.c:986: warning: type defaults to 'int' in declaration of 'U_BOOT_CMD'
cmd_bootm.c:1047:1: error: macro "U_BOOT_CMD" passed 6 arguments, but takes just 5
cmd_bootm.c:1043: warning: data definition has no type or storage class
cmd_bootm.c:1043: warning: type defaults to 'int' in declaration of 'U_BOOT_CMD'
cmd_bootm.c:1054:1: error: macro "U_BOOT_CMD" passed 6 arguments, but takes just 5
cmd_bootm.c:1050: warning: data definition has no type or storage class
cmd_bootm.c:1050: warning: type defaults to 'int' in declaration of 'U_BOOT_CMD'
cmd_bootm.c:1142:1: error: macro "U_BOOT_CMD" passed 6 arguments, but takes just 5
cmd_bootm.c:1135: warning: data definition has no type or storage class
cmd_bootm.c:1135: warning: type defaults to 'int' in declaration of 'U_BOOT_CMD'
cmd_bootm.c:1209:1: error: macro "U_BOOT_CMD" passed 6 arguments, but takes just 5
cmd_bootm.c:1203: warning: data definition has no type or storage class
cmd_bootm.c:1203: warning: type defaults to 'int' in declaration of 'U_BOOT_CMD'
cmd_boot.c:70:1: error: macro "U_BOOT_CMD" passed 6 arguments, but takes just 5
cmd_boot.c:65: warning: data definition has no type or storage class
cmd_boot.c:65: warning: type defaults to 'int' in declaration of 'U_BOOT_CMD'
cmd_boot.c:78:1: error: macro "U_BOOT_CMD" passed 6 arguments, but takes just 5
cmd_boot.c:74: warning: data definition has no type or storage class
cmd_boot.c:74: warning: type defaults to 'int' in declaration of 'U_BOOT_CMD'
...


I suggest you rebase your patches against current code, test that
they actually compile, and repost.

Thanks in advance.

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
You can't have everything... where would you put it?  - Steven Wright

[U-Boot] [RFC 0/2 v2] Remove CONFIG_SYS_MAXARGS

[John Schmoller]

On Fri, 2010-04-09 at 23:04 +0200, Wolfgang Denk wrote:
> Dear John Schmoller,
> 
> In message <cover.1268416692.git.jschmoller@xes-inc.com> you wrote:
> > The first patch removes CONFIG_SYS_MAXARGS, replacing the staticly defined
> > array with a malloc'd array of the appropriate size. When a function has no
> > upper argument limit (ie, was set to CONFIG_SYS_MAXARGS), it is now set to 0
> > to indicate this fact.  Argument count is now unlimited, within reason and
> > malloc buffer size.
> > 
> > The second patch removes cmdtp->maxargs and moves the checks to the individual
> > command functions.  Since most functions do bounds checking anyway, it's a
> > a fairly cheap task (sometimes free) to remove this bounds check in
> > common/main.c.  In addition, it's more intuitive (in my opinion) if all bounds
> > checking is done in only one place for each function.  The second patch also
> > creates a CMD_ERR_USAGE return value, which prints usage when returned.  The
> > overall effect of this patch is to reduce code size by an average of 200-250
> > bytes and, I feel, make things a bit cleaner.
> > 
> > I'm looking for comments on these two patches as they are quite invasive, and
> > will definitly cause problems for those people who maintain their own code
> > out-of-tree.  They may also require an additional amount of testing.
> 
> Did you actually run MAKEALL after applying the patches?
> 
> I tried to get a feeling for the impact on the memory footprint, but
> it doesn't work for me. I get tons of error messages like these:
> 

I did run MAKEALL on ppc and arm. So, either something must have changed
or you're seeing these errors on an arch I don't have a compiler set up
for.  Are you seeing these errors on all arches or just on a specific
arch?  I'll try to get to this as soon as I can.

John

[U-Boot] [RFC 0/2 v2] Remove CONFIG_SYS_MAXARGS

[Wolfgang Denk]

Dear John Schmoller,

In message <1270850432.4458.36.camel@johns> you wrote:
>
> > I tried to get a feeling for the impact on the memory footprint, but
> > it doesn't work for me. I get tons of error messages like these:
> 
> I did run MAKEALL on ppc and arm. So, either something must have changed
> or you're seeing these errors on an arch I don't have a compiler set up
> for.  Are you seeing these errors on all arches or just on a specific
> arch?  I'll try to get to this as soon as I can.

I was just running a selection of PPC boards.

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
You have the capacity to learn from  mistakes.  You'll  learn  a  lot
today.