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