| Message ID | 1352458087-20462-3-git-send-email-p.wilczek@samsung.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
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?
Dear Stephen, > -----Original Message----- > From: Stephen Warren [mailto:swarren@wwwdotorg.org] > Sent: Monday, November 19, 2012 10:35 PM > To: Piotr Wilczek > Cc: u-boot@lists.denx.de; Minkyu Kang; Kyungmin Park; Chang Hyun Park; > Lukasz Majewski; Stephen Warren; Tom Rini; Rob Herring > Subject: Re: [PATCH v4 6/7] gpt: Support for new "gpt" command > > 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. > The intention was that the command automatically figures out whether user passed environmental variable or directly partitions as text. Then the command splits this string, checks if it is a valid partitions list and if so the table is written to mmc. That is how the code is supposed to work. The question here is, if it should work like that. If it is desired that user explicitly states that the partitions list is passed from environmental, then I should change the code. > > + " 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. > I agree that the parameter "write" should be implemented. > > +/** > > + * 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? > Yes, it was. The u-boot's shell expands to one only, so it allow to pass any partition parameter as env when the partitions list itself is passed as env. > > +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. > p2-p1-1 gives length of the env name + trailing zero. p2-p1-2 would give only the env's length and the trailing zero wouldn't be moved. > Spaces around operators? s/p+2/p + 2/ for example. > Yes > > +/** > > + * extract_val(): Extract value from a key=value pair > > + * > > + * @param p - pointer to string > > Pointer to pointer to string, given its type? > Right > > + * @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. > I change the names. > > + 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? > I check it. > > + 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? > Yes, it is. > > +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? > If the size parameter is passed as env (and the partitions list is passed as env) it has to be expanded manually and string allocation then needed. If we decide to remove this feature then you are right. > > + printf("PARTITIONS: %s\n", s); > > Why print that? > Right. > > + 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(). > Ok. > 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()? > The 'parts_count' is needed for dynamic array size for 'partions' and it is to passed to the 'gpt_fill' function. However I think of how to organize it all in a better way. > > + 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=). > I would say that the "key=value" is flexible. The 'extract_env' function tells you if the requested key was provided or not. Also, the order of keys is not important. > > + 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. > The 'set_gpt_info' was designed in a way that allocations for ps, name and uuid are done when the value is assigned to them (the 'extract_env' function). It just consistent with that. > > + p = ps[i]; > > + size[i] = ustrtoul(p, &p, 0); > > + size[i] /= dev_desc->blksz; > > What if the size isn't rounded correctly? > Some checking should be added. > > + 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? > The only advantage is that the partitions[] is modified only when the key extraction was successful. > > +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? > Yes, you are right. > > +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? Best regards, Piotr Wilczek -- Samsung Poland R&D Center | Linux Platform Group
On 11/21/2012 04:22 AM, Piotr Wilczek wrote: > Dear Stephen, > >> Stephen Warren wrote at Monday, November 19, 2012 10:35 PM: >> 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. > > The intention was that the command automatically figures out whether user > passed environmental variable or directly partitions as text. Then the > command splits this string, checks if it is a valid partitions list and if > so the table is written to mmc. That is how the code is supposed to work. > > The question here is, if it should work like that. If it is desired that > user explicitly states that the partitions list is passed from > environmental, then I should change the code. I personally prefer things to be explicit; that way, there can't be any corner-case that isn't covered by the automatic mode. >>> +/** >>> + * 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? > > Yes, it was. The u-boot's shell expands to one only, so it allow to pass any > partition parameter as env when the partitions list itself is passed as env. OK. The issue here is that the comment doesn't exactly describe what the code is doing. Also, what if the user wrote "foo${var}bar"; I can't recall if the code handles that correct; is the result of that just "${var}", or do "foo" and "bar" actually make it into the result string? >>> +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. >> > p2-p1-1 gives length of the env name + trailing zero. > p2-p1-2 would give only the env's length and the trailing zero wouldn't be > moved. Ah right. I tend to write things like that as: p2-p1-2+1 /* strlen("${")==2, length '\0'==1 to make it obvious what's going on >>> + 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=). >> > I would say that the "key=value" is flexible. The 'extract_env' function > tells you if the requested key was provided or not. Also, the order of keys > is not important. The order of the keys shouldn't be important, but doesn't the code above expect to find key "name" first, then key "size", etc., in tha specific order, as it walks through the string?
> -----Original Message----- > From: Stephen Warren [mailto:swarren@wwwdotorg.org] > Sent: Saturday, November 24, 2012 7:01 PM > To: Piotr Wilczek > Cc: u-boot@lists.denx.de; 'Minkyu Kang'; 'Kyungmin Park'; 'Chang Hyun > Park'; Lukasz Majewski; 'Stephen Warren'; 'Tom Rini'; 'Rob Herring' > Subject: Re: [PATCH v4 6/7] gpt: Support for new "gpt" command > > On 11/21/2012 04:22 AM, Piotr Wilczek wrote: > > Dear Stephen, > > > >> Stephen Warren wrote at Monday, November 19, 2012 10:35 PM: > >> 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. > > > > The intention was that the command automatically figures out whether > > user passed environmental variable or directly partitions as text. > > Then the command splits this string, checks if it is a valid > > partitions list and if so the table is written to mmc. That is how > the code is supposed to work. > > > > The question here is, if it should work like that. If it is desired > > that user explicitly states that the partitions list is passed from > > environmental, then I should change the code. > > I personally prefer things to be explicit; that way, there can't be any > corner-case that isn't covered by the automatic mode. > Ok. The partitions list will be passed in two methods: gpt create mmc 0 ${partitions_env_name} - from environmental or gpt create mmc 0 "name=..,size=..,uuid=..;..." - form text In both cases any partition parameter can also be passes as env: gpt create mmc 0 "name=..,size=${part1_size},uuid=..;..." > >>> +/** > >>> + * 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? > > > > Yes, it was. The u-boot's shell expands to one only, so it allow to > > pass any partition parameter as env when the partitions list itself > is passed as env. > > OK. The issue here is that the comment doesn't exactly describe what > the code is doing. > > Also, what if the user wrote "foo${var}bar"; I can't recall if the code > handles that correct; is the result of that just "${var}", or do "foo" > and "bar" actually make it into the result string? The 'bar' will be dropped, but to drop 'foo' a small modification is needed. > > >>> +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. > >> > > p2-p1-1 gives length of the env name + trailing zero. > > p2-p1-2 would give only the env's length and the trailing zero > > wouldn't be moved. > > Ah right. I tend to write things like that as: > > p2-p1-2+1 /* strlen("${")==2, length '\0'==1 > > to make it obvious what's going on > > >>> + 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=). > >> > > I would say that the "key=value" is flexible. The 'extract_env' > > function tells you if the requested key was provided or not. Also, > the > > order of keys is not important. > > The order of the keys shouldn't be important, but doesn't the code > above expect to find key "name" first, then key "size", etc., in tha > specific order, as it walks through the string? The key name is passed to 'extract_val' and the function should search that key no matter what order the keys appear in searched string. I check it again.
On 11/26/2012 06:08 AM, Piotr Wilczek wrote: > > Stephen Warren wrote at Saturday, November 24, 2012 7:01 PM: >> On 11/21/2012 04:22 AM, Piotr Wilczek wrote: >>> Dear Stephen, >>> >>>> Stephen Warren wrote at Monday, November 19, 2012 10:35 PM: >>>> 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. >>>>> +/** >>>>> + * 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? >>> >>> Yes, it was. The u-boot's shell expands to one only, so it allow to >>> pass any partition parameter as env when the partitions list itself >> is passed as env. >> >> OK. The issue here is that the comment doesn't exactly describe what >> the code is doing. >> >> Also, what if the user wrote "foo${var}bar"; I can't recall if the code >> handles that correct; is the result of that just "${var}", or do "foo" >> and "bar" actually make it into the result string? > > The 'bar' will be dropped, but to drop 'foo' a small modification is needed. Rather than modify the code to drop "foo", why not modify it so that "bar" and "foo" are included in the result? That seem more like what the user desired, and is consistent with what any other shell or scripting language would do. If not, please return an error in this case so that bad syntax can be fixed.
diff --git a/common/Makefile b/common/Makefile index 9e43322..741781f 100644 --- a/common/Makefile +++ b/common/Makefile @@ -192,6 +192,7 @@ COBJS-$(CONFIG_MODEM_SUPPORT) += modem.o COBJS-$(CONFIG_UPDATE_TFTP) += update.o COBJS-$(CONFIG_USB_KEYBOARD) += usb_kbd.o COBJS-$(CONFIG_CMD_DFU) += cmd_dfu.o +COBJS-$(CONFIG_CMD_GPT) += cmd_gpt.o endif ifdef CONFIG_SPL_BUILD diff --git a/common/cmd_gpt.c b/common/cmd_gpt.c new file mode 100644 index 0000000..171eeb3 --- /dev/null +++ b/common/cmd_gpt.c @@ -0,0 +1,300 @@ +/* + * cmd_gpt.c -- GPT (GUID Partition Table) handling command + * + * Copyright (C) 2012 Samsung Electronics + * author: Lukasz Majewski <l.majewski@samsung.com> + * author: Piotr Wilczek <p.wilczek@samsung.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + */ + +#include <common.h> +#include <malloc.h> +#include <command.h> +#include <mmc.h> +#include <part_efi.h> +#include <exports.h> + +/** + * extract_env(): Convert string from '&{env_name}' to 'env_name' + * + * @param p - pointer to string + * + * @return - zero on success + */ +static int extract_env(char *p) +{ + char *p1, *p2; + + if (!p) + return -1; + + p1 = strstr(p, "${"); + p2 = strstr(p, "}"); + + if (p1 && p2) { + *p2 = '\0'; + memmove(p, p+2, p2-p1-1); + } else { + return -2; + } + + return 0; +} + +/** + * extract_val(): Extract value from a key=value pair + * + * @param p - pointer to string + * @param tab - table to store extracted value + * @param i - actual tab element to work on + * @param key - extract only given key + * + * @return - zero on success; otherwise error + */ +static int extract_val(char **p, char *tab[], int i, char *key) +{ + char *t, *e, *tok = *p; + char *k; + + t = strsep(&tok, ","); + k = t; + strsep(&t, "="); + + if (key && strcmp(k, key)) + return -2; + + if (extract_env(t) == 0) { + e = getenv(t); + if (e == NULL) + return -1; + t = e; + } + + tab[i] = calloc(strlen(t) + 1, 1); + if (tab[i] == NULL) { + printf("%s: calloc failed!\n", __func__); + return -1; + } + strcpy(tab[i], t); + *p = tok; + + return 0; +} + +/** + * set_gpt_info(): Fill gpt information for GPT header and partition entries + * + * @param dev_desc - block device descriptor + * @param str_uuid - string uuid representation + * + * @return - zero on success; otherwise error + * + */ +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]; + char *name[parts_count]; + char *uuid[parts_count]; + unsigned int size[parts_count]; + char *tok, *p, *s, *ss; + int i, ret; + + debug("%s: MMC lba num: 0x%x %d\n", __func__, + (unsigned int)dev_desc->lba, (unsigned int)dev_desc->lba); + + if (!str_part) + return -2; + ret = 0; + + s = str_part; + + printf("PARTITIONS: %s\n", s); + ss = calloc(strlen(s) + 1, 1); + if (ss == NULL) { + printf("%s: calloc failed!\n", __func__); + return -1; + } + memcpy(ss, s, strlen(s) + 1); + + for (i = 0, p = ss; i < parts_count; i++) { + 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; + } + + if (extract_val(&tok, uuid, i, "uuid")) { + /* uuid string length equals 37 */ + uuid[i] = calloc(37, 1); + if (uuid[i] == NULL) { + printf("%s: calloc failed!\n", __func__); + ret = -1; + free(name[i]); + free(ps[i]); + goto err; + } + } + } + + printf("found %d partitions\n", i); + + for (i = 0; i < parts_count; i++) { + p = ps[i]; + size[i] = ustrtoul(p, &p, 0); + size[i] /= dev_desc->blksz; + } + + for (i = 0; i < parts_count; i++) { + partitions[i]->size = size[i]; + partitions[i]->blksz = dev_desc->blksz; + strncpy((char *)&partitions[i]->name, name[i], + sizeof(partitions[i]->name)); +#ifdef CONFIG_PARTITION_UUIDS + strncpy((char *)&partitions[i]->uuid, uuid[i], + sizeof(partitions[i]->uuid)); +#endif + } + + i = parts_count; + err: + for (i--; i >= 0; i--) { + free(name[i]); + free(uuid[i]); + free(ps[i]); + } + + free(ss); + return ret; +} + +static int gpt_mmc_default(int dev, char *str_part) +{ + int i; + u8 part_count; + char *s , *p; + + struct mmc *mmc = find_mmc_device(dev); + + if (mmc == NULL) { + printf("%s: mmc dev %d NOT available\n", __func__, dev); + return CMD_RET_FAILURE; + } + + puts("Using default GPT UUID\n"); + + /* get partitions list string */ + if (!str_part) +#ifdef DEFAULT_PARTITIONS_ENV + str_part = DEFAULT_PARTITIONS_ENV; +#else + str_part = "partitions"; +#endif + s = getenv(str_part); + if (s == NULL) + s = str_part; + + /* calculate expected number of partitions */ + part_count = 1; + p = s; + while (*p) { + if (*p++ == ';') + part_count++; + } + + /* allocate memory for partitions */ + disk_partition_t *partitions[part_count]; + for (i = 0; i < part_count; i++) { + partitions[i] = calloc(sizeof(disk_partition_t), part_count); + if (partitions[i] == NULL) + goto err; + } + + /* fill partitions */ + if (!set_gpt_info(&mmc->block_dev, s, partitions, part_count)) + /* save partions layout do disk */ + gpt_fill(&mmc->block_dev, partitions, part_count); + else + printf("Partitions list incomplete\n"); + +err: + /* free memory for partitions */ + for (i--; i >= 0; i--) + free(partitions[i]); + + return 0; +} + +/** + * do_gpt(): Perform GPT operations + * + * @param cmdtp - command name + * @param flag + * @param argc + * @param argv + * + * @return zero on success; otherwise error + */ +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; + } + } + + if (strcmp(argv[1], "mmc") == 0) { + dev = (int)simple_strtoul(argv[2], NULL, 10); + if (gpt_mmc_default(dev, str_part)) + return CMD_RET_FAILURE; + } + + if (argc == 4) + free(str_part); + + return ret; +} + +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" + " 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" +);