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

[Piotr Wilczek]

Dear Stephen,

> -----Original Message-----
> From: Stephen Warren [mailto:swarren at wwwdotorg.org]
> Sent: Monday, November 19, 2012 10:35 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/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