Remove CONFIG_SYS_MAXARGS
v2 cover Remove CONFIG_SYS_MAXARGS
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(-)
RFC v2 1/2 cmd: Remove CONFIG_SYS_MAXARGS
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.
---
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(-)
board/amcc/makalu/cmd_pll.c +1 -1
@@ -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\
board/esd/du440/du440.c +2 -2
@@ -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" ,"" );
board/fads/fads.h +0 -1
@@ -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
board/freescale/common/pixis.c +2 -2
@@ -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"
board/freescale/mpc8610hpcd/mpc8610hpcd_diu.c +1 -1
@@ -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'"
board/pxa255_idp/pxa_idp.c +1 -1
@@ -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" );
common/cmd_boot.c +1 -1
@@ -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"
common/cmd_bootm.c +2 -2
@@ -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"
common/cmd_diag.c +1 -1
@@ -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"
common/cmd_display.c +1 -1
@@ -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"
common/cmd_echo.c +1 -1
@@ -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"
common/cmd_help.c +2 -2
@@ -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 ""
common/cmd_mp.c +1 -1
@@ -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"
common/cmd_nand.c +1 -1
@@ -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"
common/cmd_nvedit.c +4 -4
@@ -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'"
common/cmd_onenand.c +1 -1
@@ -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"
common/cmd_test.c +3 -3
@@ -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 );
common/command.c +5 -32
@@ -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, charreturn 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);
common/hush.c +3 -2
@@ -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"
common/kgdb.c +1 -1
@@ -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"
common/main.c +31 -10
91 lines not on this page · raw message
cpu/arm_cortexa8/mx51/clock.c +1 -1
@@ -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" ,"" );
cpu/mpc512x/diu.c +1 -1
@@ -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'"
cpu/mpc512x/iim.c +1 -1
@@ -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"
include/command.h +1 -1
@@ -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 *[]);
include/common.h +3 -1
@@ -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 );
Signature · 1 line
1.6.0.4
RFC v2 2/2 command: Remove maxargs from command structure
In reply to John SchmollerRemove 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.
---
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(-)
common/cmd_eeprom.c +3 -4
@@ -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"
common/cmd_elf.c +12 -6
@@ -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." );
common/cmd_i2c.c +28 -47
158 lines not on this page · raw message
common/hush.c +3 -8
@@ -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"
common/kgdb.c +1 -1
@@ -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"
common/lcd.c +4 -1
@@ -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" ,"" );
common/main.c +7 -12
@@ -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;
include/command.h +13 -9
@@ -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 */
Signature · 1 line
1.6.0.4
RFC v2 2/2 command: Remove maxargs from command structure
include/command.h extern int cmd_get_data_size(char* arg, int default_size); · line 111
Quoted 5 lines · On Fri, 12 Mar 2010 13:25:31 -0600 John Schmoller <[email protected]>…
include/command.h +5
-0+/* +* Command Errors: +*/ +#define CMD_ERR_USAGE 256 +
can we just use something like -EINVAL instead of reinventing the error codes wheel?
Thanks,
Kim
RFC v2 2/2 command: Remove maxargs from command structure
Quoted 11 lines · Kim Phillips
include/command.h extern int cmd_get_data_size(char* arg, int default_size); · line 111
Quoted 5 lines · On Fri, 12 Mar 2010 13:25:31 -0600 John Schmoller <[email protected]>…
include/command.h +5
-0+/* +* Command Errors: +*/ +#define CMD_ERR_USAGE 256 +can we just use something like
-EINVALinstead 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
RFC v2 2/2 command: Remove maxargs from command structure
Quoted 13 lines · On Fri, 12 Mar 2010 15:04:49 -0600 John Schmoller <[email protected]>…
Quoted 11 lines · Kim Phillips
include/command.h extern int cmd_get_data_size(char* arg, int default_size); · line 111
Quoted 5 lines · On Fri, 12 Mar 2010 13:25:31 -0600 John Schmoller <[email protected]>…
include/command.h +5
-0+/* +* Command Errors: +*/ +#define CMD_ERR_USAGE 256 +can we just use something like
-EINVALinstead of reinventing the error codes wheel?
Quoted 4 lines
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?).
Quoted 2 lines
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
v2 cover Remove CONFIG_SYS_MAXARGS
In reply to John SchmollerDear John Schmoller,
Quoted 15 lines · In message <[email protected]> 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 toCONFIG_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->maxargsand 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 incommon/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 aCMD_ERR_USAGEreturn 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.
Quoted 3 lines
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
Signature · 4 lines
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
v2 cover Remove CONFIG_SYS_MAXARGS
Quoted 18 lines · Wolfgang Denk
Dear John Schmoller,
Quoted 15 lines · In message <[email protected]> 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 toCONFIG_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->maxargsand 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 incommon/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 aCMD_ERR_USAGEreturn 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.
Quoted 9 lines
Quoted 3 lines
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
v2 cover Remove CONFIG_SYS_MAXARGS
Dear John Schmoller,
Quoted 8 lines · In message <1270850432.4458.36.camel@johns> you wrote:
Quoted 3 lines
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
Signature · 5 lines
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.