| Message ID | 20121105195111.8a326673862d0552354e2815@freescale.com |
|---|---|
| State | Rejected |
| Delegated to: | Anatolij Gustschin |
| Headers | show |
mån 2012-11-05 klockan 19:51 -0600 skrev Kim Phillips: > commit 199adb601ff34bdbbd0667fac80dfe0a87bffc2b "common/misc: sparse > fixes" broke the help command trying to fix the sparse error > "command.c:44:38: error: bad constant expression". > > As Henrik points out, the fix was bad because the commit used > CONFIG_SYS_MAXARGS whereas the code intended to use the maximum > number of commands (not arguments to a command). > > this patch fixes both by making the allocation manually on the heap. > > Reported-by: Henrik Nordström <henrik@henriknordstrom.net> > Signed-off-by: Kim Phillips <kim.phillips@freescale.com> Tested-by: Henrik Nordström <henrik@henriknordstrom.net>
On Mon, Nov 5, 2012 at 9:44 PM, Henrik Nordström <henrik@henriknordstrom.net> wrote: > mån 2012-11-05 klockan 19:51 -0600 skrev Kim Phillips: >> commit 199adb601ff34bdbbd0667fac80dfe0a87bffc2b "common/misc: sparse >> fixes" broke the help command trying to fix the sparse error >> "command.c:44:38: error: bad constant expression". >> >> As Henrik points out, the fix was bad because the commit used >> CONFIG_SYS_MAXARGS whereas the code intended to use the maximum >> number of commands (not arguments to a command). >> >> this patch fixes both by making the allocation manually on the heap. >> >> Reported-by: Henrik Nordström <henrik@henriknordstrom.net> >> Signed-off-by: Kim Phillips <kim.phillips@freescale.com> Tested-by: Simon Glass <sjg@chromium.org> Acked-by: Simon Glass <sjg@chromium.org> > > Tested-by: Henrik Nordström <henrik@henriknordstrom.net> > > > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot
Hi, On Mon, 5 Nov 2012 19:51:11 -0600 Kim Phillips <kim.phillips@freescale.com> wrote: > commit 199adb601ff34bdbbd0667fac80dfe0a87bffc2b "common/misc: sparse > fixes" broke the help command trying to fix the sparse error > "command.c:44:38: error: bad constant expression". > > As Henrik points out, the fix was bad because the commit used > CONFIG_SYS_MAXARGS whereas the code intended to use the maximum > number of commands (not arguments to a command). > > this patch fixes both by making the allocation manually on the heap. > > Reported-by: Henrik Nordström <henrik@henriknordstrom.net> > Signed-off-by: Kim Phillips <kim.phillips@freescale.com> > --- > tested on an 8572ds board > > common/command.c | 21 ++++++++++++--------- > 1 file changed, 12 insertions(+), 9 deletions(-) Applied to staging/agust@denx.de. Thanks! Anatolij
Dear Kim Phillips, In message <20121105195111.8a326673862d0552354e2815@freescale.com> you wrote: > commit 199adb601ff34bdbbd0667fac80dfe0a87bffc2b "common/misc: sparse > fixes" broke the help command trying to fix the sparse error > "command.c:44:38: error: bad constant expression". > > As Henrik points out, the fix was bad because the commit used > CONFIG_SYS_MAXARGS whereas the code intended to use the maximum > number of commands (not arguments to a command). > > this patch fixes both by making the allocation manually on the heap. > > Reported-by: Henrik Nordstr=F6m <henrik@henriknordstrom.net> > Signed-off-by: Kim Phillips <kim.phillips@freescale.com> > --- > tested on an 8572ds board > > common/command.c | 21 ++++++++++++--------- > 1 file changed, 12 insertions(+), 9 deletions(-) Thanks for the fix, and Anatolij, thanks for picking it up, but I actually think the whole approach is broken,a nd we whould just revert the original patch. Theer is nothing wrong with the original code. It was technically correct, and only sparse had a different opinion. In this case we should find ways to silence sparse, or configure it in a way to allow us using correct and efficient code. What we have is not only slower, but also increases the memory footprint with zero benefit. I object against this, and ask to revert the original modification instead. Best regards, Wolfgang Denk
On Wed, 7 Nov 2012 23:23:50 +0100
Anatolij Gustschin <agust@denx.de> wrote:
...
> Applied to staging/agust@denx.de. Thanks!
I'll drop this patch as requested by Wolfgang.
Thanks,
Anatolij
Dear Anatolij, In message <20121108120224.3b377929@wker> you wrote: > On Wed, 7 Nov 2012 23:23:50 +0100 > Anatolij Gustschin <agust@denx.de> wrote: > ... > > Applied to staging/agust@denx.de. Thanks! > > I'll drop this patch as requested by Wolfgang. But we have also to revert the other commit, to make "help" working again. This is actually urgent, as the breakage affects current mainline. Best regards, Wolfgang Denk
diff --git a/common/command.c b/common/command.c index f51df26..991a7ee 100644 --- a/common/command.c +++ b/common/command.c @@ -28,6 +28,7 @@ #include <common.h> #include <command.h> #include <linux/ctype.h> +#include <malloc.h> /* * Use puts() instead of printf() to avoid printf buffer overflow @@ -40,17 +41,16 @@ int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int int i; int rcode = 0; - if (cmd_items > CONFIG_SYS_MAXARGS) { - printf("%s: cmd_items %d exceeds hardcoded limit %d." - " Recompile with higher CONFIG_SYS_MAXARGS?\n", - __func__, cmd_items, CONFIG_SYS_MAXARGS); - return -1; - } - if (argc == 1) { /*show list of commands */ - cmd_tbl_t *cmd_array[CONFIG_SYS_MAXARGS]; + cmd_tbl_t **cmd_array; int i, j, swaps; + cmd_array = malloc(cmd_items * sizeof(*cmd_array)); + if (!cmd_array) { + printf("error: not enough memory\n"); + return 1; + } + /* Make array of commands from .uboot_cmd section */ cmdtp = cmd_start; for (i = 0; i < cmd_items; i++) { @@ -79,13 +79,16 @@ int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int const char *usage = cmd_array[i]->usage; /* allow user abort */ - if (ctrlc ()) + if (ctrlc()) { + free(cmd_array); return 1; + } if (usage == NULL) continue; printf("%-*s- %s\n", CONFIG_SYS_HELP_CMD_WIDTH, cmd_array[i]->name, usage); } + free(cmd_array); return 0; } /*
commit 199adb601ff34bdbbd0667fac80dfe0a87bffc2b "common/misc: sparse fixes" broke the help command trying to fix the sparse error "command.c:44:38: error: bad constant expression". As Henrik points out, the fix was bad because the commit used CONFIG_SYS_MAXARGS whereas the code intended to use the maximum number of commands (not arguments to a command). this patch fixes both by making the allocation manually on the heap. Reported-by: Henrik Nordström <henrik@henriknordstrom.net> Signed-off-by: Kim Phillips <kim.phillips@freescale.com> --- tested on an 8572ds board common/command.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)