Message ID | e7d9246677ecb9a8c774e9e5f8d31ae3fd53d487.1394807506.git.p.marczak@samsung.com |
---|---|
State | Superseded |
Delegated to: | Tom Rini |
Headers | show |
Dear Przemyslaw Marczak, In message <e7d9246677ecb9a8c774e9e5f8d31ae3fd53d487.1394807506.git.p.marczak@samsung.com> you wrote: > Changes: > - randomly generate partition uuid if any is undefined > - print info about set/unset/generated uuid > - update doc/README.gpt ... > + int ret = -1; > char *e, *s; > + char uuid_str[37]; Should we not rather use a #defined macro here instead of the magic number 37 ? > - printf("Environmental '%s' not set\n", str); > - return -1; /* env not set */ > + printf("%s ", str); > + gen_rand_uuid_str(uuid_str); > + setenv(s, uuid_str); > + > + e = getenv(s); > + if (e) { > + puts("set to random.\n"); Can we keep the "var not set" part, so the user understands why U-Boot assigns a random ID here? > + } else { > + printf("%s get from environment.\n", str); Make this debug() ? > + puts("Writing GPT:\n"); > + > + ret = gpt_default(blk_dev_desc, argv[4]); > + if (!ret) { > + puts("success!\n"); > + return CMD_RET_SUCCESS; > + } else { > + puts("error!\n"); > return CMD_RET_FAILURE; This code is too verbose - I suggest to turn all these puts() into debug(). > } else { > return CMD_RET_USAGE; > } Please invert the logic so you can bail out early and reduce the indentation level. Best regards, Wolfgang Denk
On 03/14/2014 05:16 PM, Wolfgang Denk wrote: > Dear Przemyslaw Marczak, > > In message <e7d9246677ecb9a8c774e9e5f8d31ae3fd53d487.1394807506.git.p.marczak@samsung.com> you wrote: >> Changes: >> - randomly generate partition uuid if any is undefined >> - print info about set/unset/generated uuid >> - update doc/README.gpt > ... >> + int ret = -1; >> char *e, *s; >> + char uuid_str[37]; > > Should we not rather use a #defined macro here instead of the magic > number 37 ? > Ok, then I create proper header for uuid: "include/uuid.h" >> - printf("Environmental '%s' not set\n", str); >> - return -1; /* env not set */ >> + printf("%s ", str); >> + gen_rand_uuid_str(uuid_str); >> + setenv(s, uuid_str); >> + >> + e = getenv(s); >> + if (e) { >> + puts("set to random.\n"); > > Can we keep the "var not set" part, so the user understands why U-Boot > assigns a random ID here? > Ok, I will add this info. >> + } else { >> + printf("%s get from environment.\n", str); > > Make this debug() ? > >> + puts("Writing GPT:\n"); >> + >> + ret = gpt_default(blk_dev_desc, argv[4]); >> + if (!ret) { >> + puts("success!\n"); >> + return CMD_RET_SUCCESS; >> + } else { >> + puts("error!\n"); >> return CMD_RET_FAILURE; > > This code is too verbose - I suggest to turn all these puts() into > debug(). > Ok. >> } else { >> return CMD_RET_USAGE; >> } > > Please invert the logic so you can bail out early and reduce the > indentation level. > Ok. > Best regards, > > Wolfgang Denk > This can be changed to debug(), but I leave print error/success info when command finish. Thanks
diff --git a/common/cmd_gpt.c b/common/cmd_gpt.c index 1f12e6d..31a3fe1 100644 --- a/common/cmd_gpt.c +++ b/common/cmd_gpt.c @@ -29,9 +29,11 @@ * * @return - zero on successful expand and env is set */ -static char extract_env(const char *str, char **env) +static int extract_env(const char *str, char **env) { + int ret = -1; char *e, *s; + char uuid_str[37]; if (!str || strlen(str) < 4) return -1; @@ -43,16 +45,28 @@ 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 ", str); + gen_rand_uuid_str(uuid_str); + setenv(s, uuid_str); + + e = getenv(s); + if (e) { + puts("set to random.\n"); + ret = 0; + } else { + puts("unset - can't get random UUID.\n"); + } + } else { + printf("%s get from environment.\n", str); + ret = 0; } + *env = e; - return 0; + free(s); } - return -1; + return ret; } /** @@ -299,8 +313,16 @@ 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:\n"); + + ret = gpt_default(blk_dev_desc, argv[4]); + if (!ret) { + puts("success!\n"); + return CMD_RET_SUCCESS; + } else { + puts("error!\n"); return CMD_RET_FAILURE; + } } else { return CMD_RET_USAGE; } diff --git a/doc/README.gpt b/doc/README.gpt index 5c133f3..9d0a8df 100644 --- a/doc/README.gpt +++ b/doc/README.gpt @@ -176,3 +176,4 @@ Please, pay attention at -l switch for parted. "uuid" program is recommended to generate UUID string. Moreover it can decode (-d switch) passed in UUID string. It can be used to generate partitions UUID passed to u-boot environment variables. +If any partition "uuid" no exists then it will be randomly generated. diff --git a/include/common.h b/include/common.h index 20e9ae6..665c98f 100644 --- a/include/common.h +++ b/include/common.h @@ -834,7 +834,8 @@ char * strmhz(char *buf, unsigned long hz); #if defined(CONFIG_RANDOM_MACADDR) || \ defined(CONFIG_BOOTP_RANDOM_DELAY) || \ defined(CONFIG_CMD_LINK_LOCAL) || \ - defined(CONFIG_RANDOM_UUID) + defined(CONFIG_RANDOM_UUID) || \ + defined(CONFIG_PARTITION_UUIDS) #define RAND_MAX -1U void srand(unsigned int seed); unsigned int rand(void); diff --git a/lib/Makefile b/lib/Makefile index fc7a24a..f4c06c6 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -66,6 +66,7 @@ obj-$(CONFIG_PARTITION_UUIDS) += rand.o obj-y += vsprintf.o obj-$(CONFIG_RANDOM_UUID) += uuid.o obj-$(CONFIG_RANDOM_UUID) += rand.o +obj-$(CONFIG_PARTITION_UUIDS) += rand.o obj-$(CONFIG_RANDOM_MACADDR) += rand.o obj-$(CONFIG_BOOTP_RANDOM_DELAY) += rand.o obj-$(CONFIG_CMD_LINK_LOCAL) += rand.o