| Message ID | 20210228234718.1208376-6-seanga2@gmail.com |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | cmd: Add support for command substitution | expand |
Am 1. März 2021 00:47:18 MEZ schrieb Sean Anderson <seanga2@gmail.com>: >This is fairly straightforward. This allows > part uuid mmc 0 foo >To be rewritten as > env set foo $(part uuid mmc 0) >or even (if the variable is not required to be environmental) > foo=$(part uuid mmc 0) Who needs this? Why? Where do you document it? Where are the tests? Best regards Heinrich > >Signed-off-by: Sean Anderson <seanga2@gmail.com> >--- > > cmd/part.c | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > >diff --git a/cmd/part.c b/cmd/part.c >index 3395c17b89..97e70d79ff 100644 >--- a/cmd/part.c >+++ b/cmd/part.c >@@ -19,9 +19,12 @@ > #include <config.h> > #include <command.h> > #include <env.h> >+#include <malloc.h> > #include <part.h> > #include <vsprintf.h> > >+DECLARE_GLOBAL_DATA_PTR; >+ > enum cmd_part_info { > CMD_PART_INFO_START = 0, > CMD_PART_INFO_SIZE, >@@ -43,12 +46,19 @@ static int do_part_uuid(int argc, char *const >argv[]) > if (part < 0) > return 1; > >- if (argc > 2) >+ if (argc > 2) { > env_set(argv[2], info.uuid); >- else >- printf("%s\n", info.uuid); >+ } else { >+ size_t result_size = sizeof(info.uuid) + 1; > >- return 0; >+ gd->cmd_result = malloc(result_size); >+ if (!gd->cmd_result) >+ return CMD_RET_FAILURE; >+ >+ snprintf(gd->cmd_result, result_size, "%s\n", info.uuid); >+ } >+ >+ return CMD_RET_SUCCESS; > } > > static int do_part_list(int argc, char *const argv[])
Am 1. März 2021 01:03:43 MEZ schrieb Heinrich Schuchardt <xypron.glpk@gmx.de>: >Am 1. März 2021 00:47:18 MEZ schrieb Sean Anderson <seanga2@gmail.com>: >>This is fairly straightforward. This allows >> part uuid mmc 0 foo >>To be rewritten as >> env set foo $(part uuid mmc 0) >>or even (if the variable is not required to be environmental) >> foo=$(part uuid mmc 0) > >Who needs this? Why? > >Where do you document it? > >Where are the tests? > >Best regards > >Heinrich > > >> >>Signed-off-by: Sean Anderson <seanga2@gmail.com> >>--- >> >> cmd/part.c | 18 ++++++++++++++---- >> 1 file changed, 14 insertions(+), 4 deletions(-) >> >>diff --git a/cmd/part.c b/cmd/part.c >>index 3395c17b89..97e70d79ff 100644 >>--- a/cmd/part.c >>+++ b/cmd/part.c >>@@ -19,9 +19,12 @@ >> #include <config.h> >> #include <command.h> >> #include <env.h> >>+#include <malloc.h> >> #include <part.h> >> #include <vsprintf.h> >> >>+DECLARE_GLOBAL_DATA_PTR; >>+ >> enum cmd_part_info { >> CMD_PART_INFO_START = 0, >> CMD_PART_INFO_SIZE, >>@@ -43,12 +46,19 @@ static int do_part_uuid(int argc, char *const >>argv[]) >> if (part < 0) >> return 1; >> >>- if (argc > 2) >>+ if (argc > 2) { >> env_set(argv[2], info.uuid); >>- else >>- printf("%s\n", info.uuid); >>+ } else { >>+ size_t result_size = sizeof(info.uuid) + 1; >> >>- return 0; >>+ gd->cmd_result = malloc(result_size); This is a memory leak. How about realloc? >>+ if (!gd->cmd_result) >>+ return CMD_RET_FAILURE; >>+ >>+ snprintf(gd->cmd_result, result_size, "%s\n", info.uuid); >>+ } >>+ >>+ return CMD_RET_SUCCESS; >> } >> >> static int do_part_list(int argc, char *const argv[])
On 2/28/21 7:07 PM, Heinrich Schuchardt wrote: > Am 1. März 2021 01:03:43 MEZ schrieb Heinrich Schuchardt <xypron.glpk@gmx.de>: >> Am 1. März 2021 00:47:18 MEZ schrieb Sean Anderson <seanga2@gmail.com>: >>> This is fairly straightforward. This allows >>> part uuid mmc 0 foo >>> To be rewritten as >>> env set foo $(part uuid mmc 0) >>> or even (if the variable is not required to be environmental) >>> foo=$(part uuid mmc 0) >> >> Who needs this? Why? This is a consistent (and familiar) syntax for the many commands which set environmental variables. >> >> Where do you document it? It's undocumented at the moment. Will be fixed in v2. >> >> Where are the tests? There's (a) test in patch 3. Though I forgot to add a test for this command. Will be done in v2. >> >> Best regards >> >> Heinrich >> >> >>> >>> Signed-off-by: Sean Anderson <seanga2@gmail.com> >>> --- >>> >>> cmd/part.c | 18 ++++++++++++++---- >>> 1 file changed, 14 insertions(+), 4 deletions(-) >>> >>> diff --git a/cmd/part.c b/cmd/part.c >>> index 3395c17b89..97e70d79ff 100644 >>> --- a/cmd/part.c >>> +++ b/cmd/part.c >>> @@ -19,9 +19,12 @@ >>> #include <config.h> >>> #include <command.h> >>> #include <env.h> >>> +#include <malloc.h> >>> #include <part.h> >>> #include <vsprintf.h> >>> >>> +DECLARE_GLOBAL_DATA_PTR; >>> + >>> enum cmd_part_info { >>> CMD_PART_INFO_START = 0, >>> CMD_PART_INFO_SIZE, >>> @@ -43,12 +46,19 @@ static int do_part_uuid(int argc, char *const >>> argv[]) >>> if (part < 0) >>> return 1; >>> >>> - if (argc > 2) >>> + if (argc > 2) { >>> env_set(argv[2], info.uuid); >>> - else >>> - printf("%s\n", info.uuid); >>> + } else { >>> + size_t result_size = sizeof(info.uuid) + 1; >>> >>> - return 0; >>> + gd->cmd_result = malloc(result_size); > > This is a memory leak. How about realloc? This is not a memory leak. See patch 2 for semantics. --Sean > > >>> + if (!gd->cmd_result) >>> + return CMD_RET_FAILURE; >>> + >>> + snprintf(gd->cmd_result, result_size, "%s\n", info.uuid); >>> + } >>> + >>> + return CMD_RET_SUCCESS; >>> } >>> >>> static int do_part_list(int argc, char *const argv[]) >
Hi Sean, On Sun, 28 Feb 2021 at 16:47, Sean Anderson <seanga2@gmail.com> wrote: > > This is fairly straightforward. This allows > part uuid mmc 0 foo > To be rewritten as > env set foo $(part uuid mmc 0) > or even (if the variable is not required to be environmental) > foo=$(part uuid mmc 0) > > Signed-off-by: Sean Anderson <seanga2@gmail.com> > --- > > cmd/part.c | 18 ++++++++++++++---- > 1 file changed, 14 insertions(+), 4 deletions(-) > > diff --git a/cmd/part.c b/cmd/part.c > index 3395c17b89..97e70d79ff 100644 > --- a/cmd/part.c > +++ b/cmd/part.c > @@ -19,9 +19,12 @@ > #include <config.h> > #include <command.h> > #include <env.h> > +#include <malloc.h> > #include <part.h> > #include <vsprintf.h> > > +DECLARE_GLOBAL_DATA_PTR; > + > enum cmd_part_info { > CMD_PART_INFO_START = 0, > CMD_PART_INFO_SIZE, > @@ -43,12 +46,19 @@ static int do_part_uuid(int argc, char *const argv[]) > if (part < 0) > return 1; > > - if (argc > 2) > + if (argc > 2) { > env_set(argv[2], info.uuid); > - else > - printf("%s\n", info.uuid); > + } else { > + size_t result_size = sizeof(info.uuid) + 1; > > - return 0; > + gd->cmd_result = malloc(result_size); > + if (!gd->cmd_result) > + return CMD_RET_FAILURE; > + > + snprintf(gd->cmd_result, result_size, "%s\n", info.uuid); > + } > + > + return CMD_RET_SUCCESS; I suppose I prefer 0 to CMD_RET_SUCCESS, since 0 is the universal success code in U-Boot, and using the enum obscures that a bit. But I don't feel strongly about it. Regards, Simon
diff --git a/cmd/part.c b/cmd/part.c index 3395c17b89..97e70d79ff 100644 --- a/cmd/part.c +++ b/cmd/part.c @@ -19,9 +19,12 @@ #include <config.h> #include <command.h> #include <env.h> +#include <malloc.h> #include <part.h> #include <vsprintf.h> +DECLARE_GLOBAL_DATA_PTR; + enum cmd_part_info { CMD_PART_INFO_START = 0, CMD_PART_INFO_SIZE, @@ -43,12 +46,19 @@ static int do_part_uuid(int argc, char *const argv[]) if (part < 0) return 1; - if (argc > 2) + if (argc > 2) { env_set(argv[2], info.uuid); - else - printf("%s\n", info.uuid); + } else { + size_t result_size = sizeof(info.uuid) + 1; - return 0; + gd->cmd_result = malloc(result_size); + if (!gd->cmd_result) + return CMD_RET_FAILURE; + + snprintf(gd->cmd_result, result_size, "%s\n", info.uuid); + } + + return CMD_RET_SUCCESS; } static int do_part_list(int argc, char *const argv[])
This is fairly straightforward. This allows part uuid mmc 0 foo To be rewritten as env set foo $(part uuid mmc 0) or even (if the variable is not required to be environmental) foo=$(part uuid mmc 0) Signed-off-by: Sean Anderson <seanga2@gmail.com> --- cmd/part.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)