[1/2] diskpart_handler: support for GPT PARTUUIDs

It is already possible to specify PARTLABELS (using `name=`) in
SWUpdate's diskpart_handler.

This commit makes it possible to explicitly provide the PARTUUID by
setting `partuuid=` in the respective partition's property section.
If omitted, the current behavior of generating an automatic PARTUUID is
preserved, i.e. this is a backwards compatible extension.

An example use-case is to mount a partition by its PARTUUID, i.e. by
mounting /dev/disk/by-partuuid/<PARTUUID> (which is typically a symlink
created by udev). On real hardware, the symlink might point to
/dev/nvme0n1p1, whereas inside a VM, it could refer to /dev/vda1.
Thus, by using PARTUUID, the same mount command can be used in both
settings. This is useful because - unlike the disk UUID - the PARTUUID
can be explicitly set.

Signed-off-by: Michael Adler <michael.adler@siemens.com>
Signed-off-by: Christian Storm <christian.storm@siemens.com>
---
 doc/source/handlers.rst     |  3 ++
 handlers/diskpart_handler.c | 81 ++++++++++++++++++++++---------------
 2 files changed, 52 insertions(+), 32 deletions(-)

Hallo Michael,

On 05.11.21 09:38, Michael Adler wrote:
> It is already possible to specify PARTLABELS (using `name=`) in
> SWUpdate's diskpart_handler.
> 
> This commit makes it possible to explicitly provide the PARTUUID by
> setting `partuuid=` in the respective partition's property section.
> If omitted, the current behavior of generating an automatic PARTUUID is
> preserved, i.e. this is a backwards compatible extension.
> 

Fully agree.

> An example use-case is to mount a partition by its PARTUUID, i.e. by
> mounting /dev/disk/by-partuuid/<PARTUUID> (which is typically a symlink
> created by udev). On real hardware, the symlink might point to
> /dev/nvme0n1p1, whereas inside a VM, it could refer to /dev/vda1.
> Thus, by using PARTUUID, the same mount command can be used in both
> settings. This is useful because - unlike the disk UUID - the PARTUUID
> can be explicitly set.

Right.

> 
> Signed-off-by: Michael Adler <michael.adler@siemens.com>
> Signed-off-by: Christian Storm <christian.storm@siemens.com>
> ---
>   doc/source/handlers.rst     |  3 ++
>   handlers/diskpart_handler.c | 81 ++++++++++++++++++++++---------------
>   2 files changed, 52 insertions(+), 32 deletions(-)
> 
> diff --git a/doc/source/handlers.rst b/doc/source/handlers.rst
> index ed811e3..f9c771b 100644
> --- a/doc/source/handlers.rst
> +++ b/doc/source/handlers.rst
> @@ -844,6 +844,9 @@ supported:
>      |             |          | will be created on the corresponding partition.    |
>      |             |          | vfat / ext2 / ext3 /ext4 file system is supported  |
>      +-------------+----------+----------------------------------------------------+
> +   | partuuid    | string   | The partition UUID (GPT only). If omitted, a UUID  |
> +   |             |          | will be generated automatically.			 |
> +   +-------------+----------+----------------------------------------------------+
>   
>   
>   
> diff --git a/handlers/diskpart_handler.c b/handlers/diskpart_handler.c
> index e08c10e..21efc8f 100644
> --- a/handlers/diskpart_handler.c
> +++ b/handlers/diskpart_handler.c
> @@ -17,6 +17,7 @@
>   #include <sys/types.h>
>   #include <libfdisk/libfdisk.h>
>   #include <fs_interface.h>
> +#include <uuid/uuid.h>
>   #include "swupdate.h"
>   #include "handler.h"
>   #include "util.h"
> @@ -50,7 +51,8 @@ enum partfield {
>   	PART_TYPE,
>   	PART_NAME,
>   	PART_FSTYPE,
> -	PART_DOSTYPE
> +	PART_DOSTYPE,
> +	PART_UUID,
>   };
>   
>   const char *fields[] = {
> @@ -59,7 +61,8 @@ const char *fields[] = {
>   	[PART_TYPE] = "type",
>   	[PART_NAME] = "name",
>   	[PART_FSTYPE] = "fstype",
> -	[PART_DOSTYPE] = "dostype"
> +	[PART_DOSTYPE] = "dostype",
> +	[PART_UUID] = "partuuid",
>   };
>   
>   struct partition_data {
> @@ -70,6 +73,7 @@ struct partition_data {
>   	char name[SWUPDATE_GENERAL_STRING_SIZE];
>   	char fstype[SWUPDATE_GENERAL_STRING_SIZE];
>   	char dostype[SWUPDATE_GENERAL_STRING_SIZE];
> +	char partuuid[UUID_STR_LEN];
>   	LIST_ENTRY(partition_data) next;
>   };
>   LIST_HEAD(listparts, partition_data);
> @@ -308,6 +312,9 @@ static int diskpart_set_partition(struct fdisk_partition *pa,
>   	if (parttype)
>   		ret |= fdisk_partition_set_type(pa, parttype);
>   
> +	if (strlen(part->partuuid))
> +	      ret |= fdisk_partition_set_uuid(pa, part->partuuid);
> +

I have supposed that the new feature should just make this call, however....

>   	return ret;
>   }
>   
> @@ -424,41 +431,44 @@ static void diskpart_partition_info(struct fdisk_context *cxt, const char *name,
>   /*
>    * Return true if partition differs
>    */
> -static bool diskpart_partition_cmp(struct fdisk_context *cxt, struct fdisk_partition *firstpa,
> -		struct fdisk_partition *secondpa)
> +static bool diskpart_partition_cmp(struct fdisk_partition *firstpa, struct fdisk_partition *secondpa)
>   {
> -	struct fdisk_parttype *type;
> -	const char *lbtype;
> -
>   	if (!firstpa || !secondpa)
>   		return true;
>   
> -	type = fdisk_partition_get_type(firstpa);
> -	if (!type)
> +	if (fdisk_partition_cmp_partno(firstpa, secondpa) ||
> +		(!fdisk_partition_start_is_default(firstpa) && !fdisk_partition_start_is_default(secondpa) &&
> +			fdisk_partition_cmp_start(firstpa, secondpa)) ||
> +		fdisk_partition_get_size(firstpa) != fdisk_partition_get_size(secondpa)) {
>   		return true;
> +	}
>   
> -	if (fdisk_parttype_get_string(type))
> -		lbtype = "gpt";
> -	else
> -		lbtype = "dos";
> -
> -	if (firstpa && secondpa && (fdisk_partition_cmp_partno(firstpa, secondpa) ||
> -		(!fdisk_partition_start_is_default(firstpa) && !fdisk_partition_start_is_default(secondpa) &&
> -		fdisk_partition_cmp_start(firstpa, secondpa)) ||
> -		(!strcmp(lbtype, "gpt") &&
> -			(strcmp(fdisk_parttype_get_string(fdisk_partition_get_type(firstpa)),
> -				fdisk_parttype_get_string(fdisk_partition_get_type(secondpa))) ||
> -			strcmp(fdisk_partition_get_name(firstpa) ? fdisk_partition_get_name(firstpa) : "",
> -		       		fdisk_partition_get_name(secondpa) ? fdisk_partition_get_name(secondpa) : ""))) ||
> -		(!strcmp(lbtype, "dos") &&
> -			fdisk_parttype_get_code(fdisk_partition_get_type(firstpa)) !=
> -			fdisk_parttype_get_code(fdisk_partition_get_type(secondpa))) ||
> -		fdisk_partition_get_size(firstpa) != fdisk_partition_get_size(secondpa))) {
> -		TRACE("Partition differ:");
> -		diskpart_partition_info(cxt, "Original", firstpa);
> -		diskpart_partition_info(cxt, "New", secondpa);
> +	struct fdisk_parttype *firstpa_type = fdisk_partition_get_type(firstpa);
> +	if (!firstpa_type)
>   		return true;
> +	struct fdisk_parttype *secondpa_type = fdisk_partition_get_type(secondpa);
> +
> +	if (fdisk_parttype_get_string(firstpa_type)) {
> +		/* gpt */
> +		const char *firstpa_name = fdisk_partition_get_name(firstpa);
> +		const char *secondpa_name = fdisk_partition_get_name(secondpa);
> +		if ((secondpa_type && strcmp(fdisk_parttype_get_string(firstpa_type), fdisk_parttype_get_string(secondpa_type))) ||
> +			strcmp(firstpa_name ? firstpa_name : "", secondpa_name ? secondpa_name : "")) {
> +			return true;
> +		}
> +
> +		const char *firstpa_uuid = fdisk_partition_get_uuid(firstpa);
> +		const char *secondpa_uuid = fdisk_partition_get_uuid(secondpa);
> +		if (firstpa_uuid && secondpa_uuid && strcmp(firstpa_uuid, secondpa_uuid)) {
> +			return true;
> +		}
> +	} else {
> +		/* dos */
> +		if (fdisk_parttype_get_code(firstpa_type) != fdisk_parttype_get_code(secondpa_type)) {
> +			return true;
> +		}
>   	}
> +

...it is not clear to me how the changes above have something to do with 
PARTUUID, or which issue should be fixed (and in any case, this should 
be done in a separate patch). Can you explain this ?

Regards,
Stefano

>   	return false;
>   }
>   
> @@ -610,7 +620,10 @@ static int diskpart_table_cmp(struct fdisk_context *cxt, struct fdisk_table *tb,
>   				fdisk_table_next_partition (oldtb, olditr, &pa)) {
>   				TRACE("Partition not found, something went wrong %lu !", i);
>   				ret = -EFAULT;
> -			} else if (diskpart_partition_cmp(cxt, pa, newpa)) {
> +			} else if (diskpart_partition_cmp(pa, newpa)) {
> +				TRACE("Partition differ:");
> +				diskpart_partition_info(cxt, "Original", pa);
> +				diskpart_partition_info(cxt, "New", newpa);
>   				ret = 1;
>   			}
>   
> @@ -845,6 +858,9 @@ static int diskpart(struct img_type *img,
>   						strncpy(part->dostype, equal, sizeof(part->dostype));
>   						hybrid++;
>   						break;
> +					case PART_UUID:
> +						strncpy(part->partuuid, equal, sizeof(part->partuuid));
> +						break;
>   					}
>   				}
>   			}
> @@ -864,12 +880,13 @@ static int diskpart(struct img_type *img,
>   			goto handler_exit;
>   		}
>   
> -		TRACE("partition-%zu:%s size %" PRIu64 " start %zu type %s",
> +		TRACE("partition-%zu:%s size %" PRIu64 " start %zu type %s partuuid %s",
>   			part->partno != LIBFDISK_INIT_UNDEF(part->partno) ? part->partno : 0,
>   			strlen(part->name) ? part->name : "UNDEF NAME",
>   			part->size != LIBFDISK_INIT_UNDEF(part->size) ? part->size : 0,
>   			part->start!= LIBFDISK_INIT_UNDEF(part->start) ? part->start : 0,
> -			part->type);
> +			part->type,
> +			strlen(part->partuuid) ? part->partuuid : "automatic");
>   
>   		/*
>   		 * Partitions in sw-description start from 1,
>

Hi Stefano,

> ...it is not clear to me how the changes above have something to do with
> PARTUUID, or which issue should be fixed (and in any case, this should be
> done in a separate patch). Can you explain this ?

the check which compares the existing GPT (on the device) with the GPT described in sw-description needs to be extended
as well. For example, let's assume that previously I did not set an explicit PARTUUID for the devices and thus a random
one has been generated. But now I want all my devices to have the same PARTUUID. diskpart_partition_cmp is used to
determine if the partition tables are different, therefore I had to extend it and check for different PARTUUIDS.

I could have inserted another || clause in the convoluted if-construct but since it's quite hard to read (and rather
inefficient) I decided to simplify it. If you agree with the simplification, I can split the commit and do the
simplification in one step and the additional PARTUUID check in another commit.

Kind regards,
Michael

Hi Michael,

On 18.11.21 11:07, Michael Adler wrote:
> Hi Stefano,
> 
>> ...it is not clear to me how the changes above have something to do with
>> PARTUUID, or which issue should be fixed (and in any case, this should be
>> done in a separate patch). Can you explain this ?
> 
> the check which compares the existing GPT (on the device) with the GPT described in sw-description needs to be extended
> as well. For example, let's assume that previously I did not set an explicit PARTUUID for the devices and thus a random
> one has been generated. But now I want all my devices to have the same PARTUUID. diskpart_partition_cmp is used to
> determine if the partition tables are different, therefore I had to extend it and check for different PARTUUIDS.
> 

This is clear.

> I could have inserted another || clause in the convoluted if-construct but since it's quite hard to read

maybe yes

> (and rather
> inefficient)

Why inefficient ?

> I decided to simplify it.

At first glance, it appeared to me that different checks are done. I 
will deeply reviewed it.

> If you agree with the simplification, I can split the commit and do the
> simplification in one step and the additional PARTUUID check in another commit.
> 

Best regards,
Stefano

> Kind regards,
> Michael
>

Hi Michael,

On 05.11.21 09:38, Michael Adler wrote:
> It is already possible to specify PARTLABELS (using `name=`) in
> SWUpdate's diskpart_handler.
> 
> This commit makes it possible to explicitly provide the PARTUUID by
> setting `partuuid=` in the respective partition's property section.
> If omitted, the current behavior of generating an automatic PARTUUID is
> preserved, i.e. this is a backwards compatible extension.
> 
> An example use-case is to mount a partition by its PARTUUID, i.e. by
> mounting /dev/disk/by-partuuid/<PARTUUID> (which is typically a symlink
> created by udev). On real hardware, the symlink might point to
> /dev/nvme0n1p1, whereas inside a VM, it could refer to /dev/vda1.
> Thus, by using PARTUUID, the same mount command can be used in both
> settings. This is useful because - unlike the disk UUID - the PARTUUID
> can be explicitly set.
> 
> Signed-off-by: Michael Adler <michael.adler@siemens.com>
> Signed-off-by: Christian Storm <christian.storm@siemens.com>
> ---
>   doc/source/handlers.rst     |  3 ++
>   handlers/diskpart_handler.c | 81 ++++++++++++++++++++++---------------
>   2 files changed, 52 insertions(+), 32 deletions(-)
> 
> diff --git a/doc/source/handlers.rst b/doc/source/handlers.rst
> index ed811e3..f9c771b 100644
> --- a/doc/source/handlers.rst
> +++ b/doc/source/handlers.rst
> @@ -844,6 +844,9 @@ supported:
>      |             |          | will be created on the corresponding partition.    |
>      |             |          | vfat / ext2 / ext3 /ext4 file system is supported  |
>      +-------------+----------+----------------------------------------------------+
> +   | partuuid    | string   | The partition UUID (GPT only). If omitted, a UUID  |
> +   |             |          | will be generated automatically.			 |
> +   +-------------+----------+----------------------------------------------------+
>   
>   
>   
> diff --git a/handlers/diskpart_handler.c b/handlers/diskpart_handler.c
> index e08c10e..21efc8f 100644
> --- a/handlers/diskpart_handler.c
> +++ b/handlers/diskpart_handler.c
> @@ -17,6 +17,7 @@
>   #include <sys/types.h>
>   #include <libfdisk/libfdisk.h>
>   #include <fs_interface.h>
> +#include <uuid/uuid.h>
>   #include "swupdate.h"
>   #include "handler.h"
>   #include "util.h"
> @@ -50,7 +51,8 @@ enum partfield {
>   	PART_TYPE,
>   	PART_NAME,
>   	PART_FSTYPE,
> -	PART_DOSTYPE
> +	PART_DOSTYPE,
> +	PART_UUID,
>   };
>   
>   const char *fields[] = {
> @@ -59,7 +61,8 @@ const char *fields[] = {
>   	[PART_TYPE] = "type",
>   	[PART_NAME] = "name",
>   	[PART_FSTYPE] = "fstype",
> -	[PART_DOSTYPE] = "dostype"
> +	[PART_DOSTYPE] = "dostype",
> +	[PART_UUID] = "partuuid",
>   };
>   
>   struct partition_data {
> @@ -70,6 +73,7 @@ struct partition_data {
>   	char name[SWUPDATE_GENERAL_STRING_SIZE];
>   	char fstype[SWUPDATE_GENERAL_STRING_SIZE];
>   	char dostype[SWUPDATE_GENERAL_STRING_SIZE];
> +	char partuuid[UUID_STR_LEN];
>   	LIST_ENTRY(partition_data) next;
>   };
>   LIST_HEAD(listparts, partition_data);
> @@ -308,6 +312,9 @@ static int diskpart_set_partition(struct fdisk_partition *pa,
>   	if (parttype)
>   		ret |= fdisk_partition_set_type(pa, parttype);
>   
> +	if (strlen(part->partuuid))
> +	      ret |= fdisk_partition_set_uuid(pa, part->partuuid);
> +
>   	return ret;
>   }
>   
> @@ -424,41 +431,44 @@ static void diskpart_partition_info(struct fdisk_context *cxt, const char *name,
>   /*
>    * Return true if partition differs
>    */
> -static bool diskpart_partition_cmp(struct fdisk_context *cxt, struct fdisk_partition *firstpa,
> -		struct fdisk_partition *secondpa)
> +static bool diskpart_partition_cmp(struct fdisk_partition *firstpa, struct fdisk_partition *secondpa)
>   {
> -	struct fdisk_parttype *type;
> -	const char *lbtype;
> -
>   	if (!firstpa || !secondpa)
>   		return true;
>   
> -	type = fdisk_partition_get_type(firstpa);
> -	if (!type)
> +	if (fdisk_partition_cmp_partno(firstpa, secondpa) ||

fdisk_partition_cmp_partno() is called, but check if the pointers are 
valid was removed. What does it happen ny passing NULL to 
fdisk_partition_cmp_partno ?

> +		(!fdisk_partition_start_is_default(firstpa) && !fdisk_partition_start_is_default(secondpa) &&
> +			fdisk_partition_cmp_start(firstpa, secondpa)) ||
> +		fdisk_partition_get_size(firstpa) != fdisk_partition_get_size(secondpa)) {
>   		return true;
> +	}
>   
> -	if (fdisk_parttype_get_string(type))
> -		lbtype = "gpt";
> -	else
> -		lbtype = "dos";
> -
> -	if (firstpa && secondpa && (fdisk_partition_cmp_partno(firstpa, secondpa) ||
> -		(!fdisk_partition_start_is_default(firstpa) && !fdisk_partition_start_is_default(secondpa) &&
> -		fdisk_partition_cmp_start(firstpa, secondpa)) ||
> -		(!strcmp(lbtype, "gpt") &&
> -			(strcmp(fdisk_parttype_get_string(fdisk_partition_get_type(firstpa)),
> -				fdisk_parttype_get_string(fdisk_partition_get_type(secondpa))) ||
> -			strcmp(fdisk_partition_get_name(firstpa) ? fdisk_partition_get_name(firstpa) : "",
> -		       		fdisk_partition_get_name(secondpa) ? fdisk_partition_get_name(secondpa) : ""))) ||
> -		(!strcmp(lbtype, "dos") &&
> -			fdisk_parttype_get_code(fdisk_partition_get_type(firstpa)) !=
> -			fdisk_parttype_get_code(fdisk_partition_get_type(secondpa))) ||
> -		fdisk_partition_get_size(firstpa) != fdisk_partition_get_size(secondpa))) {
> -		TRACE("Partition differ:");
> -		diskpart_partition_info(cxt, "Original", firstpa);
> -		diskpart_partition_info(cxt, "New", secondpa);
> +	struct fdisk_parttype *firstpa_type = fdisk_partition_get_type(firstpa);
> +	if (!firstpa_type)
>   		return true;
> +	struct fdisk_parttype *secondpa_type = fdisk_partition_get_type(secondpa);
> +
> +	if (fdisk_parttype_get_string(firstpa_type)) {

Documentation for libfdisk is scarce, but it seems to me that this has 
meaning only in case of GPT. In case of MBR, it could (just because I do 
not see if it is set) mreturns NULL, but it is undocumented in libfdisk. 
But you dropped the check for GPT. Do we rely that 
fdisk_parttype_get_string() returns a not null just in case of GPT ? 
Where is it documented ?

In libfdisk, there is no check about type as well, but it just returns 
the pointer. fdisk_parttype_set_typestr() does not check as well, and 
comment to the function says "Don't use this function for MBR". The way 
thought by libfdisk developers is to check for type, and then access to 
the right set of funtions (as done before your patch). I understand that 
behavior is unpredictable if the wrong set is used.


> +		/* gpt */
> +		const char *firstpa_name = fdisk_partition_get_name(firstpa);
> +		const char *secondpa_name = fdisk_partition_get_name(secondpa);
> +		if ((secondpa_type && strcmp(fdisk_parttype_get_string(firstpa_type), fdisk_parttype_get_string(secondpa_type))) ||
> +			strcmp(firstpa_name ? firstpa_name : "", secondpa_name ? secondpa_name : "")) {
> +			return true;
> +		}
> +
> +		const char *firstpa_uuid = fdisk_partition_get_uuid(firstpa);
> +		const char *secondpa_uuid = fdisk_partition_get_uuid(secondpa);
> +		if (firstpa_uuid && secondpa_uuid && strcmp(firstpa_uuid, secondpa_uuid)) {
> +			return true;

But this means: if one of firstpa_uuid or secondpa_uuid is not set, it 
the check is skipped and it return false (that is, they are identical). 
It looks wrong.


> +		}
> +	} else {
> +		/* dos */
> +		if (fdisk_parttype_get_code(firstpa_type) != fdisk_parttype_get_code(secondpa_type)) {
> +			return true;
> +		}

Ok, and now where is compared the size ?

>   	}
> +
>   	return false;
>   }
>   
> @@ -610,7 +620,10 @@ static int diskpart_table_cmp(struct fdisk_context *cxt, struct fdisk_table *tb,
>   				fdisk_table_next_partition (oldtb, olditr, &pa)) {
>   				TRACE("Partition not found, something went wrong %lu !", i);
>   				ret = -EFAULT;
> -			} else if (diskpart_partition_cmp(cxt, pa, newpa)) {
> +			} else if (diskpart_partition_cmp(pa, newpa)) {
> +				TRACE("Partition differ:");
> +				diskpart_partition_info(cxt, "Original", pa);
> +				diskpart_partition_info(cxt, "New", newpa);
>   				ret = 1;
>   			}
>   
> @@ -845,6 +858,9 @@ static int diskpart(struct img_type *img,
>   						strncpy(part->dostype, equal, sizeof(part->dostype));
>   						hybrid++;
>   						break;
> +					case PART_UUID:
> +						strncpy(part->partuuid, equal, sizeof(part->partuuid));
> +						break;
>   					}
>   				}
>   			}
> @@ -864,12 +880,13 @@ static int diskpart(struct img_type *img,
>   			goto handler_exit;
>   		}
>   
> -		TRACE("partition-%zu:%s size %" PRIu64 " start %zu type %s",
> +		TRACE("partition-%zu:%s size %" PRIu64 " start %zu type %s partuuid %s",
>   			part->partno != LIBFDISK_INIT_UNDEF(part->partno) ? part->partno : 0,
>   			strlen(part->name) ? part->name : "UNDEF NAME",
>   			part->size != LIBFDISK_INIT_UNDEF(part->size) ? part->size : 0,
>   			part->start!= LIBFDISK_INIT_UNDEF(part->start) ? part->start : 0,
> -			part->type);
> +			part->type,
> +			strlen(part->partuuid) ? part->partuuid : "automatic");
>   
>   		/*
>   		 * Partitions in sw-description start from 1,
> 

Best regards,
Stefano

> Why inefficient ?

Nothing major and the compiler should optimize most of it away:

* the string variable lbtype is introduced and subsequently used with strcmp
		=> no need to use a string for this (int/enum would suffice)
		=> in fact, there is no need to introduce a variable at all if you break up the if-clause like I did
* at other times, the result of a function call is not cached, e.g.
			strcmp(fdisk_partition_get_name(firstpa) ? fdisk_partition_get_name(firstpa)
		(this happens all over the place in the if-clause)

Like I said, nothing major, it's probably more about clean code than performance here.

> At first glance, it appeared to me that different checks are done. I will
> deeply reviewed it.

Thanks, appreciated! It _should_ behave the same.

> >   	if (!firstpa || !secondpa)
> >   		return true;
> > -	type = fdisk_partition_get_type(firstpa);
> > -	if (!type)
> > +	if (fdisk_partition_cmp_partno(firstpa, secondpa) ||
> 
> fdisk_partition_cmp_partno() is called, but check if the pointers are valid
> was removed. What does it happen ny passing NULL to
> fdisk_partition_cmp_partno ?

There is a NULL check a few lines earlier:

> >   	if (!firstpa || !secondpa)
> >   		return true;


> > +	if (fdisk_parttype_get_string(firstpa_type)) {
> 
> Documentation for libfdisk is scarce, but it seems to me that this has
> meaning only in case of GPT. In case of MBR, it could (just because I do not
> see if it is set) mreturns NULL, but it is undocumented in libfdisk. But you
> dropped the check for GPT. Do we rely that fdisk_parttype_get_string()
> returns a not null just in case of GPT ? Where is it documented ?

I didn't change the logic. Previously, we had:

```
  if (fdisk_parttype_get_string(type))
 		lbtype = "gpt";
 	else
 		lbtype = "dos";

	...

  (!strcmp(lbtype, "gpt") &&
			(strcmp(fdisk_parttype_get_string(fdisk_partition_get_type(firstpa)),
				fdisk_parttype_get_string(fdisk_partition_get_type(secondpa))) ||
```

So basically I inlined this and got rid of the lbtype (string!) variable.

> But this means: if one of firstpa_uuid or secondpa_uuid is not set, it the
> check is skipped and it return false (that is, they are identical). It looks
> wrong.

firstpa_uuid is always set in case of GPT. Either it has been generated randomly or it has been explicitly set by us.
Just in case there is an extra NULL check. The thing is: secondpa_uuid is the value provided in sw-description. If the
value is missing in sw-description, it means we do not care about the PARTUUID and in particular, we do not want to
modify the existing (random) value. The idea is to avoid modifying the GPT unless we really have to.

> Ok, and now where is compared the size ?

The size check is at the beginning of the function (line 442 after applying the patch).

I hope I didn't miss any question. Please let me know if I did or something else is unclear.

Michael

On 05.11.21 09:38, Michael Adler wrote:
> It is already possible to specify PARTLABELS (using `name=`) in
> SWUpdate's diskpart_handler.
> 
> This commit makes it possible to explicitly provide the PARTUUID by
> setting `partuuid=` in the respective partition's property section.
> If omitted, the current behavior of generating an automatic PARTUUID is
> preserved, i.e. this is a backwards compatible extension.
> 
> An example use-case is to mount a partition by its PARTUUID, i.e. by
> mounting /dev/disk/by-partuuid/<PARTUUID> (which is typically a symlink
> created by udev). On real hardware, the symlink might point to
> /dev/nvme0n1p1, whereas inside a VM, it could refer to /dev/vda1.
> Thus, by using PARTUUID, the same mount command can be used in both
> settings. This is useful because - unlike the disk UUID - the PARTUUID
> can be explicitly set.
> 
> Signed-off-by: Michael Adler <michael.adler@siemens.com>
> Signed-off-by: Christian Storm <christian.storm@siemens.com>
> ---
>   doc/source/handlers.rst     |  3 ++
>   handlers/diskpart_handler.c | 81 ++++++++++++++++++++++---------------
>   2 files changed, 52 insertions(+), 32 deletions(-)
> 
> diff --git a/doc/source/handlers.rst b/doc/source/handlers.rst
> index ed811e3..f9c771b 100644
> --- a/doc/source/handlers.rst
> +++ b/doc/source/handlers.rst
> @@ -844,6 +844,9 @@ supported:
>      |             |          | will be created on the corresponding partition.    |
>      |             |          | vfat / ext2 / ext3 /ext4 file system is supported  |
>      +-------------+----------+----------------------------------------------------+
> +   | partuuid    | string   | The partition UUID (GPT only). If omitted, a UUID  |
> +   |             |          | will be generated automatically.			 |
> +   +-------------+----------+----------------------------------------------------+
>   
>   
>   
> diff --git a/handlers/diskpart_handler.c b/handlers/diskpart_handler.c
> index e08c10e..21efc8f 100644
> --- a/handlers/diskpart_handler.c
> +++ b/handlers/diskpart_handler.c
> @@ -17,6 +17,7 @@
>   #include <sys/types.h>
>   #include <libfdisk/libfdisk.h>
>   #include <fs_interface.h>
> +#include <uuid/uuid.h>
>   #include "swupdate.h"
>   #include "handler.h"
>   #include "util.h"
> @@ -50,7 +51,8 @@ enum partfield {
>   	PART_TYPE,
>   	PART_NAME,
>   	PART_FSTYPE,
> -	PART_DOSTYPE
> +	PART_DOSTYPE,
> +	PART_UUID,
>   };
>   
>   const char *fields[] = {
> @@ -59,7 +61,8 @@ const char *fields[] = {
>   	[PART_TYPE] = "type",
>   	[PART_NAME] = "name",
>   	[PART_FSTYPE] = "fstype",
> -	[PART_DOSTYPE] = "dostype"
> +	[PART_DOSTYPE] = "dostype",
> +	[PART_UUID] = "partuuid",
>   };
>   
>   struct partition_data {
> @@ -70,6 +73,7 @@ struct partition_data {
>   	char name[SWUPDATE_GENERAL_STRING_SIZE];
>   	char fstype[SWUPDATE_GENERAL_STRING_SIZE];
>   	char dostype[SWUPDATE_GENERAL_STRING_SIZE];
> +	char partuuid[UUID_STR_LEN];
>   	LIST_ENTRY(partition_data) next;
>   };
>   LIST_HEAD(listparts, partition_data);
> @@ -308,6 +312,9 @@ static int diskpart_set_partition(struct fdisk_partition *pa,
>   	if (parttype)
>   		ret |= fdisk_partition_set_type(pa, parttype);
>   
> +	if (strlen(part->partuuid))
> +	      ret |= fdisk_partition_set_uuid(pa, part->partuuid);
> +
>   	return ret;
>   }
>   
> @@ -424,41 +431,44 @@ static void diskpart_partition_info(struct fdisk_context *cxt, const char *name,
>   /*
>    * Return true if partition differs
>    */
> -static bool diskpart_partition_cmp(struct fdisk_context *cxt, struct fdisk_partition *firstpa,
> -		struct fdisk_partition *secondpa)
> +static bool diskpart_partition_cmp(struct fdisk_partition *firstpa, struct fdisk_partition *secondpa)
>   {
> -	struct fdisk_parttype *type;
> -	const char *lbtype;
> -
>   	if (!firstpa || !secondpa)
>   		return true;
>   
> -	type = fdisk_partition_get_type(firstpa);
> -	if (!type)
> +	if (fdisk_partition_cmp_partno(firstpa, secondpa) ||
> +		(!fdisk_partition_start_is_default(firstpa) && !fdisk_partition_start_is_default(secondpa) &&
> +			fdisk_partition_cmp_start(firstpa, secondpa)) ||
> +		fdisk_partition_get_size(firstpa) != fdisk_partition_get_size(secondpa)) {
>   		return true;
> +	}
>   
> -	if (fdisk_parttype_get_string(type))
> -		lbtype = "gpt";
> -	else
> -		lbtype = "dos";
> -
> -	if (firstpa && secondpa && (fdisk_partition_cmp_partno(firstpa, secondpa) ||
> -		(!fdisk_partition_start_is_default(firstpa) && !fdisk_partition_start_is_default(secondpa) &&
> -		fdisk_partition_cmp_start(firstpa, secondpa)) ||
> -		(!strcmp(lbtype, "gpt") &&
> -			(strcmp(fdisk_parttype_get_string(fdisk_partition_get_type(firstpa)),
> -				fdisk_parttype_get_string(fdisk_partition_get_type(secondpa))) ||
> -			strcmp(fdisk_partition_get_name(firstpa) ? fdisk_partition_get_name(firstpa) : "",
> -		       		fdisk_partition_get_name(secondpa) ? fdisk_partition_get_name(secondpa) : ""))) ||
> -		(!strcmp(lbtype, "dos") &&
> -			fdisk_parttype_get_code(fdisk_partition_get_type(firstpa)) !=
> -			fdisk_parttype_get_code(fdisk_partition_get_type(secondpa))) ||
> -		fdisk_partition_get_size(firstpa) != fdisk_partition_get_size(secondpa))) {
> -		TRACE("Partition differ:");
> -		diskpart_partition_info(cxt, "Original", firstpa);
> -		diskpart_partition_info(cxt, "New", secondpa);
> +	struct fdisk_parttype *firstpa_type = fdisk_partition_get_type(firstpa);
> +	if (!firstpa_type)
>   		return true;
> +	struct fdisk_parttype *secondpa_type = fdisk_partition_get_type(secondpa);
> +
> +	if (fdisk_parttype_get_string(firstpa_type)) {
> +		/* gpt */
> +		const char *firstpa_name = fdisk_partition_get_name(firstpa);
> +		const char *secondpa_name = fdisk_partition_get_name(secondpa);
> +		if ((secondpa_type && strcmp(fdisk_parttype_get_string(firstpa_type), fdisk_parttype_get_string(secondpa_type))) ||
> +			strcmp(firstpa_name ? firstpa_name : "", secondpa_name ? secondpa_name : "")) {
> +			return true;
> +		}
> +
> +		const char *firstpa_uuid = fdisk_partition_get_uuid(firstpa);
> +		const char *secondpa_uuid = fdisk_partition_get_uuid(secondpa);
> +		if (firstpa_uuid && secondpa_uuid && strcmp(firstpa_uuid, secondpa_uuid)) {
> +			return true;
> +		}
> +	} else {
> +		/* dos */
> +		if (fdisk_parttype_get_code(firstpa_type) != fdisk_parttype_get_code(secondpa_type)) {
> +			return true;
> +		}
>   	}
> +
>   	return false;
>   }
>   
> @@ -610,7 +620,10 @@ static int diskpart_table_cmp(struct fdisk_context *cxt, struct fdisk_table *tb,
>   				fdisk_table_next_partition (oldtb, olditr, &pa)) {
>   				TRACE("Partition not found, something went wrong %lu !", i);
>   				ret = -EFAULT;
> -			} else if (diskpart_partition_cmp(cxt, pa, newpa)) {
> +			} else if (diskpart_partition_cmp(pa, newpa)) {
> +				TRACE("Partition differ:");
> +				diskpart_partition_info(cxt, "Original", pa);
> +				diskpart_partition_info(cxt, "New", newpa);
>   				ret = 1;
>   			}
>   
> @@ -845,6 +858,9 @@ static int diskpart(struct img_type *img,
>   						strncpy(part->dostype, equal, sizeof(part->dostype));
>   						hybrid++;
>   						break;
> +					case PART_UUID:
> +						strncpy(part->partuuid, equal, sizeof(part->partuuid));
> +						break;
>   					}
>   				}
>   			}
> @@ -864,12 +880,13 @@ static int diskpart(struct img_type *img,
>   			goto handler_exit;
>   		}
>   
> -		TRACE("partition-%zu:%s size %" PRIu64 " start %zu type %s",
> +		TRACE("partition-%zu:%s size %" PRIu64 " start %zu type %s partuuid %s",
>   			part->partno != LIBFDISK_INIT_UNDEF(part->partno) ? part->partno : 0,
>   			strlen(part->name) ? part->name : "UNDEF NAME",
>   			part->size != LIBFDISK_INIT_UNDEF(part->size) ? part->size : 0,
>   			part->start!= LIBFDISK_INIT_UNDEF(part->start) ? part->start : 0,
> -			part->type);
> +			part->type,
> +			strlen(part->partuuid) ? part->partuuid : "automatic");
>   
>   		/*
>   		 * Partitions in sw-description start from 1,
> 

Applied to -master, thanks !

Best regards,
Stefano Babic