On Wed, Oct 10, 2012 at 12:14:56PM +0200, Davidlohr Bueso wrote:
> On Wed, 2012-10-10 at 11:40 +0200, Petr Uzel wrote:
> > On Sun, Oct 07, 2012 at 04:33:53PM +0200, Davidlohr Bueso wrote:
> > > +
> > > + if (lba == GPT_PRIMARY_PARTITION_TABLE_LBA) { /* primary */
> > > + header->alternative_lba = cpu_to_le64(cxt->total_sectors - 1);
> > > + header->partition_entry_lba = cpu_to_le64(2);
> > > + } else { /* backup */
> > > + header->alternative_lba = cpu_to_le64(GPT_PRIMARY_PARTITION_TABLE_LBA);
> > > + header->partition_entry_lba = header->last_usable_lba + cpu_to_le64(1);
> >
> > Shouldn't this ^^^ rather be:
> >
> > header->partition_entry_lba = cpu_to_le64(header->last_usable_lba + 1);
Both is incorrect from my point of view. The range <first..last> usable
LBA defines data area, nothing other. We should not use these numbers
to count location of the header (or partition entry array).
I can imagine device where data area will be smaller and behind the
area will be hidden "gray zone" for example with truecrypt data...
The primary header is at the begin of the device, backup header is
at the end of the device. So, let's use cxt->total_sectors.
esz = le32_to_cpu(h->npartition_entries) * sizeof(struct gpt_entry);
h->partition_entry_lba = cpu_to_le64(cxt->total_sectors - 1 - esz)
..anyway, see parted/libparted/labels/gpt.c: _generate_header
> > Even with this modification, I'm still not sure if the computation is
> > correct. Are you sure that partition_entry_lba (in alternative header)
> > should be set to this value?
>
> Well the UEFI specs sure don't make this very clear. It's obvious that
> for the primary header we want it to be LBA 2. Now, from this diagram
> http://en.wikipedia.org/w/index.php?title=File:GUID_Partition_Table_Scheme.svg&page=1
> I interpret it for the backup to be the last usable lba + 1.
The diagram uses "-1" or "-2" LBA, it means from end of the device.
> In any case ->last_usable_lba needs to be set before calling
Please, use last_usable_lba *only* to check validity of the entries in
partition entry array. The partition start and end has to be within
fist and last usable LBA. That's all.
[...]
> > > + /*
> > > + * 128 partitions is the default. It can go behond this, however,
> > > + * we're creating a de facto header here, so no funny business.
> > > + */
> > > + header->npartition_entries = cpu_to_le32(128);
> > > + header->sizeof_partition_entry = cpu_to_le32(sizeof(struct gpt_entry));
> > > +
> > > + header->first_usable_lba =
> > > + (header->sizeof_partition_entry * header->npartition_entries /
> > > + cpu_to_le64(cxt->sector_size)) + cpu_to_le64(2);
> > > + header->last_usable_lba =
> > > + cpu_to_le64(cxt->total_sectors) - header->first_usable_lba;
I'm always somehow nervous when I see code where we use in math
non-native byte order. It would be more robust and readable to have
cpu_to_le64(a + b + c) rather than cpu_to_le64(a) + b + c;
:-)
#define GPT_NPARTITIONS 128
uint64_t esz = sizeof(struct gpt_entry) * GPT_NPARTITIONS / cxt->sector_size;
header->npartition_entries = cpu_to_le32(GPT_NPARTITIONS);
header->sizeof_partition_entry = cpu_to_le32(sizeof(struct gpt_entry))
header->first_usable_lba = cpu_to_le64(2 + esz);
header->last_usable_lba = cpu_to_le64(cxt->total_sectors - 2 - esz);
> > > - if (1 == write(cxt->dev_fd, pmbr, 1))
> > > +
> > > + /* pMBR covers the first sector (LBA) of the disk */
> > > + if (cxt->sector_size ==
> > > + (unsigned long) write(cxt->dev_fd, pmbr, cxt->sector_size))
> > > return 0;
> >
> > Why not use write_all() from all-io.h ?
+1
>
> I don't really care, calling write directly is fine with me.
>
> I'll send a v2 with the corrections. Thanks for reviewing!
Please, send v2.
--
Karel Zak <kzak@xxxxxxxxxx>
http://karelzak.blogspot.com
--
To unsubscribe from this list: send the line "unsubscribe util-linux" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
