From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Thu, 13 Mar 2014 21:13:28 +0100 Subject: [U-Boot] [PATCH V2 3/3] cmd:gpt: randomly generate each partition uuid if undefined In-Reply-To: <53220BC2.4060504@wwwdotorg.org> References: <531DF9F9.2080309@wwwdotorg.org> <5321EAB7.6050206@samsung.com> <53220BC2.4060504@wwwdotorg.org> Message-ID: <53221168.5020305@samsung.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 03/13/2014 08:49 PM, Stephen Warren wrote: > On 03/13/2014 11:28 AM, Przemyslaw Marczak wrote: >> On 03/10/2014 06:44 PM, Stephen Warren wrote: >>> On 03/05/2014 09:45 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 > >>>> @@ -43,16 +45,25 @@ 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); >>>> + gen_rand_uuid_str(uuid_str); >>>> + setenv(s, uuid_str); >>>> + >> >> In this place ret is "-1". >> >>>> + e = getenv(s); >>>> + if (e) { >>>> + puts("Setting to random.\n"); >>> >>> Shouldn't this be printed right after the "if (e == NULL)" check above? >>> That's where the decision is made to generate a random UUID. >>> >>> Here, "if (!e)", the code should return an error. >>> >> If (!e) then ret is still "-1". >> If (e) then ret = 0 and proper info is printed. > > But ret has nothing to do with it; there's no code in this function > which prints messges based on ret. > > The code should be: > > e = getenv(s); > if (e == NULL) { > printf("Env var '%s' not set; using random UUID\n", str); > // generate random UUID here > if (UUID generation failed) { > printf("ERROR generating random UUID\n"); > return -1; > } Take a note that gen_rand_uuid_str() doesn't return any error. So this error is not true. > } > > With the code in your patch, the following happens: > > * Env var is set: Nothing printed. And env var is not changed - and this is as before. > > * Env var not set, but problem during UUID generation: "%s unset. " is > printed without a \n at the end. The error does get propagated back tot > he caller, which might print a message with \n at the end, but you > shouldn't place that kind of requirement on the caller. Rather, this > function should be fully self-contained. > > * Env var set, and UUID generated OK: "%s unset. Setting to random.\n" > is printed. > >>> But, I still don't like changing the environment. Why can't the above >>> few lines be: >>> >>> + gen_rand_uuid_str(uuid_str); >>> + e = uuid_str; >> >> Such solution needs more code rewriting and breaking some existing >> cmd_gpt design. "e" is used outside this function but uuid_str is local >> here. I don't like to make it static. >> Using getenv and return its pointer will work the same as previous. >> >> Please note that variables set by user are not overwritten here so this >> code will only set null uuid env variables. Moreover user can see after >> gpt command that environment is the same with mmc part shows, I think it >> is useful instead of situation when uuid is set but not present in >> environment. > > I don't think convenience of coding or the size of the patch is > justification for writing values to the user's environment when they > didn't ask for it. What if they run saveenv after executing this > function, yet they specifically want the environment variables unset, so > that a random UUID is generated each time this function/command is run? > Actually I don't understand what is the problem. What is the difference when user set manually $uuid_gpt_* or use generated by gpt command if next he write "save", in both situations variables are saved. I don't think it is a problem. Thanks -- Przemyslaw Marczak Samsung R&D Institute Poland Samsung Electronics p.marczak at samsung.com