Hello again,
On 02/28/2014 06:03 PM, Stephen Warren wrote:
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.
Right notice, this should be fixed.
There's no need to introduce a ret variable anyway; just don't delete
the return statements that are already in the function.
But I need to move "free(s)" so I can't leave current return statements.
Other way I need to put "free(s)" in few places.
@@ -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);
And here I forget about free(e)...
Why should the environment variable be set? I rather dislike commands
that randomly set environment variables as an implicit side-effect.
Actually automatically generated uuids was the main purpose of this
patches. Setting each env variable in this place was the most easy way
to make this without a lot of duplicated code.
Why do you treat it like a side-effect?
If user wants have own generated uuids - then he can manually set env
variables like "uuid_gpt_disk".
This actually is not changed - when uuid env is set then it will be used
like in current version of code.
When user can't generate uuids or just wants to have it automatically
generated then my code do this job.
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
I understand that this is very important code, but setting each val
manually with random uuid actually will not change anything - user still
needs to do something.
The other way is to provide a function for parse e.g $partitions but
then it will be a code duplication. The main job is done by
set_gpt_info() so this is why I modified this code.
@@ -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.
The one and only reason for put saveenv() here was that if uuids are
randomly generated or even just are in environment then I can be sure
that next gpt write (e.g. in case of overwrite sector 0 by mistake) is
using the same uuids values.
Maybe saveenv() in this place is not the best idea but in other side
user actually uses this command just once.
Thank you
--
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marc...@samsung.com
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
