diff mbox series

[5/5] cmd: Convert part uuid to use cmd_result

Message ID 20210228234718.1208376-6-seanga2@gmail.com
State New
Delegated to: Tom Rini
Headers show
Series cmd: Add support for command substitution | expand

Commit Message

Sean Anderson Feb. 28, 2021, 11:47 p.m. UTC
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(-)

Comments

Heinrich Schuchardt March 1, 2021, 12:03 a.m. UTC | #1
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[])
Heinrich Schuchardt March 1, 2021, 12:07 a.m. UTC | #2
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[])
Sean Anderson March 1, 2021, 12:12 a.m. UTC | #3
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[])
>
Simon Glass March 3, 2021, 1:53 a.m. UTC | #4
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 mbox series

Patch

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[])