On 11/09/2012 03:48 AM, Piotr Wilczek wrote:
New command - "gpt" is supported. It restores the GPT partition table. It looks into the "partitions" environment variable for partitions definition. It can be enabled at target configuration file with CONFIG_CMD_GPT. Simple UUID generator has been implemented. It uses the the gd->start_addr_sp for entrophy pool. Moreover the pool address is used as crc32 seed.
diff --git a/common/cmd_gpt.c b/common/cmd_gpt.c
+U_BOOT_CMD(gpt, CONFIG_SYS_MAXARGS, 1, do_gpt,
- "GUID Partition Table",
- "<interface> <dev> <partions list>\n"
- " partions list is in format: name=..,size=..,uuid=..;...\n"
- " and can be passed as env or string ex.:\n"
- " gpt mmc 0 partitions\n"
I don't think that form makes sense. The user should just pass "${partitions}" instead. The command can't know for certain whether the user actually intended to pass the text "partitions" and made a mistake, or whether they passed an environment variable. If you really want to be able to pass an environment variable name, an explicit command-line option such as:
gpt mmc 0 name=... # definition on cmd-line gpt mmc 0 --from-environment partitions # definition in environment
seems best.
- " gpt mmc 0 "name=..,size=..;name=..,size=..;..."\n"
- " gpt mmc 0 "name=${part1_name},size=..;name=..,size=..;..."\n"
- " - GUID partition table restoration\n"
- " Restore GPT information on a device connected\n"
- " to interface\n"
Is writing a GPT to a device the only thing the gpt command will ever do. It seems best to require the user to write "gpt write mmc 0 ..." from the very start, so that if e.g. "gpt fix-crcs" or "gpt interactive-edit" or "gpt delete-partition 5" are implemented in the future, existing scripts won't have to change to add the "write" parameter.
+/**
- extract_env(): Convert string from '&{env_name}' to 'env_name'
s/&/$/
It's doing more than that; it locates that syntax within an arbitrary string and ignores anything before "${" or after "}". Is that intentional?
+static int extract_env(char *p)
- p1 = strstr(p, "${");
- p2 = strstr(p, "}");
- if (p1 && p2) {
*p2 = '\0';memmove(p, p+2, p2-p1-1);
s/-1/-2/ I think, since the length of "${" is 2 not 1.
Spaces around operators? s/p+2/p + 2/ for example.
+/**
- extract_val(): Extract value from a key=value pair
- @param p - pointer to string
Pointer to pointer to string, given its type?
- @param tab - table to store extracted value
- @param i - actual tab element to work on
Table? Why not just pass in char **tab and get rid of "i".
+static int extract_val(char **p, char *tab[], int i, char *key) +{
- char *t, *e, *tok = *p;
- char *k;
Those variable names are not exactly descriptive.
- t = strsep(&tok, ",");
- k = t;
- strsep(&t, "=");
- if (key && strcmp(k, key))
return -2;- if (extract_env(t) == 0) {
Hmm. That only allows key=${value}. What about key=text${envothertext or key=${env1}foo${env2}? Isn't there some generic code that can already expand environment variables within strings so we don't have to re-invent it here?
- tab[i] = calloc(strlen(t) + 1, 1);
- if (tab[i] == NULL) {
printf("%s: calloc failed!\n", __func__);return -1;- }
- strcpy(tab[i], t);
Isn't strdup() available?
+static int set_gpt_info(block_dev_desc_t *dev_desc, char *str_part,
- disk_partition_t *partitions[], const int parts_count)
+{
- char *ps[parts_count];
Can we call this sizes? Can't we call strtoul() and store int sizes[] rather than storing the strings first and then converting to integers in a separate piece of disconnected code?
- printf("PARTITIONS: %s\n", s);
Why print that?
- ss = calloc(strlen(s) + 1, 1);
- if (ss == NULL) {
printf("%s: calloc failed!\n", __func__);return -1;- }
- memcpy(ss, s, strlen(s) + 1);
Use strdup().
That duplicates the strdup() in do_gpt() some of the time.
- for (i = 0, p = ss; i < parts_count; i++) {
Why not calculate parts_count here, rather than splitting the parsing logic between this function and gpt_mmc_default()?
tok = strsep(&p, ";");if (tok == NULL)break;if (extract_val(&tok, name, i, "name")) {ret = -1;goto err;}if (extract_val(&tok, ps, i, "size")) {ret = -1;free(name[i]);goto err;}
I think that requires the parameters to be passed in order name=foo,size=5,uuid=xxx. That seems inflexible. The syntax may as well just be value,value,value rather than key=value,key=value,key=value in that case (although the keys are useful in order to understand the data, so I'd prefer parsing flexibility rather than removing key=).
if (extract_val(&tok, uuid, i, "uuid")) {/* uuid string length equals 37 */uuid[i] = calloc(37, 1);
Shouldn't storage for the UUID always be allocated? After all, one must always be written even if the user didn't explicitly specify one, so U-Boot makes it up.
p = ps[i];size[i] = ustrtoul(p, &p, 0);size[i] /= dev_desc->blksz;
What if the size isn't rounded correctly?
- for (i = 0; i < parts_count; i++) {
partitions[i]->size = size[i];partitions[i]->blksz = dev_desc->blksz;
Why not just write to partitions[] directly in the first place instead of using temporary variables and then copying them?
+static int gpt_mmc_default(int dev, char *str_part)
- struct mmc *mmc = find_mmc_device(dev);
- if (mmc == NULL) {
printf("%s: mmc dev %d NOT available\n", __func__, dev);return CMD_RET_FAILURE;- }
Why is this tied to MMC; shouldn't it work for e.g. USB storage as well? Use get_device_and_partition() instead.
- puts("Using default GPT UUID\n");
Even when the user explicitly supplied a partition layout on the command-line? Why print anything at all?
- /* allocate memory for partitions */
- disk_partition_t *partitions[part_count];
Don't variable declarations have to be at the start of a block in U-Boot?
+static int do_gpt(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) +{
- int ret = CMD_RET_SUCCESS;
- char *str_part = NULL;
- int dev = 0;
- if (argc < 3)
return CMD_RET_USAGE;- if (argc == 4) {
str_part = strdup(argv[3]);if (!str_part) {printf("%s: malloc failed!\n", __func__);return CMD_RET_FAILURE;}- }
The help text doesn't indicate that any of the command parameters are optional...
Why does this need to strdup() anything anyway?