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?