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

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?