-----Original Message----- From: Stephen Warren [mailto:swarren@wwwdotorg.org] Sent: Saturday, November 24, 2012 7:06 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/21/2012 06:18 AM, Piotr Wilczek wrote:
Dear Stephen,
Stephen Warren wrote at Monday, November 19, 2012 9:17 PM: 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.
...
- 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.
But if the user provided a start address, shouldn't it always be honored exactly, or any error generated; silently ignoring a start address when there's an overlap seems wrong. Also, the overlap checking seems a little simplistic; it should really sort the partition array and then walk through checking for overlaps, rather than maintaining a single "highest used offset", since there's no requirement for the physical order of partitions to match the order in the partition table, hence why I asked:
You are right. Ignoring start address when there's overlap is a bad idea and I change the code.
The overlap check method is simple: the offset points to the first address where the next partition can start. At first it is equal to the first usable lba, then is increased by size of the previous partition (now I see that there's a bug in the code I posted, it should be: offset += partitions[i]->size); I think that this simple overlap check method should be enough at the moment.
Why can't partitions be out of order? IIRC, the GPT spec allows it.
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.
Perhaps. It can always be made non-static at that time.
Making it non-static I say that it was designed to be used by other code.