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

[Stephen Warren]

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);
>>> +		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.

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:

>> 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.