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?
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
