[PATCH 1/3] libfdisk: Add support for altering GPT size

[PATCH 1/3] libfdisk: Add support for altering GPT size

[Sassan Panahinejad]

This is useful in two situations:

1. More than 128 partitions are required. Or

2. The partition table must be restricted in size, such as when a system
expects to find a bootloader at a location that would otherwise overlap the
partition table.

The gdisk partitioner supports this feature.

libfdisk is already capable of reading and writing partition tables of any
size, but previously could only create ones of 128 entries and could not
resize.

This change should be fairly safe, as it has no effect unless explicitly
activated.
---
 libfdisk/src/fdiskP.h      |  4 +++
 libfdisk/src/gpt.c         | 65 ++++++++++++++++++++++++++++++++++++++++++++++
 libfdisk/src/label.c       | 34 ++++++++++++++++++++++++
 libfdisk/src/libfdisk.h.in |  2 ++
 libfdisk/src/libfdisk.sym  |  2 ++
 5 files changed, 107 insertions(+)

diff --git a/libfdisk/src/fdiskP.h b/libfdisk/src/fdiskP.h
index 6444c3d..cb17cb0 100644
--- a/libfdisk/src/fdiskP.h
+++ b/libfdisk/src/fdiskP.h
@@ -204,6 +204,10 @@ struct fdisk_label_operations {
 	int (*get_item)(struct fdisk_context *cxt, struct fdisk_labelitem *item);
 	/* set disk label ID */
 	int (*set_id)(struct fdisk_context *cxt);
+	/* get table length */
+	unsigned long (*get_length)(struct fdisk_context *cxt);
+	/* set table length */
+	int (*set_length)(struct fdisk_context *cxt, unsigned long length);
 
 
 	/* new partition */
diff --git a/libfdisk/src/gpt.c b/libfdisk/src/gpt.c
index 39c93bb..86b6a6e 100644
--- a/libfdisk/src/gpt.c
+++ b/libfdisk/src/gpt.c
@@ -2453,6 +2453,69 @@ static int gpt_set_disklabel_id(struct fdisk_context *cxt)
 	return 0;
 }
 
+static unsigned long gpt_get_disklabel_length(struct fdisk_context *cxt)
+{
+	struct fdisk_gpt_label *gpt;
+
+	assert(cxt);
+	assert(cxt->label);
+	assert(fdisk_is_label(cxt, GPT));
+
+	gpt = self_label(cxt);
+
+	return le32_to_cpu(gpt->pheader->npartition_entries);
+}
+
+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);
+		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;
+	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);
+
+
+	/* The backup header must be recalculated */
+	gpt_mknew_header_common(cxt, gpt->bheader, le64_to_cpu(gpt->pheader->alternative_lba));
+
+	/* CRCs will have changed */
+	gpt_recompute_crc(gpt->pheader, gpt->ents);
+	gpt_recompute_crc(gpt->bheader, gpt->ents);
+
+	fdisk_info(cxt, _("Partition table length changed from %u to %u."), old, new);
+
+	fdisk_label_set_changed(cxt->label, 1);
+	return 0;
+}
+
 static int gpt_part_is_used(struct fdisk_context *cxt, size_t i)
 {
 	struct fdisk_gpt_label *gpt;
@@ -2750,6 +2813,8 @@ static const struct fdisk_label_operations gpt_operations =
 	.locate		= gpt_locate_disklabel,
 	.get_item	= gpt_get_disklabel_item,
 	.set_id		= gpt_set_disklabel_id,
+	.get_length	= gpt_get_disklabel_length,
+	.set_length	= gpt_set_disklabel_length,
 
 	.get_part	= gpt_get_partition,
 	.set_part	= gpt_set_partition,
diff --git a/libfdisk/src/label.c b/libfdisk/src/label.c
index ee5ee3e..ec2e5c5 100644
--- a/libfdisk/src/label.c
+++ b/libfdisk/src/label.c
@@ -523,6 +523,40 @@ int fdisk_set_disklabel_id(struct fdisk_context *cxt)
 }
 
 /**
+ * fdisk_get_disklabel_length:
+ * @cxt: fdisk context
+ *
+ * Returns: 0 on success, otherwise, a corresponding error.
+ */
+unsigned long fdisk_get_disklabel_length(struct fdisk_context *cxt)
+{
+	if (!cxt || !cxt->label)
+		return -EINVAL;
+	if (!cxt->label->op->get_length)
+		return -ENOSYS;
+
+	DBG(CXT, ul_debugobj(cxt, "getting %s table length", cxt->label->name));
+	return cxt->label->op->get_length(cxt);
+}
+
+/**
+ * fdisk_set_disklabel_length:
+ * @cxt: fdisk context
+ *
+ * Returns: 0 on success, otherwise, a corresponding error.
+ */
+int fdisk_set_disklabel_length(struct fdisk_context *cxt, unsigned long entries)
+{
+	if (!cxt || !cxt->label)
+		return -EINVAL;
+	if (!cxt->label->op->set_length)
+		return -ENOSYS;
+
+	DBG(CXT, ul_debugobj(cxt, "setting %s label length", cxt->label->name));
+	return cxt->label->op->set_length(cxt, entries);
+}
+
+/**
  * fdisk_set_partition_type:
  * @cxt: fdisk context
  * @partnum: partition number
diff --git a/libfdisk/src/libfdisk.h.in b/libfdisk/src/libfdisk.h.in
index 6bd3a1e..71405a2 100644
--- a/libfdisk/src/libfdisk.h.in
+++ b/libfdisk/src/libfdisk.h.in
@@ -320,6 +320,8 @@ enum fdisk_labelitem_gen {
 extern int fdisk_get_disklabel_item(struct fdisk_context *cxt, int id, struct fdisk_labelitem *item);
 extern int fdisk_get_disklabel_id(struct fdisk_context *cxt, char **id);
 extern int fdisk_set_disklabel_id(struct fdisk_context *cxt);
+extern int fdisk_set_disklabel_length(struct fdisk_context *cxt, unsigned long entries);
+extern unsigned long fdisk_get_disklabel_length(struct fdisk_context *cxt);
 
 extern int fdisk_get_partition(struct fdisk_context *cxt, size_t partno, struct fdisk_partition **pa);
 extern int fdisk_set_partition(struct fdisk_context *cxt, size_t partno, struct fdisk_partition *pa);
diff --git a/libfdisk/src/libfdisk.sym b/libfdisk/src/libfdisk.sym
index c5aeac6..6ecec44 100644
--- a/libfdisk/src/libfdisk.sym
+++ b/libfdisk/src/libfdisk.sym
@@ -64,6 +64,7 @@ global:
 	fdisk_get_devfd;
 	fdisk_get_devname;
 	fdisk_get_disklabel_id;
+	fdisk_get_disklabel_length;
 	fdisk_get_first_lba;
 	fdisk_get_freespaces;
 	fdisk_get_geom_cylinders;
@@ -202,6 +203,7 @@ global:
 	fdisk_script_write_file;
 	fdisk_set_ask;
 	fdisk_set_disklabel_id;
+	fdisk_set_disklabel_length;
 	fdisk_set_first_lba;
 	fdisk_set_last_lba;
 	fdisk_set_partition;
-- 
2.1.0

[PATCH 2/3] fdisk: Add support for altering GPT size

[Sassan Panahinejad]

Adds an options (l) to the GPT menu to resize the GPT.
---
 disk-utils/fdisk-menu.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/disk-utils/fdisk-menu.c b/disk-utils/fdisk-menu.c
index b099cc3..21cde24 100644
--- a/disk-utils/fdisk-menu.c
+++ b/disk-utils/fdisk-menu.c
@@ -165,6 +165,7 @@ struct menu menu_gpt = {
 		MENU_XENT('i', N_("change disk GUID")),
 		MENU_XENT('n', N_("change partition name")),
 		MENU_XENT('u', N_("change partition UUID")),
+		MENU_XENT('l', N_("change table length")),
 		MENU_XENT('M', N_("enter protective/hybrid MBR")),
 
 		MENU_XSEP(""),
@@ -691,6 +692,7 @@ static int gpt_menu_cb(struct fdisk_context **cxt0,
 	struct fdisk_partition *pa = NULL;
 	size_t n;
 	int rc = 0;
+	unsigned long length;
 
 	assert(cxt);
 	assert(ent);
@@ -702,6 +704,12 @@ static int gpt_menu_cb(struct fdisk_context **cxt0,
 		switch (ent->key) {
 		case 'i':
 			return fdisk_set_disklabel_id(cxt);
+		case 'l':
+	                rc =  fdisk_ask_number(cxt, 1, fdisk_get_disklabel_length(cxt),
+	                                ~0UL, _("New maximum entries"), &length);
+			if (rc)
+				return rc;
+			return fdisk_set_disklabel_length(cxt, length);
 		case 'M':
 			mbr = fdisk_new_nested_context(cxt, "dos");
 			if (!mbr)
-- 
2.1.0

[PATCH 3/3] sfdisk: Add support for altering GPT size

[Sassan Panahinejad]

Adds a header option to alter the GPT table length
---
 libfdisk/src/script.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/libfdisk/src/script.c b/libfdisk/src/script.c
index 096f37c..e6fd0de 100644
--- a/libfdisk/src/script.c
+++ b/libfdisk/src/script.c
@@ -705,7 +705,8 @@ static int parse_line_header(struct fdisk_script *dp, char *s)
 	} else if (strcmp(name, "label-id") == 0
 		   || strcmp(name, "device") == 0
 		   || strcmp(name, "first-lba") == 0
-		   || strcmp(name, "last-lba") == 0) {
+		   || strcmp(name, "last-lba") == 0
+		   || strcmp(name, "table-length") == 0) {
 		;					/* whatever is posssible */
 	} else
 		goto done;				/* unknown header */
@@ -1326,6 +1327,8 @@ struct fdisk_script *fdisk_get_script(struct fdisk_context *cxt)
 int fdisk_apply_script_headers(struct fdisk_context *cxt, struct fdisk_script *dp)
 {
 	const char *name;
+	const char *str;
+	int rc;
 
 	assert(cxt);
 	assert(dp);
@@ -1338,7 +1341,15 @@ int fdisk_apply_script_headers(struct fdisk_context *cxt, struct fdisk_script *d
 	if (!name)
 		return -EINVAL;
 
-	return fdisk_create_disklabel(cxt, name);
+	rc = fdisk_create_disklabel(cxt, name);
+	if (rc)
+		return rc;
+
+	str = fdisk_script_get_header(dp, "table-length");
+	if (str)
+		return fdisk_set_disklabel_length(cxt, strtoul(str, NULL, 0));
+
+	return 0;
 }
 
 /**
-- 
2.1.0

Re: [PATCH 1/3] libfdisk: Add support for altering GPT size

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 1201 bytes --]

On 11 May 2016 15:54, Sassan Panahinejad wrote:
> This is useful in two situations:
> 
> 1. More than 128 partitions are required. Or
> 
> 2. The partition table must be restricted in size, such as when a system
> expects to find a bootloader at a location that would otherwise overlap the
> partition table.
> 
> The gdisk partitioner supports this feature.
> 
> libfdisk is already capable of reading and writing partition tables of any
> size, but previously could only create ones of 128 entries and could not
> resize.
> 
> This change should be fairly safe, as it has no effect unless explicitly
> activated.

i'm not super familiar with the codebase, but these look pretty nice.  thanks!

> +	fdisk_info(cxt, _("Partition table length changed from %u to %u."), old, new);

old & new are unsigned long, so you'll want %lu

> +int fdisk_set_disklabel_length(struct fdisk_context *cxt, unsigned long entries)
> +{
> +	if (!cxt || !cxt->label)
> +		return -EINVAL;
> +	if (!cxt->label->op->set_length)
> +		return -ENOSYS;
> +
> +	DBG(CXT, ul_debugobj(cxt, "setting %s label length", cxt->label->name));

want to include |entries| in the debug output too ?
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/3] libfdisk: Add support for altering GPT size

[Sassan Panahinejad]

Thanks Mike. I'll attend to those issues in the morning and resubmit.

On 11 May 2016 at 17:39, Mike Frysinger <vapier@gentoo.org> wrote:
> On 11 May 2016 15:54, Sassan Panahinejad wrote:
>> This is useful in two situations:
>>
>> 1. More than 128 partitions are required. Or
>>
>> 2. The partition table must be restricted in size, such as when a system
>> expects to find a bootloader at a location that would otherwise overlap the
>> partition table.
>>
>> The gdisk partitioner supports this feature.
>>
>> libfdisk is already capable of reading and writing partition tables of any
>> size, but previously could only create ones of 128 entries and could not
>> resize.
>>
>> This change should be fairly safe, as it has no effect unless explicitly
>> activated.
>
> i'm not super familiar with the codebase, but these look pretty nice.  thanks!
>
>> +     fdisk_info(cxt, _("Partition table length changed from %u to %u."), old, new);
>
> old & new are unsigned long, so you'll want %lu
>
>> +int fdisk_set_disklabel_length(struct fdisk_context *cxt, unsigned long entries)
>> +{
>> +     if (!cxt || !cxt->label)
>> +             return -EINVAL;
>> +     if (!cxt->label->op->set_length)
>> +             return -ENOSYS;
>> +
>> +     DBG(CXT, ul_debugobj(cxt, "setting %s label length", cxt->label->name));
>
> want to include |entries| in the debug output too ?
> -mike

Re: [PATCH 1/3] libfdisk: Add support for altering GPT size

[Karel Zak]

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@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 1/3] libfdisk: Add support for altering GPT size

[Sassan Panahinejad]

Hi Karel,

Thanks for the feedback.

I think you're right that this is specific to GPT, I don't know of any
others that have variable size.

I'll fix the problems listed for this patch and modify the other
patches to use fdisk_gpt_set_npartitions() and
fdisk_get_npartitions().

On 12 May 2016 at 10:43, Karel Zak <kzak@redhat.com> wrote:
> 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@redhat.com>
>  http://karelzak.blogspot.com

Re: [PATCH 1/3] libfdisk: Add support for altering GPT size

[Sassan Panahinejad]

Hi Karel,

I've submitted updated patches. Please see v2 for libfdisk and sfdisk
and v3 fdisk patch.

On 12 May 2016 at 10:43, Karel Zak <kzak@redhat.com> wrote:
> 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@redhat.com>
>  http://karelzak.blogspot.com

Re: [PATCH 1/3] libfdisk: Add support for altering GPT size

[Karel Zak]

On Fri, May 13, 2016 at 11:44:18AM +0100, Sassan Panahinejad wrote:
> Hi Karel,
> 
> I've submitted updated patches. Please see v2 for libfdisk and sfdisk
> and v3 fdisk patch.

 Seem OK, I'll merge it next week. Thanks.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 1/3] libfdisk: Add support for altering GPT size

[Sassan Panahinejad]

Great! Cheers!

On 13 May 2016 at 14:48, Karel Zak <kzak@redhat.com> wrote:
> On Fri, May 13, 2016 at 11:44:18AM +0100, Sassan Panahinejad wrote:
>> Hi Karel,
>>
>> I've submitted updated patches. Please see v2 for libfdisk and sfdisk
>> and v3 fdisk patch.
>
>  Seem OK, I'll merge it next week. Thanks.
>
>     Karel
>
> --
>  Karel Zak  <kzak@redhat.com>
>  http://karelzak.blogspot.com