[U-Boot] [PATCH v4 6/7] gpt: Support for new "gpt" command

[Stephen Warren <swarren@wwwdotorg.org>]

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?