[U-Boot] [PATCH v4 5/7] gpt: Support for GPT (GUID Partition Table) restoration
Stephen Warren
swarren at wwwdotorg.org
Mon Nov 19 21:16:58 CET 2012
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?
More information about the U-Boot
mailing list