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

[Stephen Warren <swarren@wwwdotorg.org>]

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?