[U-Boot] [PATCH v4 5/7] gpt: Support for GPT (GUID Partition Table) restoration

Stephen Warren
2012-11-19T20:16:58Z

On 11/09/2012 03:48 AM, Piotr Wilczek wrote:
> The restoration of GPT table (both primary and secondary) is now possible.
> Simple GUID generation is supported.

> diff --git a/include/part.h b/include/part.h

> +int set_gpt_table(block_dev_desc_t *dev_desc,
> + gpt_header *gpt_h, gpt_entry *gpt_e);
> +void gpt_fill_pte(gpt_header *gpt_h, gpt_entry *gpt_e,
> + disk_partition_t *partitions[], int parts);
> +void gpt_fill_header(block_dev_desc_t *dev_desc, gpt_header *gpt_h);

> +void gpt_fill(block_dev_desc_t *dev_desc, disk_partition_t *partitions[],
> + const int parts_count);

Why const?

Some documentation for these functions (particularly high-level
information such as which order you'd expect to call them and why/what
for) would be useful in the header rather than the .c file; the header
is where people would go to find out what functions they can call, so
making them also go look in the .c file would be a bit annoying.

> diff --git a/disk/part_efi.c b/disk/part_efi.c

> +static int set_protective_mbr(block_dev_desc_t *dev_desc);
> +
> +static void guid_dump(u8 *guid, int i);
> +
> +static void guid_gen(const u32 *pool, u8 *guid);
> +
> +static int convert_uuid(char *uuid, u8 *dst);
> +
> +static void set_part_name(efi_char16_t *part_name, char *name);

There probably should be a CONFIG_ option required to enable this new
feature, so people who don't want it don't suffer code bloat.

Do you really need the blank lines between prototypes?

I suspect you can re-order the functions so that none of the prototypes
are needed anyway.

> +/**
> + * set_gpt_table() - Restore the GUID Partition Table

"write" would probably be better than "set".

> + *
> + * @param dev_desc - block device descriptor
> + * @param parts - number of partitions
> + * @param size - pointer to array with each partition size
> + * @param name - pointer to array with each partition name

Those last 3 lines don't match the prototype.

> + * @return - zero on success, otherwise error
> + */
> +int set_gpt_table(block_dev_desc_t *dev_desc,
> + gpt_header *gpt_h, gpt_entry *gpt_e)

Presumably the code assumes that gpt_e always has 128(?) entries.
Instead of taking a pointer, should the function take an array:
gpt_entry gpt_e[GPT_ENTRY_NUMBERS] to enforce this?

> +{
> + const int pte_blk_num = (GPT_ENTRY_NUMBERS * sizeof(gpt_entry)) /
> + dev_desc->blksz;

Related, this hard-codes the number of ptes as GPT_ENTRY_NUMBERS,
whereas ...

> + u32 calc_crc32;
> + u64 val;
> +
> + debug("max lba: %x\n", (u32) dev_desc->lba);
> + /* Setup the Protective MBR */
> + if (set_protective_mbr(dev_desc) < 0)
> + goto err;
> +
> + /* Generate CRC for the Primary GPT Header */
> + calc_crc32 = efi_crc32((const unsigned char *)gpt_e,
> + le32_to_cpu(gpt_h->num_partition_entries) *
> + le32_to_cpu(gpt_h->sizeof_partition_entry));

... here, gpt_h->num_partition_entries is used instead. Should both
places use the same size (entry count) definition?

> + if (dev_desc->block_write(dev_desc->dev, 2, pte_blk_num, gpt_e)
> + != pte_blk_num)
> + goto err;

Here, we assume GPT_ENTRY_NUMBERS entries in gpt_e. If the array isn't
that big, the block_write() above will read past the end of it.

> + puts("GPT successfully written to block device!\n");

Isn't that something that a command should be doing, not a utility
function? I'd rather have this function not print anything, except
perhaps on errors, just like typical Unix command-line applications.

> +void gpt_fill_pte(gpt_header *gpt_h, gpt_entry *gpt_e,
> + disk_partition_t *partitions[], int parts)

Why is partitions an array of pointers rather than an array of partitions?

> +{
> + u32 offset = (u32) le32_to_cpu(gpt_h->first_usable_lba);

I don't think there should be a space after the cast. The same comment
probably applies elsewhere.

> + for (i = 0; i < parts; i++) {
> + /* partition starting lba */
> + start = partitions[i]->start;
> + if (start && (offset <= start))
> + gpt_e[i].starting_lba = cpu_to_le32(start);
> + else
> + gpt_e[i].starting_lba = cpu_to_le32(offset);

That seems a little odd. The else branch seems fine when !start, but
what about when (start && (offset > start))? Shouldn't that be an error,
rather than just ignoring the requested start value?

Why can't partitions be out of order? IIRC, the GPT spec allows it.

> + /* partition ending lba */
> + if (i != parts - 1)
> + gpt_e[i].ending_lba =
> + cpu_to_le64(offset - 1);
> + else
> + gpt_e[i].ending_lba = gpt_h->last_usable_lba;

Why extend the last partition to fill the disk; why not simply always
use the requested size? If a "fill to max size" option is implemented,
the user might not want it to apply to the last partition, but perhaps
some arbitrary partition in the middle of the list.

> + /* partition type GUID*/
> + memcpy(gpt_e[i].partition_type_guid.b,
> + &PARTITION_BASIC_DATA_GUID, 16);

Shouldn't that be a user-specifiable option too? I suppose we could add
that later.

> + str_uuid = NULL;
> +#ifdef CONFIG_PARTITION_UUIDS
> + str_uuid = partitions[i]->uuid;
> +#endif

I think it should be required to enable that feature if creating GPTs;
writing a GPT without a value UUID seems wrong.

> + if (convert_uuid(str_uuid, gpt_e[i].unique_partition_guid.b)) {
> + guid_gen((((u32 *) gd->start_addr_sp) - i - 1),
> + guid);
> + memcpy(gpt_e[i].unique_partition_guid.b, guid,
> + sizeof(efi_guid_t));
> + }

Shouldn't there be a difference between no specified UUID, and an error
converting a specified UUID?

> + /* partition attributes */
> + memset(&gpt_e[i].attributes, 0,
> + sizeof(gpt_entry_attributes));

(Perhaps in the future) We should allow specifying attributes too.

> +void gpt_fill_header(block_dev_desc_t *dev_desc, gpt_header *gpt_h)

> + gpt_h->first_usable_lba = cpu_to_le64(34);
> + gpt_h->last_usable_lba = cpu_to_le64(dev_desc->lba - 34);

Is lba the number of LBAs or the last LBA number? include/part.h says
its the number of LBAs, in which case I think that should be
dev_desc->lba - 1 - 34?

> + s = getenv("uuid_gpt_disk");

I'd rather not depend on environment variables; can't this be a
command-line parameter to the gpt command?

> +void gpt_fill(block_dev_desc_t *dev_desc, disk_partition_t *partitions[],
> + const int parts_count)

Rename to gpt_write()?

> + puts("save the GPT ...\n");

Again, why print anything here?

> + set_gpt_table(dev_desc, gpt_h, gpt_e);

Oh, so is set_gpt_table() an internal-only function? If so, shouldn't it
be static and not in the header file?

> +static int set_protective_mbr(block_dev_desc_t *dev_desc)
> +{
> + legacy_mbr p_mbr;
> +
> + /* Setup the Protective MBR */
> + memset((u32 *) &p_mbr, 0x00, sizeof(p_mbr));

Why use a stack variable and memset() here, but use calloc() in
gpt_fill() above? That seems inconsistent.

> +#ifdef DEBUG
> +static void guid_dump(u8 *guid, int i)
> +{
> + int k;
> +
> + debug("GUID: ");
> + for (k = 0; k < i; k++, guid++)
> + debug(" %x ", *guid);
> + debug("\n");
> +}
> +#else
> +static void guid_dump(u8 *guid, int i) {}
> +#endif

Wouldn't using the existing uuid_string be better?

> +static int convert_uuid(char *uuid, u8 *dst)

It's not too clear from the function name what kind of conversion is
being applied. string_uuid() would be more consistent with the existing
uuid_string(), or perhaps even better string_to_uuid().

> +static void set_part_name(efi_char16_t *part_name, char *name)

> + for (k = 0, j = 0; k < strlen(name); k++, j += 2) {
> + p[j] = *(name + k);
> + p[j + 1] = '.';

Not '\0'?

> + }
> + memcpy(part_name, p, strlen(p));

Why not write directly to part_name?