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

[Piotr Wilczek]

> -----Original Message-----
> From: Stephen Warren [mailto:swarren at wwwdotorg.org]
> Sent: Saturday, November 24, 2012 7:01 PM
> To: Piotr Wilczek
> Cc: u-boot at 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.