[U-Boot] [PATCH 2/2] cmd:gpt: randomly generate each partition uuid if undefined

[Stephen Warren]

On 02/28/2014 08:18 AM, Przemyslaw Marczak wrote:
> Changes:
> - randomly generate each partition uuid if undefined
> - print info about generated uuid
> - save environment on gpt write success
> - update doc/README.gpt

> diff --git a/common/cmd_gpt.c b/common/cmd_gpt.c

>  static char extract_env(const char *str, char **env)
>  {
> +	int ret = -1;

Why does the function return char not int? At least the type of ret
should match the return type of the function.

There's no need to introduce a ret variable anyway; just don't delete
the return statements that are already in the function.

> @@ -43,16 +44,23 @@ static char extract_env(const char *str, char **env)
>  		memset(s + strlen(s) - 1, '\0', 1);
>  		memmove(s, s + 2, strlen(s) - 1);
>  		e = getenv(s);
> -		free(s);
>  		if (e == NULL) {
> -			printf("Environmental '%s' not set\n", str);
> -			return -1; /* env not set */
> +			printf("%s unset. ", str);
> +			e = get_uuid_str();
> +			if (e) {
> +				printf("Setting to: %s\n", e);
> +				setenv(s, e);

Why should the environment variable be set? I rather dislike commands
that randomly set environment variables as an implicit side-effect.

It'd be far better if this function simply wasn't modified, but rather
the user was provided with a function to explicitly set an environment
variable to a randomly generated GPT. That way the user/script would be
in control. Something like:

$ gen_random_uuid env_var_name

> @@ -299,8 +307,17 @@ static int do_gpt(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  			return CMD_RET_FAILURE;
>  		}
>  
> -		if (gpt_default(blk_dev_desc, argv[4]))
> +		puts("Writing GPT: ");
> +
> +		ret = gpt_default(blk_dev_desc, argv[4]);
> +		if (!ret) {
> +			puts("success!\n");
> +			saveenv();

Uggh. Definitely don't save the environment behind the user's back.
There is no reason to believe that's safe. What if the user had added
some temporary changes to their environment that they didn't want saved?
This kind of logic belongs in scripts, not code.