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

[Piotr Wilczek]

Dear Stephen,

> -----Original Message-----
> From: Stephen Warren [mailto:swarren at wwwdotorg.org]
> Sent: Monday, November 19, 2012 9:17 PM
> To: Piotr Wilczek
> Cc: u-boot at lists.denx.de; Minkyu Kang; Kyungmin Park; Chang Hyun Park;
> Lukasz Majewski; Stephen Warren; Tom Rini; Rob Herring
> Subject: Re: [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?
> 
The const is not necessary.

> 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.
> 
That documentation should be moved to header files.

> > 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.
> 
I try to reorder the functions and eliminate prototypes.

> > +/**
> > + * set_gpt_table() - Restore the GUID Partition Table
> 
> "write" would probably be better than "set".
> 
Yes, "write" is better.

> > + *
> > + * @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.
> 
I should fix it.

> > + * @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?
> 
The partitions is a variable size array stack allocated and each element
points to heap allocated disk_partition. I think of how to reorganize this
part of the code.

> > +{
> > +	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?
> 
The idea is that if the user provided start address and the partitions does
not overlap, the partition is located at the start address. Otherwise
partitions are located next to each other.

> 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.
> 
Yes, it should be checked if CONFIG_PARTITION_UUIDS is defined at all.

> > +		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?
> 
Yes, it's a good idea.

> > +		/* partition attributes */
> > +		memset(&gpt_e[i].attributes, 0,
> > +		       sizeof(gpt_entry_attributes));
> 
> (Perhaps in the future) We should allow specifying attributes too.
> 
Ok

> > +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()?
> 
Ok.

> > +	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?
> 
It could be used by some other code in future.

> > +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?
Right, I fix it.

Best regards,
Piotr Wilczek
--
Samsung Poland R&D Center | Linux Platform Group