On Wed, May 11, 2016 at 03:54:16PM +0100, Sassan Panahinejad wrote:
> + /* get table length */
> + unsigned long (*get_length)(struct fdisk_context *cxt);
This is already available as fdisk_get_npartitions(), for GPT it
returns header->npartition_entries.
And also
fdisk_get_disklabel_item(cxt, GPT_LABELITEM_ENTRIESALLOC, item);
returns the information, but public labelitem API is not implemented
yet (you cannot access 'item' struct members). I'll fix it ASAP.
> + /* set table length */
> + int (*set_length)(struct fdisk_context *cxt, unsigned long length);
Seems like a good idea, but I'm not sure if we really need it as
generic label operation (basic fdisk API). It seems that reallocate
number of PT entries is very GPT specific.
I think it would be enough to add GPT function to the library API, we
already have many functions to set for example SUN partition table
stuff and I think add
fdisk_gpt_set_npartitions()
would be good enough.
(It would be also possible to add fdisk_set_disklabel_item(), to make
these things more generic, but I think it's still more user friendly
to have fdisk_gpt_set_npartitions() shortcut for this type of operations.)
> +static int gpt_set_disklabel_length(struct fdisk_context *cxt, unsigned long new)
> +{
> + struct fdisk_gpt_label *gpt;
> + ssize_t esz;
> + unsigned long old;
> +
> + assert(cxt);
> + assert(cxt->label);
> + assert(fdisk_is_label(cxt, GPT));
> +
> + gpt = self_label(cxt);
> +
> + old = le32_to_cpu(gpt->pheader->npartition_entries);
> +
> + gpt->pheader->npartition_entries = cpu_to_le32(new);
> + gpt->bheader->npartition_entries = cpu_to_le32(new);
> +
> + /* calculate new size of the entries array */
> + esz = le32_to_cpu(gpt->pheader->npartition_entries) *
> + le32_to_cpu(gpt->pheader->sizeof_partition_entry);
> +
> + /* if expanding the table, allocate more memory and zero.
> + * Warn the user that this can cause data loss! (if table overlaps partition) */
> + if (new > old) {
> + fdisk_warnx(cxt, _("Expanding GPT may cause data loss!"));
> + gpt->ents = realloc(gpt->ents, esz);
We care about allocation errors.
tmp = realloc(gpt->ents, esz);
if (!tmp)
return -ENOMEM;
gpt->ents = tmp;
It would be also better to reallocate before you change anything in
the header to avoid inconsistent header after ENOMEM.
> + memset(gpt->ents + old, 0, (new - old) * le32_to_cpu(gpt->pheader->sizeof_partition_entry));
> + }
> + /* usable LBA addresses will have changed */
> + cxt->last_lba = cxt->total_sectors - 2 - (esz / cxt->sector_size);
> + cxt->first_lba = (esz / cxt->sector_size) + 2;
Please, call fdisk_set_last_lba() and fdisk_set_first_lba()
> + gpt->pheader->first_usable_lba = cpu_to_le32(cxt->first_lba);
> + gpt->bheader->first_usable_lba = cpu_to_le32(cxt->first_lba);
> + gpt->pheader->last_usable_lba = cpu_to_le32(cxt->last_lba);
> + gpt->bheader->last_usable_lba = cpu_to_le32(cxt->last_lba);
All GPT LBA are 64-bit.
It seems you don't care if existing partitions are in the new range
specified by first_usable_lba and last_usable_lba. It seems very wrong.
My recommendation is to check the partitions before you change
anything and if any partition is out of the range then report it to
user as error (and suggest new start offset for the partition).
wanted_first_lba = (esz / cxt->sector_size) + 2;
for (i = 0; i < le32_to_cpu(header->npartition_entries); i++) {
if (partition_unused(&ents[i]))
continue;
if (gpt_partition_start(&ents[i]) < wanted_first_lba) {
fdisk_warnx(cxt, _("Partition #%d out of range (minimal start is %ju sectors)",
i + 1, wanted_first_lba));
rc = -EINVAL;
}
}
if (rc)
return rc;
IMHO user should be forced to fix the partitions before he modify
first_usable_lba. (Yes, we don't want to resize the partitions
automatically in this function.)
Karel
--
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
