Hi Piotr,
Dear Stephen,
-----Original Message----- From: Stephen Warren [mailto:swarren@wwwdotorg.org] Sent: Monday, November 19, 2012 9:17 PM To: Piotr Wilczek Cc: u-boot@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.
The code allocates (and thereafter reads) the GPT_ENTRY_NUMBERS (which is 128) * sizeof(struct pte). It will be changed to allocate only as much space as needed.
- 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);
elsegpt_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);elsegpt_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.
Yes, you are right. User shall have the option to specify the size for the last partition. I think, that it is welcome to specify a special "mark" (e.g. size=- or size=0) to express that we want partition's size up to the end of the device.
/* 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?
LBA is the number of blocks. So the addressable range is 0 to LBA-1. The last_usable_lba is calculated with following equation (it is similar with parted tool):
lba - 2 - ptes_size = lba -2 -32 = lba - 34
ptes_size = number_of_entries(128) * sizeof(pte_entry) (128) / sector_size (512) = 32
- 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?
I think, that the uuid_gpt_disk shall be added as another key=value to the partitions.
e.g. gpt uuid_gpt_disk=6aa01a70-349d-11e2-ae74-001fd09285c0;name=boot,size=100MiB,uuid=xx;name=kernel, size=200MiB,uuid=yy;....
+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?
Will be changed to debug().
- 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.
Good point. Consistency will be provided in the next version.
+#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?
Since the guid_gen will be removed in a next version (the correct UUID will be provided from command line/env), the guid_dump will be removed as well.
+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'?
Yes, this is a mistake. Will be fixed.
- }
- 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