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

[Stephen Warren]

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?