> -----Original Message-----
> From: Stephen Warren [mailto:swar...@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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
