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

login
register
mail settings
Submitter Piotr Wilczek
Date Nov. 9, 2012, 10:48 a.m.
Message ID <1352458087-20462-3-git-send-email-p.wilczek@samsung.com>
Download mbox | patch
Permalink /patch/198018/
State Changes Requested
Delegated to: Tom Rini
Headers show

Comments

Piotr Wilczek - Nov. 9, 2012, 10:48 a.m.
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.

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
Signed-off-by: Piotr Wilczek <p.wilczek@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
Changes for v2:
- gpt command now accepts device medium and its number (e.g. gpt mmc 0)
- UUIDs can be passed via u-boot prompt when used with gpt command
- Format of restored GPT has been changed - now key=value pairs are used
  'name="PARTS_CSA",size=8MiB;\ '
  'name="PARTS_BOOTLOADER",size=60MiB; \'
- guid_gen now accepts "pool" pointer and guid pointer
- gd->start_addr_sp is used as a primary source of entrophy
- static buffers definitions have been removed
- remove memsize_to_blocks function with call to standard ustrtoul
- doxygen comments for functions added

Changes for v3:
- Remove unnecessary braces

Changes for v4:
- partions list can be passed as environment variable or directly as text
- each value in partions list can be passed as environment variable
---
 common/Makefile  |    1 +
 common/cmd_gpt.c |  300 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 301 insertions(+), 0 deletions(-)
 create mode 100644 common/cmd_gpt.c
Stephen Warren - Nov. 19, 2012, 9:34 p.m.
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.

> +	"    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.

> +/**
> + * 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?

> +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.

Spaces around operators? s/p+2/p + 2/ for example.

> +/**
> + * extract_val(): Extract value from a key=value pair
> + *
> + * @param p - pointer to string

Pointer to pointer to string, given its type?

> + * @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.

> +	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?

> +	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?

> +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?

> +	printf("PARTITIONS: %s\n", s);

Why print that?

> +	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().

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()?

> +		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=).

> +		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.

> +		p = ps[i];
> +		size[i] = ustrtoul(p, &p, 0);
> +		size[i] /= dev_desc->blksz;

What if the size isn't rounded correctly?

> +	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?

> +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?

> +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?
Piotr Wilczek - Nov. 21, 2012, 11:22 a.m.
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
Stephen Warren - Nov. 24, 2012, 6 p.m.
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?
Piotr Wilczek - Nov. 26, 2012, 1:08 p.m.
> -----Original Message-----
> From: Stephen Warren [mailto:swarren@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.
Stephen Warren - Nov. 26, 2012, 11:49 p.m.
On 11/26/2012 06:08 AM, Piotr Wilczek wrote:
> 
> Stephen Warren wrote at Saturday, November 24, 2012 7:01 PM:
>> 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.

>>>>> +/**
>>>>> + * 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.

Rather than modify the code to drop "foo", why not modify it so that
"bar" and "foo" are included in the result? That seem more like what the
user desired, and is consistent with what any other shell or scripting
language would do. If not, please return an error in this case so that
bad syntax can be fixed.

Patch

diff --git a/common/Makefile b/common/Makefile
index 9e43322..741781f 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -192,6 +192,7 @@  COBJS-$(CONFIG_MODEM_SUPPORT) += modem.o
 COBJS-$(CONFIG_UPDATE_TFTP) += update.o
 COBJS-$(CONFIG_USB_KEYBOARD) += usb_kbd.o
 COBJS-$(CONFIG_CMD_DFU) += cmd_dfu.o
+COBJS-$(CONFIG_CMD_GPT) += cmd_gpt.o
 endif
 
 ifdef CONFIG_SPL_BUILD
diff --git a/common/cmd_gpt.c b/common/cmd_gpt.c
new file mode 100644
index 0000000..171eeb3
--- /dev/null
+++ b/common/cmd_gpt.c
@@ -0,0 +1,300 @@ 
+/*
+ * cmd_gpt.c -- GPT (GUID Partition Table) handling command
+ *
+ * Copyright (C) 2012 Samsung Electronics
+ * author: Lukasz Majewski <l.majewski@samsung.com>
+ * author: Piotr Wilczek <p.wilczek@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <common.h>
+#include <malloc.h>
+#include <command.h>
+#include <mmc.h>
+#include <part_efi.h>
+#include <exports.h>
+
+/**
+ * extract_env(): Convert string from '&{env_name}' to 'env_name'
+ *
+ * @param p - pointer to string
+ *
+ * @return - zero on success
+ */
+static int extract_env(char *p)
+{
+	char *p1, *p2;
+
+	if (!p)
+		return -1;
+
+	p1 = strstr(p, "${");
+	p2 = strstr(p, "}");
+
+	if (p1 && p2) {
+		*p2 = '\0';
+		memmove(p, p+2, p2-p1-1);
+	} else {
+		return -2;
+	}
+
+	return 0;
+}
+
+/**
+ * extract_val(): Extract value from a key=value pair
+ *
+ * @param p - pointer to string
+ * @param tab - table to store extracted value
+ * @param i - actual tab element to work on
+ * @param key - extract only given key
+ *
+ * @return - zero on success; otherwise error
+ */
+static int extract_val(char **p, char *tab[], int i, char *key)
+{
+	char *t, *e, *tok = *p;
+	char *k;
+
+	t = strsep(&tok, ",");
+	k = t;
+	strsep(&t, "=");
+
+	if (key && strcmp(k, key))
+		return -2;
+
+	if (extract_env(t) == 0) {
+		e = getenv(t);
+		if (e == NULL)
+			return -1;
+		t = e;
+	}
+
+	tab[i] = calloc(strlen(t) + 1, 1);
+	if (tab[i] == NULL) {
+		printf("%s: calloc failed!\n", __func__);
+		return -1;
+	}
+	strcpy(tab[i], t);
+	*p = tok;
+
+	return 0;
+}
+
+/**
+ * set_gpt_info(): Fill gpt information for GPT header and partition entries
+ *
+ * @param dev_desc - block device descriptor
+ * @param str_uuid - string uuid representation
+ *
+ * @return - zero on success; otherwise error
+ *
+ */
+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];
+	char *name[parts_count];
+	char *uuid[parts_count];
+	unsigned int size[parts_count];
+	char *tok, *p, *s, *ss;
+	int i, ret;
+
+	debug("%s: MMC lba num: 0x%x %d\n", __func__,
+	      (unsigned int)dev_desc->lba, (unsigned int)dev_desc->lba);
+
+	if (!str_part)
+		return -2;
+	ret = 0;
+
+	s = str_part;
+
+	printf("PARTITIONS: %s\n", s);
+	ss = calloc(strlen(s) + 1, 1);
+	if (ss == NULL) {
+		printf("%s: calloc failed!\n", __func__);
+		return -1;
+	}
+	memcpy(ss, s, strlen(s) + 1);
+
+	for (i = 0, p = ss; i < parts_count; i++) {
+		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;
+		}
+
+		if (extract_val(&tok, uuid, i, "uuid")) {
+			/* uuid string length equals 37 */
+			uuid[i] = calloc(37, 1);
+			if (uuid[i] == NULL) {
+				printf("%s: calloc failed!\n", __func__);
+				ret = -1;
+				free(name[i]);
+				free(ps[i]);
+				goto err;
+			}
+		}
+	}
+
+	printf("found %d partitions\n", i);
+
+	for (i = 0; i < parts_count; i++) {
+		p = ps[i];
+		size[i] = ustrtoul(p, &p, 0);
+		size[i] /= dev_desc->blksz;
+	}
+
+	for (i = 0; i < parts_count; i++) {
+		partitions[i]->size = size[i];
+		partitions[i]->blksz = dev_desc->blksz;
+		strncpy((char *)&partitions[i]->name, name[i],
+				sizeof(partitions[i]->name));
+#ifdef CONFIG_PARTITION_UUIDS
+		strncpy((char *)&partitions[i]->uuid, uuid[i],
+			sizeof(partitions[i]->uuid));
+#endif
+	}
+
+	i = parts_count;
+ err:
+	for (i--; i >= 0; i--) {
+		free(name[i]);
+		free(uuid[i]);
+		free(ps[i]);
+	}
+
+	free(ss);
+	return ret;
+}
+
+static int gpt_mmc_default(int dev, char *str_part)
+{
+	int i;
+	u8 part_count;
+	char *s , *p;
+
+	struct mmc *mmc = find_mmc_device(dev);
+
+	if (mmc == NULL) {
+		printf("%s: mmc dev %d NOT available\n", __func__, dev);
+		return CMD_RET_FAILURE;
+	}
+
+	puts("Using default GPT UUID\n");
+
+	/* get partitions list string */
+	if (!str_part)
+#ifdef DEFAULT_PARTITIONS_ENV
+		str_part = DEFAULT_PARTITIONS_ENV;
+#else
+		str_part = "partitions";
+#endif
+	s = getenv(str_part);
+	if (s == NULL)
+		s = str_part;
+
+	/* calculate expected number of partitions */
+	part_count = 1;
+	p = s;
+	while (*p) {
+		if (*p++ == ';')
+			part_count++;
+	}
+
+	/* allocate memory for partitions */
+	disk_partition_t *partitions[part_count];
+	for (i = 0; i < part_count; i++) {
+		partitions[i] = calloc(sizeof(disk_partition_t), part_count);
+		if (partitions[i] == NULL)
+			goto err;
+	}
+
+	/* fill partitions */
+	if (!set_gpt_info(&mmc->block_dev, s, partitions, part_count))
+		/* save partions layout do disk */
+		gpt_fill(&mmc->block_dev, partitions, part_count);
+	else
+		printf("Partitions list incomplete\n");
+
+err:
+	/* free memory for partitions */
+	for (i--; i >= 0; i--)
+		free(partitions[i]);
+
+	return 0;
+}
+
+/**
+ * do_gpt(): Perform GPT operations
+ *
+ * @param cmdtp - command name
+ * @param flag
+ * @param argc
+ * @param argv
+ *
+ * @return zero on success; otherwise error
+ */
+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;
+		}
+	}
+
+	if (strcmp(argv[1], "mmc") == 0) {
+		dev = (int)simple_strtoul(argv[2], NULL, 10);
+		if (gpt_mmc_default(dev, str_part))
+			return CMD_RET_FAILURE;
+	}
+
+	if (argc == 4)
+		free(str_part);
+
+	return ret;
+}
+
+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"
+	"    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"
+);