[RFC,1/1] Add support for setting hybrid dos partition entries.

This adds the ability to set dos partition entries for gpt disks.

This is mostly useful for booting on GPT disks from boards that
don't natively support GPT.

Config example:
partition-1 = [
        "size=550M",
        "start=2048",
        "name=boot",
        "type=C12A7328-F81F-11D2-BA4B-00A0C93EC93B",
        "fstype=vfat",
        "dostype=0x0C"
];

Some things such as disk partition comparisons are not fully
working yet.

Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 handlers/diskpart_handler.c | 208 +++++++++++++++++++++++++++++++++---
 1 file changed, 195 insertions(+), 13 deletions(-)

Hi James,

On 12.05.21 06:16, James Hilliard wrote:
> This adds the ability to set dos partition entries for gpt disks.
> 
> This is mostly useful for booting on GPT disks from boards that
> don't natively support GPT.
> 

In principle: even if using hybrid partition is discouraged, I have 
nothing against this if it does not change the behavior for the standard 
case, that is with hybrid off.

> Config example:
> partition-1 = [
>          "size=550M",
>          "start=2048",
>          "name=boot",
>          "type=C12A7328-F81F-11D2-BA4B-00A0C93EC93B",
>          "fstype=vfat",
>          "dostype=0x0C"
> ];
> 

General remark: when you repost without RFC, you should also add 
documentation for it, that is in handlers.rst, chapter "Disk partitioner".

> Some things such as disk partition comparisons are not fully
> working yet.
> 

That is ok if it is fully documented.

> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> ---
>   handlers/diskpart_handler.c | 208 +++++++++++++++++++++++++++++++++---
>   1 file changed, 195 insertions(+), 13 deletions(-)
> 
> diff --git a/handlers/diskpart_handler.c b/handlers/diskpart_handler.c
> index 65010c1..cefe200 100644
> --- a/handlers/diskpart_handler.c
> +++ b/handlers/diskpart_handler.c
> @@ -60,7 +60,8 @@ enum partfield {
>   	PART_START,
>   	PART_TYPE,
>   	PART_NAME,
> -	PART_FSTYPE
> +	PART_FSTYPE,
> +	PART_DOSTYPE
>   };
>   
>   const char *fields[] = {
> @@ -68,7 +69,8 @@ const char *fields[] = {
>   	[PART_START] = "start",
>   	[PART_TYPE] = "type",
>   	[PART_NAME] = "name",
> -	[PART_FSTYPE] = "fstype"
> +	[PART_FSTYPE] = "fstype",
> +	[PART_DOSTYPE] = "dostype"


Just a question (I have not fully read the spec for hybrid): is 
"dostype" a good name ? Sure, hybrid means with have also a MBR-like 
approach, but at first glance it does not say that a hybrid partition is 
used. Something with "hybrid", maybe (hybridtype,..) ?

>   };
>   
>   struct partition_data {
> @@ -78,6 +80,7 @@ struct partition_data {
>   	char type[SWUPDATE_GENERAL_STRING_SIZE];
>   	char name[SWUPDATE_GENERAL_STRING_SIZE];
>   	char fstype[SWUPDATE_GENERAL_STRING_SIZE];
> +	char dostype[SWUPDATE_GENERAL_STRING_SIZE];
>   	LIST_ENTRY(partition_data) next;
>   };
>   LIST_HEAD(listparts, partition_data);
> @@ -101,8 +104,17 @@ struct hnd_priv {
>   static int diskpart_set_partition(struct fdisk_partition *pa,
>   				  struct partition_data *part,
>   				  unsigned long sector_size,
> -				  struct fdisk_parttype *parttype)
> +				  struct fdisk_parttype *parttype,
> +				  unsigned long hybrid)
>   {
> +	TRACE("set partition-%zu:%s size %" PRIu64 " start %zu type %s%s%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 / sector_size : 0,
> +		  part->start!= LIBFDISK_INIT_UNDEF(part->start) ? part->start : 0,
> +		  part->type,
> +		  strlen(part->dostype) ? " dostype " : "",
> +		  strlen(part->dostype) ? part->dostype : "");

We have already a big TRACE() in diskpart() when properties are read. 
This quite duplicates the verbosity of an already verbose handler. If 
you think it is still better to have, I suggest to change from TRACE to 
DEBUG (or drop at all if you think it is redundant).

>   	int ret = 0;
>   
>   	if (!sector_size)
> @@ -114,14 +126,14 @@ static int diskpart_set_partition(struct fdisk_partition *pa,
>   		ret = fdisk_partition_set_start(pa, part->start);
>   	else
>   		ret = fdisk_partition_start_follow_default(pa, 1);
> -	if (part->partno != LIBFDISK_INIT_UNDEF(part->partno))
> +	if (!hybrid && part->partno != LIBFDISK_INIT_UNDEF(part->partno))
>   		ret |= fdisk_partition_set_partno(pa, part->partno);
>   	else
>   		ret |= fdisk_partition_partno_follow_default(pa, 1);
> -	if (strlen(part->name))
> -	      ret |= fdisk_partition_set_name(pa, part->name);
> +	if (!hybrid && strlen(part->name))
> +		  ret |= fdisk_partition_set_name(pa, part->name);
>   	if (part->size != LIBFDISK_INIT_UNDEF(part->size))
> -	      ret |= fdisk_partition_set_size(pa, part->size / sector_size);
> +		  ret |= fdisk_partition_set_size(pa, part->size / sector_size);
>   	else
>   		ret |= fdisk_partition_end_follow_default(pa, 1);
>   
> @@ -146,7 +158,7 @@ static bool diskpart_partition_cmp(const char *lbtype, struct fdisk_partition *f
>   			(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) : ""))) ||
> +					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))) ||
> @@ -168,15 +180,23 @@ static int diskpart(struct img_type *img,
>   	struct dict_list *parts;
>   	struct dict_list_elem *elem;
>   	struct fdisk_context *cxt;
> +	struct fdisk_context *doscxt = NULL;
>   	struct partition_data *part;
>   	struct partition_data *tmp;
>   	struct fdisk_table *tb = NULL;
> +	struct fdisk_table *dostb = NULL;
>   	struct fdisk_table *oldtb = NULL;
> +	struct fdisk_table *olddostb = NULL;
>   	struct fdisk_parttype *parttype = NULL;
> +	struct fdisk_parttype *dosparttype = NULL;
>   	int ret = 0;
>   	unsigned long i;
> +	unsigned long hybrid = 0;
>   	struct hnd_priv priv =  {FDISK_DISKLABEL_DOS};
>   	bool createtable = false;
> +	bool createdostable = false;
> +
> +	//fdisk_init_debug(0xffff);

Dead code

>   
>   	if (!lbtype || (strcmp(lbtype, "gpt") && strcmp(lbtype, "dos"))) {
>   		ERROR("Just GPT or DOS partition table are supported");
> @@ -194,6 +214,8 @@ static int diskpart(struct img_type *img,
>   		return -ENOMEM;
>   	}
>   
> +	fdisk_disable_dialogs(cxt, 1);
> +

Do we need it ? for main context was not necessary.

>   	ret = fdisk_assign_device(cxt, img->device, 0);
>   	if (ret == -EACCES) {
>   		ERROR("no access to %s", img->device);
> @@ -247,18 +269,31 @@ static int diskpart(struct img_type *img,
>   					case PART_FSTYPE:
>   						strncpy(part->fstype, equal, sizeof(part->fstype));
>   						break;
> +					case PART_DOSTYPE:
> +						strncpy(part->dostype, equal, sizeof(part->dostype));
> +						hybrid++;
> +						break;

Is there some check we should do it on the value ? dostype cannot be any 
string.

>   					}
>   				}
>   			}
>   			elem = LIST_NEXT(elem, next);
>   		}
>   
> -		TRACE("partition-%zu:%s size %" PRIu64 " start %zu type %s",
> +		if (hybrid > 3) {
> +			ERROR("I cannot add hybrid partition %zu(%s): hybrid dos partition limit of 3 exceeded",
> +				  part->partno, strlen(part->name) ? part->name : "UNDEF NAME");
> +			ret = -EINVAL;
> +			goto handler_exit;
> +		}
> +
> +		TRACE("partition-%zu:%s size %" PRIu64 " start %zu type %s%s%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->dostype) ? " dostype " : "",
> +			strlen(part->dostype) ? part->dostype : "");

This is what I mind, there is already a long TRACE with all setup.

>   
>   		/*
>   		 * Partitions in sw-description start from 1,
> @@ -287,7 +322,11 @@ static int diskpart(struct img_type *img,
>   	if (!fdisk_has_label(cxt)) {
>   		WARN("%s does not contain a recognized partition table",
>   		     img->device);
> -		fdisk_create_disklabel(cxt, lbtype);
> +		ret = fdisk_create_disklabel(cxt, lbtype);
> +		if (ret) {
> +			ERROR("Failed to create disk label");
> +			goto handler_release;
> +		}

This is a fix - I suggest you post this in a separate patch, together 
with...

>   		createtable = true;
>   	} else if (lbtype) {
>   		if (!strcmp(lbtype, "gpt"))
> @@ -298,12 +337,17 @@ static int diskpart(struct img_type *img,
>   		if (!fdisk_is_labeltype(cxt, priv.labeltype)) {
>   			WARN("Partition table of different type, setting to %s, all data lost !",
>   				lbtype);
> -			fdisk_create_disklabel(cxt, lbtype);
> +			ret = fdisk_create_disklabel(cxt, lbtype);
> +			if (ret) {
> +				ERROR("Failed to create disk label");
> +				goto handler_release;
> +			}

...this one.

>   			createtable = true;
>   		}
>   	}
>   
>   	struct fdisk_label *lb = fdisk_get_label(cxt, NULL);
> +	struct fdisk_label *doslb = NULL;
>   	unsigned long sector_size = fdisk_get_sector_size(cxt);
>   
>   	/*
> @@ -321,6 +365,32 @@ static int diskpart(struct img_type *img,
>   		goto handler_exit;
>   	}
>   
> +	if (hybrid) {
> +		doscxt = fdisk_new_nested_context(cxt, "dos");
> +		fdisk_disable_dialogs(doscxt, 1);
See my question regarding this call.

> +
> +		if (!fdisk_is_labeltype(doscxt, FDISK_DISKLABEL_DOS)) {
> +			ret = fdisk_create_disklabel(doscxt, NULL);
> +			if (ret) {
> +				ERROR("Failed to create hybrid disk label");
> +				goto handler_release;
> +			}
> +			createdostable = true;
> +		}
> +
> +		doslb = fdisk_get_label(doscxt, "dos");
> +		dostb = fdisk_new_table();
> +
> +		if (fdisk_get_partitions(doscxt, &olddostb))
> +			createdostable = true;
> +
> +		if (!dostb) {
> +			ERROR("OOM creating new table !");
> +			ret = -ENOMEM;
> +			goto handler_exit;
> +		}
> +	}
> +
>   	i = 0;
>   
>   	LIST_FOREACH(part, &priv.listparts, next) {
> @@ -334,10 +404,30 @@ static int diskpart(struct img_type *img,
>   			parttype = fdisk_label_get_parttype_from_string(lb, part->type);
>   			if (!parttype)
>   				parttype = fdisk_label_get_parttype_from_string(lb, GPT_DEFAULT_ENTRY_TYPE);
> +			if (strlen(part->dostype)) {
> +				struct fdisk_partition *newdospa;
> +				newdospa = fdisk_new_partition();
> +				dosparttype = fdisk_label_get_parttype_from_code(doslb, ustrtoull(part->dostype, 16));
> +				if (!dosparttype) {
> +					WARN("Failed to set partition type for hybrid partition: %zu(%s)", part->partno, part->name);
> +				}
> +				ret = diskpart_set_partition(newdospa, part, sector_size, dosparttype, hybrid);
> +				if (ret) {
> +					WARN("I cannot set all hybrid partition's parameters");
> +				}
> +				if ((ret = fdisk_table_add_partition(dostb, newdospa)) < 0) {
> +					ERROR("I cannot add hybrid partition %zu(%s): %d", part->partno, part->name, ret);
> +				}
> +				fdisk_unref_partition(newdospa);
> +				if (ret < 0) {
> +					fdisk_unref_partition(newpa);
> +					goto handler_exit;
> +				}
> +			}
>   		} else {
>   			parttype = fdisk_label_get_parttype_from_code(lb, ustrtoull(part->type, 16));
>   		}
> -		ret = diskpart_set_partition(newpa, part, sector_size, parttype);
> +		ret = diskpart_set_partition(newpa, part, sector_size, parttype, 0);
>   		if (ret) {
>   			WARN("I cannot set all partition's parameters");
>   		}
> @@ -350,6 +440,31 @@ static int diskpart(struct img_type *img,
>   		i++;
>   	}
>   
> +	if (hybrid) {
> +		struct fdisk_partition *newpa;
> +		newpa = fdisk_new_partition();
> +		part = (struct partition_data *)calloc(1, sizeof(struct partition_data));
> +		if (!part) {
> +			ERROR("FAULT: no memory");
> +			ret = -ENOMEM;
> +			goto handler_exit;
> +		}
> +		part->start = 1;
> +		part->size = 33 * sector_size;
> +		dosparttype = fdisk_label_get_parttype_from_code(doslb, 0xee);
> +		ret = diskpart_set_partition(newpa, part, sector_size, dosparttype, hybrid);
> +		if (ret) {
> +			WARN("I cannot set all pmbr partition's parameters");
> +		}
> +		if ((ret = fdisk_table_add_partition(dostb, newpa)) < 0) {
> +			ERROR("I cannot add the pmbr partition");
> +		}
> +		fdisk_unref_partition(newpa);
> +		if (ret < 0)
> +			goto handler_exit;
> +		hybrid++;
> +	}
> +
>   	/*
>   	 * A partiton table was found on disk, now compares the two tables
>   	 * to check if they differ.
> @@ -387,6 +502,69 @@ static int diskpart(struct img_type *img,
>   		}
>   	}
>   
> +	/*
> +	 * A dos partiton table was found on disk, now compares the two tables
> +	 * to check if they differ.
> +	 */
> +	/*
> +	if (hybrid && !createdostable) {
> +		size_t numpartondisk = fdisk_table_get_nents(olddostb);
> +
> +		i = hybrid;
> +		if (numpartondisk != i) {
> +			TRACE("Number of hybrid partitions differs on disk: %lu <--> requested: %lu",
> +				  (long unsigned int)numpartondisk, i);
> +			createdostable = true;
> +		} else {
> +			struct fdisk_partition *pa, *newpa;
> +			struct fdisk_iter *itr	 = fdisk_new_iter(FDISK_ITER_FORWARD);
> +			struct fdisk_iter *olditr = fdisk_new_iter(FDISK_ITER_FORWARD);
> +
> +			i = 0;
> +			while (i < numpartondisk && !createdostable) {
> +				newpa=NULL;
> +				pa = NULL;
> +				if (fdisk_table_next_partition (dostb, itr, &newpa) ||
> +					fdisk_table_next_partition (olddostb, olditr, &pa)) {
> +					TRACE("Hybrid partition not found, something went wrong %lu !", i);
> +					ret = -EFAULT;
> +					goto handler_exit;
> +				}
> +				if (diskpart_partition_cmp("dos", pa, newpa)) {
> +					createdostable = true;
> +				}
> +
> +				fdisk_unref_partition(newpa);
> +				fdisk_unref_partition(pa);
> +				i++;
> +			}
> +		}
> +	}
> +	*/

Ok, so klet the code in when it works. As first step, you can force that 
the table must be updated if hybrid is set.

> +	if (hybrid) {
> +		// TODO fix dos table comparison
> +		createdostable = true;
> +	}
> +
> +	if (createdostable) {
> +		TRACE("Hybrid partitions on disk differ, write to disk;");
> +		fdisk_delete_all_partitions(doscxt);
> +		ret = fdisk_apply_table(doscxt, dostb);
> +		if (ret) {
> +			ERROR("Hybrid partition table cannot be applied! %d", ret);
> +			goto handler_exit;
> +		}
> +
> +		/*
> +		 * Everything done, write into disk
> +		 */
> +		ret = fdisk_write_disklabel(doscxt);
> +		if (ret) {
> +			ERROR("Hybrid partition table cannot be written on disk %d", ret);
> +			goto handler_exit;
> +		}
> +	}
> +
>   	if (createtable) {
>   		TRACE("Partitions on disk differ, write to disk;");
>   		fdisk_delete_all_partitions(cxt);
> @@ -412,12 +590,16 @@ static int diskpart(struct img_type *img,
>   handler_exit:
>   	if (tb)
>   		fdisk_unref_table(tb);
> +	if (dostb)
> +		fdisk_unref_table(dostb);
>   	if (oldtb)
>   		fdisk_unref_table(oldtb);
>   	if (fdisk_deassign_device(cxt, 0))
>   		WARN("Error deassign device %s", img->device);
>   
>   handler_release:
> +	if (fdisk_get_parent(doscxt))
> +		fdisk_unref_context(doscxt);
>   	fdisk_unref_context(cxt);
>   
>   	/*
> 

As far as I can see, this does not change the current behavior. Yes, it 
must be well tested (as any patch should be..)

Best regards,
Stefano Babic

On Wed, May 12, 2021 at 9:44 AM Stefano Babic <sbabic@denx.de> wrote:
>
> Hi James,
>
> On 12.05.21 06:16, James Hilliard wrote:
> > This adds the ability to set dos partition entries for gpt disks.
> >
> > This is mostly useful for booting on GPT disks from boards that
> > don't natively support GPT.
> >
>
> In principle: even if using hybrid partition is discouraged, I have
> nothing against this if it does not change the behavior for the standard
> case, that is with hybrid off.
I think most of the issues with hybrid partitions are due to windows dual
boot setups, which is much less of an issue for embedded Linux systems.
>
> > Config example:
> > partition-1 = [
> >          "size=550M",
> >          "start=2048",
> >          "name=boot",
> >          "type=C12A7328-F81F-11D2-BA4B-00A0C93EC93B",
> >          "fstype=vfat",
> >          "dostype=0x0C"
> > ];
> >
>
> General remark: when you repost without RFC, you should also add
> documentation for it, that is in handlers.rst, chapter "Disk partitioner".
Will do.
>
> > Some things such as disk partition comparisons are not fully
> > working yet.
> >
>
> That is ok if it is fully documented.
Yeah, I'm planning to fix these issues first, just haven't had the chance to
fully track down some edge case bugs.
>
> > Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> > ---
> >   handlers/diskpart_handler.c | 208 +++++++++++++++++++++++++++++++++---
> >   1 file changed, 195 insertions(+), 13 deletions(-)
> >
> > diff --git a/handlers/diskpart_handler.c b/handlers/diskpart_handler.c
> > index 65010c1..cefe200 100644
> > --- a/handlers/diskpart_handler.c
> > +++ b/handlers/diskpart_handler.c
> > @@ -60,7 +60,8 @@ enum partfield {
> >       PART_START,
> >       PART_TYPE,
> >       PART_NAME,
> > -     PART_FSTYPE
> > +     PART_FSTYPE,
> > +     PART_DOSTYPE
> >   };
> >
> >   const char *fields[] = {
> > @@ -68,7 +69,8 @@ const char *fields[] = {
> >       [PART_START] = "start",
> >       [PART_TYPE] = "type",
> >       [PART_NAME] = "name",
> > -     [PART_FSTYPE] = "fstype"
> > +     [PART_FSTYPE] = "fstype",
> > +     [PART_DOSTYPE] = "dostype"
>
>
> Just a question (I have not fully read the spec for hybrid): is
> "dostype" a good name ? Sure, hybrid means with have also a MBR-like
> approach, but at first glance it does not say that a hybrid partition is
> used. Something with "hybrid", maybe (hybridtype,..) ?
Not sure exactly if this is the best name but I don't think there is really
much of a defined spec for hybrid dos partitions. Keep in mind basically
all GPT formatted disks have protective dos tables, this basically just
allows adding additional entries to the protective dos table so that the
bootloader can find the kernel(the kernel will automatically then detect
the GPT table and use that instead). This GPT detection behavior is at
least well defined for the Linux kernel but might not be for other operating
systems.
>
> >   };
> >
> >   struct partition_data {
> > @@ -78,6 +80,7 @@ struct partition_data {
> >       char type[SWUPDATE_GENERAL_STRING_SIZE];
> >       char name[SWUPDATE_GENERAL_STRING_SIZE];
> >       char fstype[SWUPDATE_GENERAL_STRING_SIZE];
> > +     char dostype[SWUPDATE_GENERAL_STRING_SIZE];
> >       LIST_ENTRY(partition_data) next;
> >   };
> >   LIST_HEAD(listparts, partition_data);
> > @@ -101,8 +104,17 @@ struct hnd_priv {
> >   static int diskpart_set_partition(struct fdisk_partition *pa,
> >                                 struct partition_data *part,
> >                                 unsigned long sector_size,
> > -                               struct fdisk_parttype *parttype)
> > +                               struct fdisk_parttype *parttype,
> > +                               unsigned long hybrid)
> >   {
> > +     TRACE("set partition-%zu:%s size %" PRIu64 " start %zu type %s%s%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 / sector_size : 0,
> > +               part->start!= LIBFDISK_INIT_UNDEF(part->start) ? part->start : 0,
> > +               part->type,
> > +               strlen(part->dostype) ? " dostype " : "",
> > +               strlen(part->dostype) ? part->dostype : "");
>
> We have already a big TRACE() in diskpart() when properties are read.
> This quite duplicates the verbosity of an already verbose handler. If
> you think it is still better to have, I suggest to change from TRACE to
> DEBUG (or drop at all if you think it is redundant).
Yeah, this is just debug code that I'll strip out after I finish testing.
> >       int ret = 0;
> >
> >       if (!sector_size)
> > @@ -114,14 +126,14 @@ static int diskpart_set_partition(struct fdisk_partition *pa,
> >               ret = fdisk_partition_set_start(pa, part->start);
> >       else
> >               ret = fdisk_partition_start_follow_default(pa, 1);
> > -     if (part->partno != LIBFDISK_INIT_UNDEF(part->partno))
> > +     if (!hybrid && part->partno != LIBFDISK_INIT_UNDEF(part->partno))
> >               ret |= fdisk_partition_set_partno(pa, part->partno);
> >       else
> >               ret |= fdisk_partition_partno_follow_default(pa, 1);
> > -     if (strlen(part->name))
> > -           ret |= fdisk_partition_set_name(pa, part->name);
> > +     if (!hybrid && strlen(part->name))
> > +               ret |= fdisk_partition_set_name(pa, part->name);
> >       if (part->size != LIBFDISK_INIT_UNDEF(part->size))
> > -           ret |= fdisk_partition_set_size(pa, part->size / sector_size);
> > +               ret |= fdisk_partition_set_size(pa, part->size / sector_size);
> >       else
> >               ret |= fdisk_partition_end_follow_default(pa, 1);
> >
> > @@ -146,7 +158,7 @@ static bool diskpart_partition_cmp(const char *lbtype, struct fdisk_partition *f
> >                       (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) : ""))) ||
> > +                                     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))) ||
> > @@ -168,15 +180,23 @@ static int diskpart(struct img_type *img,
> >       struct dict_list *parts;
> >       struct dict_list_elem *elem;
> >       struct fdisk_context *cxt;
> > +     struct fdisk_context *doscxt = NULL;
> >       struct partition_data *part;
> >       struct partition_data *tmp;
> >       struct fdisk_table *tb = NULL;
> > +     struct fdisk_table *dostb = NULL;
> >       struct fdisk_table *oldtb = NULL;
> > +     struct fdisk_table *olddostb = NULL;
> >       struct fdisk_parttype *parttype = NULL;
> > +     struct fdisk_parttype *dosparttype = NULL;
> >       int ret = 0;
> >       unsigned long i;
> > +     unsigned long hybrid = 0;
> >       struct hnd_priv priv =  {FDISK_DISKLABEL_DOS};
> >       bool createtable = false;
> > +     bool createdostable = false;
> > +
> > +     //fdisk_init_debug(0xffff);
>
> Dead code
Yeah, will remove once finished testing.
>
> >
> >       if (!lbtype || (strcmp(lbtype, "gpt") && strcmp(lbtype, "dos"))) {
> >               ERROR("Just GPT or DOS partition table are supported");
> > @@ -194,6 +214,8 @@ static int diskpart(struct img_type *img,
> >               return -ENOMEM;
> >       }
> >
> > +     fdisk_disable_dialogs(cxt, 1);
> > +
>
> Do we need it ? for main context was not necessary.
Well there are some failures this fixes where different code paths are taken
when dialogs are disabled, since we don't support interactive dialogs at all
it's best to ensure they are disabled fully to avoid otherwise hitting an error.

See here:
https://github.com/karelzak/util-linux/blob/ad0963685b8105920abd38e6878cc2cdfa427ea4/libfdisk/src/dos.c#L1897-L1903
https://github.com/karelzak/util-linux/blob/ba3b93525920f2782e12016e7444fca89fb5bb9c/libfdisk/src/partition.c#L770-L771
>
> >       ret = fdisk_assign_device(cxt, img->device, 0);
> >       if (ret == -EACCES) {
> >               ERROR("no access to %s", img->device);
> > @@ -247,18 +269,31 @@ static int diskpart(struct img_type *img,
> >                                       case PART_FSTYPE:
> >                                               strncpy(part->fstype, equal, sizeof(part->fstype));
> >                                               break;
> > +                                     case PART_DOSTYPE:
> > +                                             strncpy(part->dostype, equal, sizeof(part->dostype));
> > +                                             hybrid++;
> > +                                             break;
>
> Is there some check we should do it on the value ? dostype cannot be any
> string.
I basically just copied this part from how we currently parse dos
entry types, if
we need to add validation we should do it for non-hybrid dos type
parsing as well.
>
> >                                       }
> >                               }
> >                       }
> >                       elem = LIST_NEXT(elem, next);
> >               }
> >
> > -             TRACE("partition-%zu:%s size %" PRIu64 " start %zu type %s",
> > +             if (hybrid > 3) {
> > +                     ERROR("I cannot add hybrid partition %zu(%s): hybrid dos partition limit of 3 exceeded",
> > +                               part->partno, strlen(part->name) ? part->name : "UNDEF NAME");
> > +                     ret = -EINVAL;
> > +                     goto handler_exit;
> > +             }
> > +
> > +             TRACE("partition-%zu:%s size %" PRIu64 " start %zu type %s%s%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->dostype) ? " dostype " : "",
> > +                     strlen(part->dostype) ? part->dostype : "");
>
> This is what I mind, there is already a long TRACE with all setup.
Ok, so I should keep this and remove the other temporary debugging
TRACE right?
>
> >
> >               /*
> >                * Partitions in sw-description start from 1,
> > @@ -287,7 +322,11 @@ static int diskpart(struct img_type *img,
> >       if (!fdisk_has_label(cxt)) {
> >               WARN("%s does not contain a recognized partition table",
> >                    img->device);
> > -             fdisk_create_disklabel(cxt, lbtype);
> > +             ret = fdisk_create_disklabel(cxt, lbtype);
> > +             if (ret) {
> > +                     ERROR("Failed to create disk label");
> > +                     goto handler_release;
> > +             }
>
> This is a fix - I suggest you post this in a separate patch, together
> with...
Patch sent.
>
> >               createtable = true;
> >       } else if (lbtype) {
> >               if (!strcmp(lbtype, "gpt"))
> > @@ -298,12 +337,17 @@ static int diskpart(struct img_type *img,
> >               if (!fdisk_is_labeltype(cxt, priv.labeltype)) {
> >                       WARN("Partition table of different type, setting to %s, all data lost !",
> >                               lbtype);
> > -                     fdisk_create_disklabel(cxt, lbtype);
> > +                     ret = fdisk_create_disklabel(cxt, lbtype);
> > +                     if (ret) {
> > +                             ERROR("Failed to create disk label");
> > +                             goto handler_release;
> > +                     }
>
> ...this one.
...with this as well.
>
> >                       createtable = true;
> >               }
> >       }
> >
> >       struct fdisk_label *lb = fdisk_get_label(cxt, NULL);
> > +     struct fdisk_label *doslb = NULL;
> >       unsigned long sector_size = fdisk_get_sector_size(cxt);
> >
> >       /*
> > @@ -321,6 +365,32 @@ static int diskpart(struct img_type *img,
> >               goto handler_exit;
> >       }
> >
> > +     if (hybrid) {
> > +             doscxt = fdisk_new_nested_context(cxt, "dos");
> > +             fdisk_disable_dialogs(doscxt, 1);
> See my question regarding this call.
Yeah, we def need this to prevent failures in the codepaths I mentioned above.
>
> > +
> > +             if (!fdisk_is_labeltype(doscxt, FDISK_DISKLABEL_DOS)) {
> > +                     ret = fdisk_create_disklabel(doscxt, NULL);
> > +                     if (ret) {
> > +                             ERROR("Failed to create hybrid disk label");
> > +                             goto handler_release;
> > +                     }
> > +                     createdostable = true;
> > +             }
> > +
> > +             doslb = fdisk_get_label(doscxt, "dos");
> > +             dostb = fdisk_new_table();
> > +
> > +             if (fdisk_get_partitions(doscxt, &olddostb))
> > +                     createdostable = true;
> > +
> > +             if (!dostb) {
> > +                     ERROR("OOM creating new table !");
> > +                     ret = -ENOMEM;
> > +                     goto handler_exit;
> > +             }
> > +     }
> > +
> >       i = 0;
> >
> >       LIST_FOREACH(part, &priv.listparts, next) {
> > @@ -334,10 +404,30 @@ static int diskpart(struct img_type *img,
> >                       parttype = fdisk_label_get_parttype_from_string(lb, part->type);
> >                       if (!parttype)
> >                               parttype = fdisk_label_get_parttype_from_string(lb, GPT_DEFAULT_ENTRY_TYPE);
> > +                     if (strlen(part->dostype)) {
> > +                             struct fdisk_partition *newdospa;
> > +                             newdospa = fdisk_new_partition();
> > +                             dosparttype = fdisk_label_get_parttype_from_code(doslb, ustrtoull(part->dostype, 16));
> > +                             if (!dosparttype) {
> > +                                     WARN("Failed to set partition type for hybrid partition: %zu(%s)", part->partno, part->name);
> > +                             }
> > +                             ret = diskpart_set_partition(newdospa, part, sector_size, dosparttype, hybrid);
> > +                             if (ret) {
> > +                                     WARN("I cannot set all hybrid partition's parameters");
> > +                             }
> > +                             if ((ret = fdisk_table_add_partition(dostb, newdospa)) < 0) {
> > +                                     ERROR("I cannot add hybrid partition %zu(%s): %d", part->partno, part->name, ret);
> > +                             }
> > +                             fdisk_unref_partition(newdospa);
> > +                             if (ret < 0) {
> > +                                     fdisk_unref_partition(newpa);
> > +                                     goto handler_exit;
> > +                             }
> > +                     }
> >               } else {
> >                       parttype = fdisk_label_get_parttype_from_code(lb, ustrtoull(part->type, 16));
> >               }
> > -             ret = diskpart_set_partition(newpa, part, sector_size, parttype);
> > +             ret = diskpart_set_partition(newpa, part, sector_size, parttype, 0);
> >               if (ret) {
> >                       WARN("I cannot set all partition's parameters");
> >               }
> > @@ -350,6 +440,31 @@ static int diskpart(struct img_type *img,
> >               i++;
> >       }
> >
> > +     if (hybrid) {
> > +             struct fdisk_partition *newpa;
> > +             newpa = fdisk_new_partition();
> > +             part = (struct partition_data *)calloc(1, sizeof(struct partition_data));
> > +             if (!part) {
> > +                     ERROR("FAULT: no memory");
> > +                     ret = -ENOMEM;
> > +                     goto handler_exit;
> > +             }
> > +             part->start = 1;
> > +             part->size = 33 * sector_size;
> > +             dosparttype = fdisk_label_get_parttype_from_code(doslb, 0xee);
> > +             ret = diskpart_set_partition(newpa, part, sector_size, dosparttype, hybrid);
> > +             if (ret) {
> > +                     WARN("I cannot set all pmbr partition's parameters");
> > +             }
> > +             if ((ret = fdisk_table_add_partition(dostb, newpa)) < 0) {
> > +                     ERROR("I cannot add the pmbr partition");
> > +             }
> > +             fdisk_unref_partition(newpa);
> > +             if (ret < 0)
> > +                     goto handler_exit;
> > +             hybrid++;
> > +     }
> > +
> >       /*
> >        * A partiton table was found on disk, now compares the two tables
> >        * to check if they differ.
> > @@ -387,6 +502,69 @@ static int diskpart(struct img_type *img,
> >               }
> >       }
> >
> > +     /*
> > +      * A dos partiton table was found on disk, now compares the two tables
> > +      * to check if they differ.
> > +      */
> > +     /*
> > +     if (hybrid && !createdostable) {
> > +             size_t numpartondisk = fdisk_table_get_nents(olddostb);
> > +
> > +             i = hybrid;
> > +             if (numpartondisk != i) {
> > +                     TRACE("Number of hybrid partitions differs on disk: %lu <--> requested: %lu",
> > +                               (long unsigned int)numpartondisk, i);
> > +                     createdostable = true;
> > +             } else {
> > +                     struct fdisk_partition *pa, *newpa;
> > +                     struct fdisk_iter *itr   = fdisk_new_iter(FDISK_ITER_FORWARD);
> > +                     struct fdisk_iter *olditr = fdisk_new_iter(FDISK_ITER_FORWARD);
> > +
> > +                     i = 0;
> > +                     while (i < numpartondisk && !createdostable) {
> > +                             newpa=NULL;
> > +                             pa = NULL;
> > +                             if (fdisk_table_next_partition (dostb, itr, &newpa) ||
> > +                                     fdisk_table_next_partition (olddostb, olditr, &pa)) {
> > +                                     TRACE("Hybrid partition not found, something went wrong %lu !", i);
> > +                                     ret = -EFAULT;
> > +                                     goto handler_exit;
> > +                             }
> > +                             if (diskpart_partition_cmp("dos", pa, newpa)) {
> > +                                     createdostable = true;
> > +                             }
> > +
> > +                             fdisk_unref_partition(newpa);
> > +                             fdisk_unref_partition(pa);
> > +                             i++;
> > +                     }
> > +             }
> > +     }
> > +     */
>
> Ok, so klet the code in when it works. As first step, you can force that
> the table must be updated if hybrid is set.
I think I might try and clean up diskpart_partition_cmp first, the current
implementation is a little annoying to debug as it doesn't really log all the
differences between partitions properly.
>
> > +     if (hybrid) {
> > +             // TODO fix dos table comparison
> > +             createdostable = true;
> > +     }
> > +
> > +     if (createdostable) {
> > +             TRACE("Hybrid partitions on disk differ, write to disk;");
> > +             fdisk_delete_all_partitions(doscxt);
> > +             ret = fdisk_apply_table(doscxt, dostb);
> > +             if (ret) {
> > +                     ERROR("Hybrid partition table cannot be applied! %d", ret);
> > +                     goto handler_exit;
> > +             }
> > +
> > +             /*
> > +              * Everything done, write into disk
> > +              */
> > +             ret = fdisk_write_disklabel(doscxt);
> > +             if (ret) {
> > +                     ERROR("Hybrid partition table cannot be written on disk %d", ret);
> > +                     goto handler_exit;
> > +             }
> > +     }
> > +
> >       if (createtable) {
> >               TRACE("Partitions on disk differ, write to disk;");
> >               fdisk_delete_all_partitions(cxt);
> > @@ -412,12 +590,16 @@ static int diskpart(struct img_type *img,
> >   handler_exit:
> >       if (tb)
> >               fdisk_unref_table(tb);
> > +     if (dostb)
> > +             fdisk_unref_table(dostb);
> >       if (oldtb)
> >               fdisk_unref_table(oldtb);
> >       if (fdisk_deassign_device(cxt, 0))
> >               WARN("Error deassign device %s", img->device);
> >
> >   handler_release:
> > +     if (fdisk_get_parent(doscxt))
> > +             fdisk_unref_context(doscxt);
> >       fdisk_unref_context(cxt);
> >
> >       /*
> >
>
> As far as I can see, this does not change the current behavior. Yes, it
> must be well tested (as any patch should be..)
Yeah, that's what I was going for.
>
> Best regards,
> Stefano Babic
>
> --
> =====================================================================
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de
> =====================================================================

Hi James,

On 13.05.21 09:40, James Hilliard wrote:
> On Wed, May 12, 2021 at 9:44 AM Stefano Babic <sbabic@denx.de> wrote:
>>
>> Hi James,
>>
>> On 12.05.21 06:16, James Hilliard wrote:
>>> This adds the ability to set dos partition entries for gpt disks.
>>>
>>> This is mostly useful for booting on GPT disks from boards that
>>> don't natively support GPT.
>>>
>>
>> In principle: even if using hybrid partition is discouraged, I have
>> nothing against this if it does not change the behavior for the standard
>> case, that is with hybrid off.
> I think most of the issues with hybrid partitions are due to windows dual
> boot setups, which is much less of an issue for embedded Linux systems.

ok

>>
>>> Config example:
>>> partition-1 = [
>>>           "size=550M",
>>>           "start=2048",
>>>           "name=boot",
>>>           "type=C12A7328-F81F-11D2-BA4B-00A0C93EC93B",
>>>           "fstype=vfat",
>>>           "dostype=0x0C"
>>> ];
>>>
>>
>> General remark: when you repost without RFC, you should also add
>> documentation for it, that is in handlers.rst, chapter "Disk partitioner".
> Will do.
>>
>>> Some things such as disk partition comparisons are not fully
>>> working yet.
>>>
>>
>> That is ok if it is fully documented.
> Yeah, I'm planning to fix these issues first, just haven't had the chance to
> fully track down some edge case bugs.
>>
>>> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
>>> ---
>>>    handlers/diskpart_handler.c | 208 +++++++++++++++++++++++++++++++++---
>>>    1 file changed, 195 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/handlers/diskpart_handler.c b/handlers/diskpart_handler.c
>>> index 65010c1..cefe200 100644
>>> --- a/handlers/diskpart_handler.c
>>> +++ b/handlers/diskpart_handler.c
>>> @@ -60,7 +60,8 @@ enum partfield {
>>>        PART_START,
>>>        PART_TYPE,
>>>        PART_NAME,
>>> -     PART_FSTYPE
>>> +     PART_FSTYPE,
>>> +     PART_DOSTYPE
>>>    };
>>>
>>>    const char *fields[] = {
>>> @@ -68,7 +69,8 @@ const char *fields[] = {
>>>        [PART_START] = "start",
>>>        [PART_TYPE] = "type",
>>>        [PART_NAME] = "name",
>>> -     [PART_FSTYPE] = "fstype"
>>> +     [PART_FSTYPE] = "fstype",
>>> +     [PART_DOSTYPE] = "dostype"
>>
>>
>> Just a question (I have not fully read the spec for hybrid): is
>> "dostype" a good name ? Sure, hybrid means with have also a MBR-like
>> approach, but at first glance it does not say that a hybrid partition is
>> used. Something with "hybrid", maybe (hybridtype,..) ?
> Not sure exactly if this is the best name but I don't think there is really
> much of a defined spec for hybrid dos partitions. Keep in mind basically
> all GPT formatted disks have protective dos tables, this basically just
> allows adding additional entries to the protective dos table so that the
> bootloader can find the kernel(the kernel will automatically then detect
> the GPT table and use that instead). This GPT detection behavior is at
> least well defined for the Linux kernel but might not be for other operating
> systems.

Ok, fine with me.

>>
>>>    };
>>>
>>>    struct partition_data {
>>> @@ -78,6 +80,7 @@ struct partition_data {
>>>        char type[SWUPDATE_GENERAL_STRING_SIZE];
>>>        char name[SWUPDATE_GENERAL_STRING_SIZE];
>>>        char fstype[SWUPDATE_GENERAL_STRING_SIZE];
>>> +     char dostype[SWUPDATE_GENERAL_STRING_SIZE];
>>>        LIST_ENTRY(partition_data) next;
>>>    };
>>>    LIST_HEAD(listparts, partition_data);
>>> @@ -101,8 +104,17 @@ struct hnd_priv {
>>>    static int diskpart_set_partition(struct fdisk_partition *pa,
>>>                                  struct partition_data *part,
>>>                                  unsigned long sector_size,
>>> -                               struct fdisk_parttype *parttype)
>>> +                               struct fdisk_parttype *parttype,
>>> +                               unsigned long hybrid)
>>>    {
>>> +     TRACE("set partition-%zu:%s size %" PRIu64 " start %zu type %s%s%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 / sector_size : 0,
>>> +               part->start!= LIBFDISK_INIT_UNDEF(part->start) ? part->start : 0,
>>> +               part->type,
>>> +               strlen(part->dostype) ? " dostype " : "",
>>> +               strlen(part->dostype) ? part->dostype : "");
>>
>> We have already a big TRACE() in diskpart() when properties are read.
>> This quite duplicates the verbosity of an already verbose handler. If
>> you think it is still better to have, I suggest to change from TRACE to
>> DEBUG (or drop at all if you think it is redundant).
> Yeah, this is just debug code that I'll strip out after I finish testing.

Ok

>>>        int ret = 0;
>>>
>>>        if (!sector_size)
>>> @@ -114,14 +126,14 @@ static int diskpart_set_partition(struct fdisk_partition *pa,
>>>                ret = fdisk_partition_set_start(pa, part->start);
>>>        else
>>>                ret = fdisk_partition_start_follow_default(pa, 1);
>>> -     if (part->partno != LIBFDISK_INIT_UNDEF(part->partno))
>>> +     if (!hybrid && part->partno != LIBFDISK_INIT_UNDEF(part->partno))
>>>                ret |= fdisk_partition_set_partno(pa, part->partno);
>>>        else
>>>                ret |= fdisk_partition_partno_follow_default(pa, 1);
>>> -     if (strlen(part->name))
>>> -           ret |= fdisk_partition_set_name(pa, part->name);
>>> +     if (!hybrid && strlen(part->name))
>>> +               ret |= fdisk_partition_set_name(pa, part->name);
>>>        if (part->size != LIBFDISK_INIT_UNDEF(part->size))
>>> -           ret |= fdisk_partition_set_size(pa, part->size / sector_size);
>>> +               ret |= fdisk_partition_set_size(pa, part->size / sector_size);
>>>        else
>>>                ret |= fdisk_partition_end_follow_default(pa, 1);
>>>
>>> @@ -146,7 +158,7 @@ static bool diskpart_partition_cmp(const char *lbtype, struct fdisk_partition *f
>>>                        (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) : ""))) ||
>>> +                                     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))) ||
>>> @@ -168,15 +180,23 @@ static int diskpart(struct img_type *img,
>>>        struct dict_list *parts;
>>>        struct dict_list_elem *elem;
>>>        struct fdisk_context *cxt;
>>> +     struct fdisk_context *doscxt = NULL;
>>>        struct partition_data *part;
>>>        struct partition_data *tmp;
>>>        struct fdisk_table *tb = NULL;
>>> +     struct fdisk_table *dostb = NULL;
>>>        struct fdisk_table *oldtb = NULL;
>>> +     struct fdisk_table *olddostb = NULL;
>>>        struct fdisk_parttype *parttype = NULL;
>>> +     struct fdisk_parttype *dosparttype = NULL;
>>>        int ret = 0;
>>>        unsigned long i;
>>> +     unsigned long hybrid = 0;
>>>        struct hnd_priv priv =  {FDISK_DISKLABEL_DOS};
>>>        bool createtable = false;
>>> +     bool createdostable = false;
>>> +
>>> +     //fdisk_init_debug(0xffff);
>>
>> Dead code
> Yeah, will remove once finished testing.
>>
>>>
>>>        if (!lbtype || (strcmp(lbtype, "gpt") && strcmp(lbtype, "dos"))) {
>>>                ERROR("Just GPT or DOS partition table are supported");
>>> @@ -194,6 +214,8 @@ static int diskpart(struct img_type *img,
>>>                return -ENOMEM;
>>>        }
>>>
>>> +     fdisk_disable_dialogs(cxt, 1);
>>> +
>>
>> Do we need it ? for main context was not necessary.
> Well there are some failures this fixes where different code paths are taken
> when dialogs are disabled, since we don't support interactive dialogs at all
> it's best to ensure they are disabled fully to avoid otherwise hitting an error.
> 
> See here:
> https://github.com/karelzak/util-linux/blob/ad0963685b8105920abd38e6878cc2cdfa427ea4/libfdisk/src/dos.c#L1897-L1903
> https://github.com/karelzak/util-linux/blob/ba3b93525920f2782e12016e7444fca89fb5bb9c/libfdisk/src/partition.c#L770-L771

Just because it appeared to me that the default is disable. 
fdisk_new_context() zeroes via calloc the whole structure, that means 
cxt->no_disalogs = disable = 0;

But it is ok to explicitley disable it, yes.

[1] 
https://github.com/karelzak/util-linux/blob/ad0963685b8105920abd38e6878cc2cdfa427ea4/libfdisk/src/context.c#L52

>>
>>>        ret = fdisk_assign_device(cxt, img->device, 0);
>>>        if (ret == -EACCES) {
>>>                ERROR("no access to %s", img->device);
>>> @@ -247,18 +269,31 @@ static int diskpart(struct img_type *img,
>>>                                        case PART_FSTYPE:
>>>                                                strncpy(part->fstype, equal, sizeof(part->fstype));
>>>                                                break;
>>> +                                     case PART_DOSTYPE:
>>> +                                             strncpy(part->dostype, equal, sizeof(part->dostype));
>>> +                                             hybrid++;
>>> +                                             break;
>>
>> Is there some check we should do it on the value ? dostype cannot be any
>> string.
> I basically just copied this part from how we currently parse dos
> entry types, if
> we need to add validation we should do it for non-hybrid dos type
> parsing as well.

Verify the FSTYPE adds a lot of code. Then it is ok, if we will need it, 
it will be added in future.

>>
>>>                                        }
>>>                                }
>>>                        }
>>>                        elem = LIST_NEXT(elem, next);
>>>                }
>>>
>>> -             TRACE("partition-%zu:%s size %" PRIu64 " start %zu type %s",
>>> +             if (hybrid > 3) {
>>> +                     ERROR("I cannot add hybrid partition %zu(%s): hybrid dos partition limit of 3 exceeded",
>>> +                               part->partno, strlen(part->name) ? part->name : "UNDEF NAME");
>>> +                     ret = -EINVAL;
>>> +                     goto handler_exit;
>>> +             }
>>> +
>>> +             TRACE("partition-%zu:%s size %" PRIu64 " start %zu type %s%s%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->dostype) ? " dostype " : "",
>>> +                     strlen(part->dostype) ? part->dostype : "");
>>
>> This is what I mind, there is already a long TRACE with all setup.
> Ok, so I should keep this and remove the other temporary debugging
> TRACE right?

Right.

>>
>>>
>>>                /*
>>>                 * Partitions in sw-description start from 1,
>>> @@ -287,7 +322,11 @@ static int diskpart(struct img_type *img,
>>>        if (!fdisk_has_label(cxt)) {
>>>                WARN("%s does not contain a recognized partition table",
>>>                     img->device);
>>> -             fdisk_create_disklabel(cxt, lbtype);
>>> +             ret = fdisk_create_disklabel(cxt, lbtype);
>>> +             if (ret) {
>>> +                     ERROR("Failed to create disk label");
>>> +                     goto handler_release;
>>> +             }
>>
>> This is a fix - I suggest you post this in a separate patch, together
>> with...
> Patch sent.

Thanks, I'll merge it.

>>
>>>                createtable = true;
>>>        } else if (lbtype) {
>>>                if (!strcmp(lbtype, "gpt"))
>>> @@ -298,12 +337,17 @@ static int diskpart(struct img_type *img,
>>>                if (!fdisk_is_labeltype(cxt, priv.labeltype)) {
>>>                        WARN("Partition table of different type, setting to %s, all data lost !",
>>>                                lbtype);
>>> -                     fdisk_create_disklabel(cxt, lbtype);
>>> +                     ret = fdisk_create_disklabel(cxt, lbtype);
>>> +                     if (ret) {
>>> +                             ERROR("Failed to create disk label");
>>> +                             goto handler_release;
>>> +                     }
>>
>> ...this one.
> ...with this as well.
>>
>>>                        createtable = true;
>>>                }
>>>        }
>>>
>>>        struct fdisk_label *lb = fdisk_get_label(cxt, NULL);
>>> +     struct fdisk_label *doslb = NULL;
>>>        unsigned long sector_size = fdisk_get_sector_size(cxt);
>>>
>>>        /*
>>> @@ -321,6 +365,32 @@ static int diskpart(struct img_type *img,
>>>                goto handler_exit;
>>>        }
>>>
>>> +     if (hybrid) {
>>> +             doscxt = fdisk_new_nested_context(cxt, "dos");
>>> +             fdisk_disable_dialogs(doscxt, 1);
>> See my question regarding this call.
> Yeah, we def need this to prevent failures in the codepaths I mentioned above.
>>
>>> +
>>> +             if (!fdisk_is_labeltype(doscxt, FDISK_DISKLABEL_DOS)) {
>>> +                     ret = fdisk_create_disklabel(doscxt, NULL);
>>> +                     if (ret) {
>>> +                             ERROR("Failed to create hybrid disk label");
>>> +                             goto handler_release;
>>> +                     }
>>> +                     createdostable = true;
>>> +             }
>>> +
>>> +             doslb = fdisk_get_label(doscxt, "dos");
>>> +             dostb = fdisk_new_table();
>>> +
>>> +             if (fdisk_get_partitions(doscxt, &olddostb))
>>> +                     createdostable = true;
>>> +
>>> +             if (!dostb) {
>>> +                     ERROR("OOM creating new table !");
>>> +                     ret = -ENOMEM;
>>> +                     goto handler_exit;
>>> +             }
>>> +     }
>>> +
>>>        i = 0;
>>>
>>>        LIST_FOREACH(part, &priv.listparts, next) {
>>> @@ -334,10 +404,30 @@ static int diskpart(struct img_type *img,
>>>                        parttype = fdisk_label_get_parttype_from_string(lb, part->type);
>>>                        if (!parttype)
>>>                                parttype = fdisk_label_get_parttype_from_string(lb, GPT_DEFAULT_ENTRY_TYPE);
>>> +                     if (strlen(part->dostype)) {
>>> +                             struct fdisk_partition *newdospa;
>>> +                             newdospa = fdisk_new_partition();
>>> +                             dosparttype = fdisk_label_get_parttype_from_code(doslb, ustrtoull(part->dostype, 16));
>>> +                             if (!dosparttype) {
>>> +                                     WARN("Failed to set partition type for hybrid partition: %zu(%s)", part->partno, part->name);
>>> +                             }
>>> +                             ret = diskpart_set_partition(newdospa, part, sector_size, dosparttype, hybrid);
>>> +                             if (ret) {
>>> +                                     WARN("I cannot set all hybrid partition's parameters");
>>> +                             }
>>> +                             if ((ret = fdisk_table_add_partition(dostb, newdospa)) < 0) {
>>> +                                     ERROR("I cannot add hybrid partition %zu(%s): %d", part->partno, part->name, ret);
>>> +                             }
>>> +                             fdisk_unref_partition(newdospa);
>>> +                             if (ret < 0) {
>>> +                                     fdisk_unref_partition(newpa);
>>> +                                     goto handler_exit;
>>> +                             }
>>> +                     }
>>>                } else {
>>>                        parttype = fdisk_label_get_parttype_from_code(lb, ustrtoull(part->type, 16));
>>>                }
>>> -             ret = diskpart_set_partition(newpa, part, sector_size, parttype);
>>> +             ret = diskpart_set_partition(newpa, part, sector_size, parttype, 0);
>>>                if (ret) {
>>>                        WARN("I cannot set all partition's parameters");
>>>                }
>>> @@ -350,6 +440,31 @@ static int diskpart(struct img_type *img,
>>>                i++;
>>>        }
>>>
>>> +     if (hybrid) {
>>> +             struct fdisk_partition *newpa;
>>> +             newpa = fdisk_new_partition();
>>> +             part = (struct partition_data *)calloc(1, sizeof(struct partition_data));
>>> +             if (!part) {
>>> +                     ERROR("FAULT: no memory");
>>> +                     ret = -ENOMEM;
>>> +                     goto handler_exit;
>>> +             }
>>> +             part->start = 1;
>>> +             part->size = 33 * sector_size;
>>> +             dosparttype = fdisk_label_get_parttype_from_code(doslb, 0xee);
>>> +             ret = diskpart_set_partition(newpa, part, sector_size, dosparttype, hybrid);
>>> +             if (ret) {
>>> +                     WARN("I cannot set all pmbr partition's parameters");
>>> +             }
>>> +             if ((ret = fdisk_table_add_partition(dostb, newpa)) < 0) {
>>> +                     ERROR("I cannot add the pmbr partition");
>>> +             }
>>> +             fdisk_unref_partition(newpa);
>>> +             if (ret < 0)
>>> +                     goto handler_exit;
>>> +             hybrid++;
>>> +     }
>>> +
>>>        /*
>>>         * A partiton table was found on disk, now compares the two tables
>>>         * to check if they differ.
>>> @@ -387,6 +502,69 @@ static int diskpart(struct img_type *img,
>>>                }
>>>        }
>>>
>>> +     /*
>>> +      * A dos partiton table was found on disk, now compares the two tables
>>> +      * to check if they differ.
>>> +      */
>>> +     /*
>>> +     if (hybrid && !createdostable) {
>>> +             size_t numpartondisk = fdisk_table_get_nents(olddostb);
>>> +
>>> +             i = hybrid;
>>> +             if (numpartondisk != i) {
>>> +                     TRACE("Number of hybrid partitions differs on disk: %lu <--> requested: %lu",
>>> +                               (long unsigned int)numpartondisk, i);
>>> +                     createdostable = true;
>>> +             } else {
>>> +                     struct fdisk_partition *pa, *newpa;
>>> +                     struct fdisk_iter *itr   = fdisk_new_iter(FDISK_ITER_FORWARD);
>>> +                     struct fdisk_iter *olditr = fdisk_new_iter(FDISK_ITER_FORWARD);
>>> +
>>> +                     i = 0;
>>> +                     while (i < numpartondisk && !createdostable) {
>>> +                             newpa=NULL;
>>> +                             pa = NULL;
>>> +                             if (fdisk_table_next_partition (dostb, itr, &newpa) ||
>>> +                                     fdisk_table_next_partition (olddostb, olditr, &pa)) {
>>> +                                     TRACE("Hybrid partition not found, something went wrong %lu !", i);
>>> +                                     ret = -EFAULT;
>>> +                                     goto handler_exit;
>>> +                             }
>>> +                             if (diskpart_partition_cmp("dos", pa, newpa)) {
>>> +                                     createdostable = true;
>>> +                             }
>>> +
>>> +                             fdisk_unref_partition(newpa);
>>> +                             fdisk_unref_partition(pa);
>>> +                             i++;
>>> +                     }
>>> +             }
>>> +     }
>>> +     */
>>
>> Ok, so klet the code in when it works. As first step, you can force that
>> the table must be updated if hybrid is set.
> I think I might try and clean up diskpart_partition_cmp first, the current
> implementation is a little annoying to debug as it doesn't really log all the
> differences between partitions properly.

Ok, fine.

>>
>>> +     if (hybrid) {
>>> +             // TODO fix dos table comparison
>>> +             createdostable = true;
>>> +     }
>>> +
>>> +     if (createdostable) {
>>> +             TRACE("Hybrid partitions on disk differ, write to disk;");
>>> +             fdisk_delete_all_partitions(doscxt);
>>> +             ret = fdisk_apply_table(doscxt, dostb);
>>> +             if (ret) {
>>> +                     ERROR("Hybrid partition table cannot be applied! %d", ret);
>>> +                     goto handler_exit;
>>> +             }
>>> +
>>> +             /*
>>> +              * Everything done, write into disk
>>> +              */
>>> +             ret = fdisk_write_disklabel(doscxt);
>>> +             if (ret) {
>>> +                     ERROR("Hybrid partition table cannot be written on disk %d", ret);
>>> +                     goto handler_exit;
>>> +             }
>>> +     }
>>> +
>>>        if (createtable) {
>>>                TRACE("Partitions on disk differ, write to disk;");
>>>                fdisk_delete_all_partitions(cxt);
>>> @@ -412,12 +590,16 @@ static int diskpart(struct img_type *img,
>>>    handler_exit:
>>>        if (tb)
>>>                fdisk_unref_table(tb);
>>> +     if (dostb)
>>> +             fdisk_unref_table(dostb);
>>>        if (oldtb)
>>>                fdisk_unref_table(oldtb);
>>>        if (fdisk_deassign_device(cxt, 0))
>>>                WARN("Error deassign device %s", img->device);
>>>
>>>    handler_release:
>>> +     if (fdisk_get_parent(doscxt))
>>> +             fdisk_unref_context(doscxt);
>>>        fdisk_unref_context(cxt);
>>>
>>>        /*
>>>
>>
>> As far as I can see, this does not change the current behavior. Yes, it
>> must be well tested (as any patch should be..)
> Yeah, that's what I was going for.

Best regards,
Stefano Babic

On Thu, May 13, 2021 at 3:33 AM Stefano Babic <sbabic@denx.de> wrote:
>
> Hi James,
>
> On 13.05.21 09:40, James Hilliard wrote:
> > On Wed, May 12, 2021 at 9:44 AM Stefano Babic <sbabic@denx.de> wrote:
> >>
> >> Hi James,
> >>
> >> On 12.05.21 06:16, James Hilliard wrote:
> >>> This adds the ability to set dos partition entries for gpt disks.
> >>>
> >>> This is mostly useful for booting on GPT disks from boards that
> >>> don't natively support GPT.
> >>>
> >>
> >> In principle: even if using hybrid partition is discouraged, I have
> >> nothing against this if it does not change the behavior for the standard
> >> case, that is with hybrid off.
> > I think most of the issues with hybrid partitions are due to windows dual
> > boot setups, which is much less of an issue for embedded Linux systems.
>
> ok
>
> >>
> >>> Config example:
> >>> partition-1 = [
> >>>           "size=550M",
> >>>           "start=2048",
> >>>           "name=boot",
> >>>           "type=C12A7328-F81F-11D2-BA4B-00A0C93EC93B",
> >>>           "fstype=vfat",
> >>>           "dostype=0x0C"
> >>> ];
> >>>
> >>
> >> General remark: when you repost without RFC, you should also add
> >> documentation for it, that is in handlers.rst, chapter "Disk partitioner".
> > Will do.
> >>
> >>> Some things such as disk partition comparisons are not fully
> >>> working yet.
> >>>
> >>
> >> That is ok if it is fully documented.
> > Yeah, I'm planning to fix these issues first, just haven't had the chance to
> > fully track down some edge case bugs.
> >>
> >>> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> >>> ---
> >>>    handlers/diskpart_handler.c | 208 +++++++++++++++++++++++++++++++++---
> >>>    1 file changed, 195 insertions(+), 13 deletions(-)
> >>>
> >>> diff --git a/handlers/diskpart_handler.c b/handlers/diskpart_handler.c
> >>> index 65010c1..cefe200 100644
> >>> --- a/handlers/diskpart_handler.c
> >>> +++ b/handlers/diskpart_handler.c
> >>> @@ -60,7 +60,8 @@ enum partfield {
> >>>        PART_START,
> >>>        PART_TYPE,
> >>>        PART_NAME,
> >>> -     PART_FSTYPE
> >>> +     PART_FSTYPE,
> >>> +     PART_DOSTYPE
> >>>    };
> >>>
> >>>    const char *fields[] = {
> >>> @@ -68,7 +69,8 @@ const char *fields[] = {
> >>>        [PART_START] = "start",
> >>>        [PART_TYPE] = "type",
> >>>        [PART_NAME] = "name",
> >>> -     [PART_FSTYPE] = "fstype"
> >>> +     [PART_FSTYPE] = "fstype",
> >>> +     [PART_DOSTYPE] = "dostype"
> >>
> >>
> >> Just a question (I have not fully read the spec for hybrid): is
> >> "dostype" a good name ? Sure, hybrid means with have also a MBR-like
> >> approach, but at first glance it does not say that a hybrid partition is
> >> used. Something with "hybrid", maybe (hybridtype,..) ?
> > Not sure exactly if this is the best name but I don't think there is really
> > much of a defined spec for hybrid dos partitions. Keep in mind basically
> > all GPT formatted disks have protective dos tables, this basically just
> > allows adding additional entries to the protective dos table so that the
> > bootloader can find the kernel(the kernel will automatically then detect
> > the GPT table and use that instead). This GPT detection behavior is at
> > least well defined for the Linux kernel but might not be for other operating
> > systems.
>
> Ok, fine with me.
>
> >>
> >>>    };
> >>>
> >>>    struct partition_data {
> >>> @@ -78,6 +80,7 @@ struct partition_data {
> >>>        char type[SWUPDATE_GENERAL_STRING_SIZE];
> >>>        char name[SWUPDATE_GENERAL_STRING_SIZE];
> >>>        char fstype[SWUPDATE_GENERAL_STRING_SIZE];
> >>> +     char dostype[SWUPDATE_GENERAL_STRING_SIZE];
> >>>        LIST_ENTRY(partition_data) next;
> >>>    };
> >>>    LIST_HEAD(listparts, partition_data);
> >>> @@ -101,8 +104,17 @@ struct hnd_priv {
> >>>    static int diskpart_set_partition(struct fdisk_partition *pa,
> >>>                                  struct partition_data *part,
> >>>                                  unsigned long sector_size,
> >>> -                               struct fdisk_parttype *parttype)
> >>> +                               struct fdisk_parttype *parttype,
> >>> +                               unsigned long hybrid)
> >>>    {
> >>> +     TRACE("set partition-%zu:%s size %" PRIu64 " start %zu type %s%s%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 / sector_size : 0,
> >>> +               part->start!= LIBFDISK_INIT_UNDEF(part->start) ? part->start : 0,
> >>> +               part->type,
> >>> +               strlen(part->dostype) ? " dostype " : "",
> >>> +               strlen(part->dostype) ? part->dostype : "");
> >>
> >> We have already a big TRACE() in diskpart() when properties are read.
> >> This quite duplicates the verbosity of an already verbose handler. If
> >> you think it is still better to have, I suggest to change from TRACE to
> >> DEBUG (or drop at all if you think it is redundant).
> > Yeah, this is just debug code that I'll strip out after I finish testing.
>
> Ok
>
> >>>        int ret = 0;
> >>>
> >>>        if (!sector_size)
> >>> @@ -114,14 +126,14 @@ static int diskpart_set_partition(struct fdisk_partition *pa,
> >>>                ret = fdisk_partition_set_start(pa, part->start);
> >>>        else
> >>>                ret = fdisk_partition_start_follow_default(pa, 1);
> >>> -     if (part->partno != LIBFDISK_INIT_UNDEF(part->partno))
> >>> +     if (!hybrid && part->partno != LIBFDISK_INIT_UNDEF(part->partno))
> >>>                ret |= fdisk_partition_set_partno(pa, part->partno);
> >>>        else
> >>>                ret |= fdisk_partition_partno_follow_default(pa, 1);
> >>> -     if (strlen(part->name))
> >>> -           ret |= fdisk_partition_set_name(pa, part->name);
> >>> +     if (!hybrid && strlen(part->name))
> >>> +               ret |= fdisk_partition_set_name(pa, part->name);
> >>>        if (part->size != LIBFDISK_INIT_UNDEF(part->size))
> >>> -           ret |= fdisk_partition_set_size(pa, part->size / sector_size);
> >>> +               ret |= fdisk_partition_set_size(pa, part->size / sector_size);
> >>>        else
> >>>                ret |= fdisk_partition_end_follow_default(pa, 1);
> >>>
> >>> @@ -146,7 +158,7 @@ static bool diskpart_partition_cmp(const char *lbtype, struct fdisk_partition *f
> >>>                        (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) : ""))) ||
> >>> +                                     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))) ||
> >>> @@ -168,15 +180,23 @@ static int diskpart(struct img_type *img,
> >>>        struct dict_list *parts;
> >>>        struct dict_list_elem *elem;
> >>>        struct fdisk_context *cxt;
> >>> +     struct fdisk_context *doscxt = NULL;
> >>>        struct partition_data *part;
> >>>        struct partition_data *tmp;
> >>>        struct fdisk_table *tb = NULL;
> >>> +     struct fdisk_table *dostb = NULL;
> >>>        struct fdisk_table *oldtb = NULL;
> >>> +     struct fdisk_table *olddostb = NULL;
> >>>        struct fdisk_parttype *parttype = NULL;
> >>> +     struct fdisk_parttype *dosparttype = NULL;
> >>>        int ret = 0;
> >>>        unsigned long i;
> >>> +     unsigned long hybrid = 0;
> >>>        struct hnd_priv priv =  {FDISK_DISKLABEL_DOS};
> >>>        bool createtable = false;
> >>> +     bool createdostable = false;
> >>> +
> >>> +     //fdisk_init_debug(0xffff);
> >>
> >> Dead code
> > Yeah, will remove once finished testing.
> >>
> >>>
> >>>        if (!lbtype || (strcmp(lbtype, "gpt") && strcmp(lbtype, "dos"))) {
> >>>                ERROR("Just GPT or DOS partition table are supported");
> >>> @@ -194,6 +214,8 @@ static int diskpart(struct img_type *img,
> >>>                return -ENOMEM;
> >>>        }
> >>>
> >>> +     fdisk_disable_dialogs(cxt, 1);
> >>> +
> >>
> >> Do we need it ? for main context was not necessary.
> > Well there are some failures this fixes where different code paths are taken
> > when dialogs are disabled, since we don't support interactive dialogs at all
> > it's best to ensure they are disabled fully to avoid otherwise hitting an error.
> >
> > See here:
> > https://github.com/karelzak/util-linux/blob/ad0963685b8105920abd38e6878cc2cdfa427ea4/libfdisk/src/dos.c#L1897-L1903
> > https://github.com/karelzak/util-linux/blob/ba3b93525920f2782e12016e7444fca89fb5bb9c/libfdisk/src/partition.c#L770-L771
>
> Just because it appeared to me that the default is disable.
> fdisk_new_context() zeroes via calloc the whole structure, that means
> cxt->no_disalogs = disable = 0;
Looks to me like 0 would mean dialogs are enabled however, see:
https://github.com/karelzak/util-linux/blob/1c75a85101e36ebc193183733821546f0fa430fc/libfdisk/src/context.c#L382
>
> But it is ok to explicitley disable it, yes.
>
> [1]
> https://github.com/karelzak/util-linux/blob/ad0963685b8105920abd38e6878cc2cdfa427ea4/libfdisk/src/context.c#L52
>
> >>
> >>>        ret = fdisk_assign_device(cxt, img->device, 0);
> >>>        if (ret == -EACCES) {
> >>>                ERROR("no access to %s", img->device);
> >>> @@ -247,18 +269,31 @@ static int diskpart(struct img_type *img,
> >>>                                        case PART_FSTYPE:
> >>>                                                strncpy(part->fstype, equal, sizeof(part->fstype));
> >>>                                                break;
> >>> +                                     case PART_DOSTYPE:
> >>> +                                             strncpy(part->dostype, equal, sizeof(part->dostype));
> >>> +                                             hybrid++;
> >>> +                                             break;
> >>
> >> Is there some check we should do it on the value ? dostype cannot be any
> >> string.
> > I basically just copied this part from how we currently parse dos
> > entry types, if
> > we need to add validation we should do it for non-hybrid dos type
> > parsing as well.
>
> Verify the FSTYPE adds a lot of code. Then it is ok, if we will need it,
> it will be added in future.
>
> >>
> >>>                                        }
> >>>                                }
> >>>                        }
> >>>                        elem = LIST_NEXT(elem, next);
> >>>                }
> >>>
> >>> -             TRACE("partition-%zu:%s size %" PRIu64 " start %zu type %s",
> >>> +             if (hybrid > 3) {
> >>> +                     ERROR("I cannot add hybrid partition %zu(%s): hybrid dos partition limit of 3 exceeded",
> >>> +                               part->partno, strlen(part->name) ? part->name : "UNDEF NAME");
> >>> +                     ret = -EINVAL;
> >>> +                     goto handler_exit;
> >>> +             }
> >>> +
> >>> +             TRACE("partition-%zu:%s size %" PRIu64 " start %zu type %s%s%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->dostype) ? " dostype " : "",
> >>> +                     strlen(part->dostype) ? part->dostype : "");
> >>
> >> This is what I mind, there is already a long TRACE with all setup.
> > Ok, so I should keep this and remove the other temporary debugging
> > TRACE right?
>
> Right.
>
> >>
> >>>
> >>>                /*
> >>>                 * Partitions in sw-description start from 1,
> >>> @@ -287,7 +322,11 @@ static int diskpart(struct img_type *img,
> >>>        if (!fdisk_has_label(cxt)) {
> >>>                WARN("%s does not contain a recognized partition table",
> >>>                     img->device);
> >>> -             fdisk_create_disklabel(cxt, lbtype);
> >>> +             ret = fdisk_create_disklabel(cxt, lbtype);
> >>> +             if (ret) {
> >>> +                     ERROR("Failed to create disk label");
> >>> +                     goto handler_release;
> >>> +             }
> >>
> >> This is a fix - I suggest you post this in a separate patch, together
> >> with...
> > Patch sent.
>
> Thanks, I'll merge it.
>
> >>
> >>>                createtable = true;
> >>>        } else if (lbtype) {
> >>>                if (!strcmp(lbtype, "gpt"))
> >>> @@ -298,12 +337,17 @@ static int diskpart(struct img_type *img,
> >>>                if (!fdisk_is_labeltype(cxt, priv.labeltype)) {
> >>>                        WARN("Partition table of different type, setting to %s, all data lost !",
> >>>                                lbtype);
> >>> -                     fdisk_create_disklabel(cxt, lbtype);
> >>> +                     ret = fdisk_create_disklabel(cxt, lbtype);
> >>> +                     if (ret) {
> >>> +                             ERROR("Failed to create disk label");
> >>> +                             goto handler_release;
> >>> +                     }
> >>
> >> ...this one.
> > ...with this as well.
> >>
> >>>                        createtable = true;
> >>>                }
> >>>        }
> >>>
> >>>        struct fdisk_label *lb = fdisk_get_label(cxt, NULL);
> >>> +     struct fdisk_label *doslb = NULL;
> >>>        unsigned long sector_size = fdisk_get_sector_size(cxt);
> >>>
> >>>        /*
> >>> @@ -321,6 +365,32 @@ static int diskpart(struct img_type *img,
> >>>                goto handler_exit;
> >>>        }
> >>>
> >>> +     if (hybrid) {
> >>> +             doscxt = fdisk_new_nested_context(cxt, "dos");
> >>> +             fdisk_disable_dialogs(doscxt, 1);
> >> See my question regarding this call.
> > Yeah, we def need this to prevent failures in the codepaths I mentioned above.
> >>
> >>> +
> >>> +             if (!fdisk_is_labeltype(doscxt, FDISK_DISKLABEL_DOS)) {
> >>> +                     ret = fdisk_create_disklabel(doscxt, NULL);
> >>> +                     if (ret) {
> >>> +                             ERROR("Failed to create hybrid disk label");
> >>> +                             goto handler_release;
> >>> +                     }
> >>> +                     createdostable = true;
> >>> +             }
> >>> +
> >>> +             doslb = fdisk_get_label(doscxt, "dos");
> >>> +             dostb = fdisk_new_table();
> >>> +
> >>> +             if (fdisk_get_partitions(doscxt, &olddostb))
> >>> +                     createdostable = true;
> >>> +
> >>> +             if (!dostb) {
> >>> +                     ERROR("OOM creating new table !");
> >>> +                     ret = -ENOMEM;
> >>> +                     goto handler_exit;
> >>> +             }
> >>> +     }
> >>> +
> >>>        i = 0;
> >>>
> >>>        LIST_FOREACH(part, &priv.listparts, next) {
> >>> @@ -334,10 +404,30 @@ static int diskpart(struct img_type *img,
> >>>                        parttype = fdisk_label_get_parttype_from_string(lb, part->type);
> >>>                        if (!parttype)
> >>>                                parttype = fdisk_label_get_parttype_from_string(lb, GPT_DEFAULT_ENTRY_TYPE);
> >>> +                     if (strlen(part->dostype)) {
> >>> +                             struct fdisk_partition *newdospa;
> >>> +                             newdospa = fdisk_new_partition();
> >>> +                             dosparttype = fdisk_label_get_parttype_from_code(doslb, ustrtoull(part->dostype, 16));
> >>> +                             if (!dosparttype) {
> >>> +                                     WARN("Failed to set partition type for hybrid partition: %zu(%s)", part->partno, part->name);
> >>> +                             }
> >>> +                             ret = diskpart_set_partition(newdospa, part, sector_size, dosparttype, hybrid);
> >>> +                             if (ret) {
> >>> +                                     WARN("I cannot set all hybrid partition's parameters");
> >>> +                             }
> >>> +                             if ((ret = fdisk_table_add_partition(dostb, newdospa)) < 0) {
> >>> +                                     ERROR("I cannot add hybrid partition %zu(%s): %d", part->partno, part->name, ret);
> >>> +                             }
> >>> +                             fdisk_unref_partition(newdospa);
> >>> +                             if (ret < 0) {
> >>> +                                     fdisk_unref_partition(newpa);
> >>> +                                     goto handler_exit;
> >>> +                             }
> >>> +                     }
> >>>                } else {
> >>>                        parttype = fdisk_label_get_parttype_from_code(lb, ustrtoull(part->type, 16));
> >>>                }
> >>> -             ret = diskpart_set_partition(newpa, part, sector_size, parttype);
> >>> +             ret = diskpart_set_partition(newpa, part, sector_size, parttype, 0);
> >>>                if (ret) {
> >>>                        WARN("I cannot set all partition's parameters");
> >>>                }
> >>> @@ -350,6 +440,31 @@ static int diskpart(struct img_type *img,
> >>>                i++;
> >>>        }
> >>>
> >>> +     if (hybrid) {
> >>> +             struct fdisk_partition *newpa;
> >>> +             newpa = fdisk_new_partition();
> >>> +             part = (struct partition_data *)calloc(1, sizeof(struct partition_data));
> >>> +             if (!part) {
> >>> +                     ERROR("FAULT: no memory");
> >>> +                     ret = -ENOMEM;
> >>> +                     goto handler_exit;
> >>> +             }
> >>> +             part->start = 1;
> >>> +             part->size = 33 * sector_size;
> >>> +             dosparttype = fdisk_label_get_parttype_from_code(doslb, 0xee);
> >>> +             ret = diskpart_set_partition(newpa, part, sector_size, dosparttype, hybrid);
> >>> +             if (ret) {
> >>> +                     WARN("I cannot set all pmbr partition's parameters");
> >>> +             }
> >>> +             if ((ret = fdisk_table_add_partition(dostb, newpa)) < 0) {
> >>> +                     ERROR("I cannot add the pmbr partition");
> >>> +             }
> >>> +             fdisk_unref_partition(newpa);
> >>> +             if (ret < 0)
> >>> +                     goto handler_exit;
> >>> +             hybrid++;
> >>> +     }
> >>> +
> >>>        /*
> >>>         * A partiton table was found on disk, now compares the two tables
> >>>         * to check if they differ.
> >>> @@ -387,6 +502,69 @@ static int diskpart(struct img_type *img,
> >>>                }
> >>>        }
> >>>
> >>> +     /*
> >>> +      * A dos partiton table was found on disk, now compares the two tables
> >>> +      * to check if they differ.
> >>> +      */
> >>> +     /*
> >>> +     if (hybrid && !createdostable) {
> >>> +             size_t numpartondisk = fdisk_table_get_nents(olddostb);
> >>> +
> >>> +             i = hybrid;
> >>> +             if (numpartondisk != i) {
> >>> +                     TRACE("Number of hybrid partitions differs on disk: %lu <--> requested: %lu",
> >>> +                               (long unsigned int)numpartondisk, i);
> >>> +                     createdostable = true;
> >>> +             } else {
> >>> +                     struct fdisk_partition *pa, *newpa;
> >>> +                     struct fdisk_iter *itr   = fdisk_new_iter(FDISK_ITER_FORWARD);
> >>> +                     struct fdisk_iter *olditr = fdisk_new_iter(FDISK_ITER_FORWARD);
> >>> +
> >>> +                     i = 0;
> >>> +                     while (i < numpartondisk && !createdostable) {
> >>> +                             newpa=NULL;
> >>> +                             pa = NULL;
> >>> +                             if (fdisk_table_next_partition (dostb, itr, &newpa) ||
> >>> +                                     fdisk_table_next_partition (olddostb, olditr, &pa)) {
> >>> +                                     TRACE("Hybrid partition not found, something went wrong %lu !", i);
> >>> +                                     ret = -EFAULT;
> >>> +                                     goto handler_exit;
> >>> +                             }
> >>> +                             if (diskpart_partition_cmp("dos", pa, newpa)) {
> >>> +                                     createdostable = true;
> >>> +                             }
> >>> +
> >>> +                             fdisk_unref_partition(newpa);
> >>> +                             fdisk_unref_partition(pa);
> >>> +                             i++;
> >>> +                     }
> >>> +             }
> >>> +     }
> >>> +     */
> >>
> >> Ok, so klet the code in when it works. As first step, you can force that
> >> the table must be updated if hybrid is set.
> > I think I might try and clean up diskpart_partition_cmp first, the current
> > implementation is a little annoying to debug as it doesn't really log all the
> > differences between partitions properly.
>
> Ok, fine.
>
> >>
> >>> +     if (hybrid) {
> >>> +             // TODO fix dos table comparison
> >>> +             createdostable = true;
> >>> +     }
> >>> +
> >>> +     if (createdostable) {
> >>> +             TRACE("Hybrid partitions on disk differ, write to disk;");
> >>> +             fdisk_delete_all_partitions(doscxt);
> >>> +             ret = fdisk_apply_table(doscxt, dostb);
> >>> +             if (ret) {
> >>> +                     ERROR("Hybrid partition table cannot be applied! %d", ret);
> >>> +                     goto handler_exit;
> >>> +             }
> >>> +
> >>> +             /*
> >>> +              * Everything done, write into disk
> >>> +              */
> >>> +             ret = fdisk_write_disklabel(doscxt);
> >>> +             if (ret) {
> >>> +                     ERROR("Hybrid partition table cannot be written on disk %d", ret);
> >>> +                     goto handler_exit;
> >>> +             }
> >>> +     }
> >>> +
> >>>        if (createtable) {
> >>>                TRACE("Partitions on disk differ, write to disk;");
> >>>                fdisk_delete_all_partitions(cxt);
> >>> @@ -412,12 +590,16 @@ static int diskpart(struct img_type *img,
> >>>    handler_exit:
> >>>        if (tb)
> >>>                fdisk_unref_table(tb);
> >>> +     if (dostb)
> >>> +             fdisk_unref_table(dostb);
> >>>        if (oldtb)
> >>>                fdisk_unref_table(oldtb);
> >>>        if (fdisk_deassign_device(cxt, 0))
> >>>                WARN("Error deassign device %s", img->device);
> >>>
> >>>    handler_release:
> >>> +     if (fdisk_get_parent(doscxt))
> >>> +             fdisk_unref_context(doscxt);
> >>>        fdisk_unref_context(cxt);
> >>>
> >>>        /*
> >>>
> >>
> >> As far as I can see, this does not change the current behavior. Yes, it
> >> must be well tested (as any patch should be..)
> > Yeah, that's what I was going for.
>
> Best regards,
> Stefano Babic
>
>
> --
> =====================================================================
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de
> =====================================================================

On Thu, May 13, 2021 at 3:33 AM Stefano Babic <sbabic@denx.de> wrote:
>
> Hi James,
>
> On 13.05.21 09:40, James Hilliard wrote:
> > On Wed, May 12, 2021 at 9:44 AM Stefano Babic <sbabic@denx.de> wrote:
> >>
> >> Hi James,
> >>
> >> On 12.05.21 06:16, James Hilliard wrote:
> >>> This adds the ability to set dos partition entries for gpt disks.
> >>>
> >>> This is mostly useful for booting on GPT disks from boards that
> >>> don't natively support GPT.
> >>>
> >>
> >> In principle: even if using hybrid partition is discouraged, I have
> >> nothing against this if it does not change the behavior for the standard
> >> case, that is with hybrid off.
> > I think most of the issues with hybrid partitions are due to windows dual
> > boot setups, which is much less of an issue for embedded Linux systems.
>
> ok
>
> >>
> >>> Config example:
> >>> partition-1 = [
> >>>           "size=550M",
> >>>           "start=2048",
> >>>           "name=boot",
> >>>           "type=C12A7328-F81F-11D2-BA4B-00A0C93EC93B",
> >>>           "fstype=vfat",
> >>>           "dostype=0x0C"
> >>> ];
> >>>
> >>
> >> General remark: when you repost without RFC, you should also add
> >> documentation for it, that is in handlers.rst, chapter "Disk partitioner".
> > Will do.
> >>
> >>> Some things such as disk partition comparisons are not fully
> >>> working yet.
> >>>
> >>
> >> That is ok if it is fully documented.
> > Yeah, I'm planning to fix these issues first, just haven't had the chance to
> > fully track down some edge case bugs.
> >>
> >>> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> >>> ---
> >>>    handlers/diskpart_handler.c | 208 +++++++++++++++++++++++++++++++++---
> >>>    1 file changed, 195 insertions(+), 13 deletions(-)
> >>>
> >>> diff --git a/handlers/diskpart_handler.c b/handlers/diskpart_handler.c
> >>> index 65010c1..cefe200 100644
> >>> --- a/handlers/diskpart_handler.c
> >>> +++ b/handlers/diskpart_handler.c
> >>> @@ -60,7 +60,8 @@ enum partfield {
> >>>        PART_START,
> >>>        PART_TYPE,
> >>>        PART_NAME,
> >>> -     PART_FSTYPE
> >>> +     PART_FSTYPE,
> >>> +     PART_DOSTYPE
> >>>    };
> >>>
> >>>    const char *fields[] = {
> >>> @@ -68,7 +69,8 @@ const char *fields[] = {
> >>>        [PART_START] = "start",
> >>>        [PART_TYPE] = "type",
> >>>        [PART_NAME] = "name",
> >>> -     [PART_FSTYPE] = "fstype"
> >>> +     [PART_FSTYPE] = "fstype",
> >>> +     [PART_DOSTYPE] = "dostype"
> >>
> >>
> >> Just a question (I have not fully read the spec for hybrid): is
> >> "dostype" a good name ? Sure, hybrid means with have also a MBR-like
> >> approach, but at first glance it does not say that a hybrid partition is
> >> used. Something with "hybrid", maybe (hybridtype,..) ?
> > Not sure exactly if this is the best name but I don't think there is really
> > much of a defined spec for hybrid dos partitions. Keep in mind basically
> > all GPT formatted disks have protective dos tables, this basically just
> > allows adding additional entries to the protective dos table so that the
> > bootloader can find the kernel(the kernel will automatically then detect
> > the GPT table and use that instead). This GPT detection behavior is at
> > least well defined for the Linux kernel but might not be for other operating
> > systems.
>
> Ok, fine with me.
>
> >>
> >>>    };
> >>>
> >>>    struct partition_data {
> >>> @@ -78,6 +80,7 @@ struct partition_data {
> >>>        char type[SWUPDATE_GENERAL_STRING_SIZE];
> >>>        char name[SWUPDATE_GENERAL_STRING_SIZE];
> >>>        char fstype[SWUPDATE_GENERAL_STRING_SIZE];
> >>> +     char dostype[SWUPDATE_GENERAL_STRING_SIZE];
> >>>        LIST_ENTRY(partition_data) next;
> >>>    };
> >>>    LIST_HEAD(listparts, partition_data);
> >>> @@ -101,8 +104,17 @@ struct hnd_priv {
> >>>    static int diskpart_set_partition(struct fdisk_partition *pa,
> >>>                                  struct partition_data *part,
> >>>                                  unsigned long sector_size,
> >>> -                               struct fdisk_parttype *parttype)
> >>> +                               struct fdisk_parttype *parttype,
> >>> +                               unsigned long hybrid)
> >>>    {
> >>> +     TRACE("set partition-%zu:%s size %" PRIu64 " start %zu type %s%s%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 / sector_size : 0,
> >>> +               part->start!= LIBFDISK_INIT_UNDEF(part->start) ? part->start : 0,
> >>> +               part->type,
> >>> +               strlen(part->dostype) ? " dostype " : "",
> >>> +               strlen(part->dostype) ? part->dostype : "");
> >>
> >> We have already a big TRACE() in diskpart() when properties are read.
> >> This quite duplicates the verbosity of an already verbose handler. If
> >> you think it is still better to have, I suggest to change from TRACE to
> >> DEBUG (or drop at all if you think it is redundant).
> > Yeah, this is just debug code that I'll strip out after I finish testing.
>
> Ok
>
> >>>        int ret = 0;
> >>>
> >>>        if (!sector_size)
> >>> @@ -114,14 +126,14 @@ static int diskpart_set_partition(struct fdisk_partition *pa,
> >>>                ret = fdisk_partition_set_start(pa, part->start);
> >>>        else
> >>>                ret = fdisk_partition_start_follow_default(pa, 1);
> >>> -     if (part->partno != LIBFDISK_INIT_UNDEF(part->partno))
> >>> +     if (!hybrid && part->partno != LIBFDISK_INIT_UNDEF(part->partno))
> >>>                ret |= fdisk_partition_set_partno(pa, part->partno);
> >>>        else
> >>>                ret |= fdisk_partition_partno_follow_default(pa, 1);
> >>> -     if (strlen(part->name))
> >>> -           ret |= fdisk_partition_set_name(pa, part->name);
> >>> +     if (!hybrid && strlen(part->name))
> >>> +               ret |= fdisk_partition_set_name(pa, part->name);
> >>>        if (part->size != LIBFDISK_INIT_UNDEF(part->size))
> >>> -           ret |= fdisk_partition_set_size(pa, part->size / sector_size);
> >>> +               ret |= fdisk_partition_set_size(pa, part->size / sector_size);
> >>>        else
> >>>                ret |= fdisk_partition_end_follow_default(pa, 1);
> >>>
> >>> @@ -146,7 +158,7 @@ static bool diskpart_partition_cmp(const char *lbtype, struct fdisk_partition *f
> >>>                        (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) : ""))) ||
> >>> +                                     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))) ||
> >>> @@ -168,15 +180,23 @@ static int diskpart(struct img_type *img,
> >>>        struct dict_list *parts;
> >>>        struct dict_list_elem *elem;
> >>>        struct fdisk_context *cxt;
> >>> +     struct fdisk_context *doscxt = NULL;
> >>>        struct partition_data *part;
> >>>        struct partition_data *tmp;
> >>>        struct fdisk_table *tb = NULL;
> >>> +     struct fdisk_table *dostb = NULL;
> >>>        struct fdisk_table *oldtb = NULL;
> >>> +     struct fdisk_table *olddostb = NULL;
> >>>        struct fdisk_parttype *parttype = NULL;
> >>> +     struct fdisk_parttype *dosparttype = NULL;
> >>>        int ret = 0;
> >>>        unsigned long i;
> >>> +     unsigned long hybrid = 0;
> >>>        struct hnd_priv priv =  {FDISK_DISKLABEL_DOS};
> >>>        bool createtable = false;
> >>> +     bool createdostable = false;
> >>> +
> >>> +     //fdisk_init_debug(0xffff);
> >>
> >> Dead code
> > Yeah, will remove once finished testing.
> >>
> >>>
> >>>        if (!lbtype || (strcmp(lbtype, "gpt") && strcmp(lbtype, "dos"))) {
> >>>                ERROR("Just GPT or DOS partition table are supported");
> >>> @@ -194,6 +214,8 @@ static int diskpart(struct img_type *img,
> >>>                return -ENOMEM;
> >>>        }
> >>>
> >>> +     fdisk_disable_dialogs(cxt, 1);
> >>> +
> >>
> >> Do we need it ? for main context was not necessary.
> > Well there are some failures this fixes where different code paths are taken
> > when dialogs are disabled, since we don't support interactive dialogs at all
> > it's best to ensure they are disabled fully to avoid otherwise hitting an error.
> >
> > See here:
> > https://github.com/karelzak/util-linux/blob/ad0963685b8105920abd38e6878cc2cdfa427ea4/libfdisk/src/dos.c#L1897-L1903
> > https://github.com/karelzak/util-linux/blob/ba3b93525920f2782e12016e7444fca89fb5bb9c/libfdisk/src/partition.c#L770-L771
>
> Just because it appeared to me that the default is disable.
> fdisk_new_context() zeroes via calloc the whole structure, that means
> cxt->no_disalogs = disable = 0;
>
> But it is ok to explicitley disable it, yes.
>
> [1]
> https://github.com/karelzak/util-linux/blob/ad0963685b8105920abd38e6878cc2cdfa427ea4/libfdisk/src/context.c#L52
>
> >>
> >>>        ret = fdisk_assign_device(cxt, img->device, 0);
> >>>        if (ret == -EACCES) {
> >>>                ERROR("no access to %s", img->device);
> >>> @@ -247,18 +269,31 @@ static int diskpart(struct img_type *img,
> >>>                                        case PART_FSTYPE:
> >>>                                                strncpy(part->fstype, equal, sizeof(part->fstype));
> >>>                                                break;
> >>> +                                     case PART_DOSTYPE:
> >>> +                                             strncpy(part->dostype, equal, sizeof(part->dostype));
> >>> +                                             hybrid++;
> >>> +                                             break;
> >>
> >> Is there some check we should do it on the value ? dostype cannot be any
> >> string.
> > I basically just copied this part from how we currently parse dos
> > entry types, if
> > we need to add validation we should do it for non-hybrid dos type
> > parsing as well.
>
> Verify the FSTYPE adds a lot of code. Then it is ok, if we will need it,
> it will be added in future.
>
> >>
> >>>                                        }
> >>>                                }
> >>>                        }
> >>>                        elem = LIST_NEXT(elem, next);
> >>>                }
> >>>
> >>> -             TRACE("partition-%zu:%s size %" PRIu64 " start %zu type %s",
> >>> +             if (hybrid > 3) {
> >>> +                     ERROR("I cannot add hybrid partition %zu(%s): hybrid dos partition limit of 3 exceeded",
> >>> +                               part->partno, strlen(part->name) ? part->name : "UNDEF NAME");
> >>> +                     ret = -EINVAL;
> >>> +                     goto handler_exit;
> >>> +             }
> >>> +
> >>> +             TRACE("partition-%zu:%s size %" PRIu64 " start %zu type %s%s%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->dostype) ? " dostype " : "",
> >>> +                     strlen(part->dostype) ? part->dostype : "");
> >>
> >> This is what I mind, there is already a long TRACE with all setup.
> > Ok, so I should keep this and remove the other temporary debugging
> > TRACE right?
>
> Right.
>
> >>
> >>>
> >>>                /*
> >>>                 * Partitions in sw-description start from 1,
> >>> @@ -287,7 +322,11 @@ static int diskpart(struct img_type *img,
> >>>        if (!fdisk_has_label(cxt)) {
> >>>                WARN("%s does not contain a recognized partition table",
> >>>                     img->device);
> >>> -             fdisk_create_disklabel(cxt, lbtype);
> >>> +             ret = fdisk_create_disklabel(cxt, lbtype);
> >>> +             if (ret) {
> >>> +                     ERROR("Failed to create disk label");
> >>> +                     goto handler_release;
> >>> +             }
> >>
> >> This is a fix - I suggest you post this in a separate patch, together
> >> with...
> > Patch sent.
>
> Thanks, I'll merge it.
>
> >>
> >>>                createtable = true;
> >>>        } else if (lbtype) {
> >>>                if (!strcmp(lbtype, "gpt"))
> >>> @@ -298,12 +337,17 @@ static int diskpart(struct img_type *img,
> >>>                if (!fdisk_is_labeltype(cxt, priv.labeltype)) {
> >>>                        WARN("Partition table of different type, setting to %s, all data lost !",
> >>>                                lbtype);
> >>> -                     fdisk_create_disklabel(cxt, lbtype);
> >>> +                     ret = fdisk_create_disklabel(cxt, lbtype);
> >>> +                     if (ret) {
> >>> +                             ERROR("Failed to create disk label");
> >>> +                             goto handler_release;
> >>> +                     }
> >>
> >> ...this one.
> > ...with this as well.
> >>
> >>>                        createtable = true;
> >>>                }
> >>>        }
> >>>
> >>>        struct fdisk_label *lb = fdisk_get_label(cxt, NULL);
> >>> +     struct fdisk_label *doslb = NULL;
> >>>        unsigned long sector_size = fdisk_get_sector_size(cxt);
> >>>
> >>>        /*
> >>> @@ -321,6 +365,32 @@ static int diskpart(struct img_type *img,
> >>>                goto handler_exit;
> >>>        }
> >>>
> >>> +     if (hybrid) {
> >>> +             doscxt = fdisk_new_nested_context(cxt, "dos");
> >>> +             fdisk_disable_dialogs(doscxt, 1);
> >> See my question regarding this call.
> > Yeah, we def need this to prevent failures in the codepaths I mentioned above.
> >>
> >>> +
> >>> +             if (!fdisk_is_labeltype(doscxt, FDISK_DISKLABEL_DOS)) {
> >>> +                     ret = fdisk_create_disklabel(doscxt, NULL);
> >>> +                     if (ret) {
> >>> +                             ERROR("Failed to create hybrid disk label");
> >>> +                             goto handler_release;
> >>> +                     }
> >>> +                     createdostable = true;
> >>> +             }
> >>> +
> >>> +             doslb = fdisk_get_label(doscxt, "dos");
> >>> +             dostb = fdisk_new_table();
> >>> +
> >>> +             if (fdisk_get_partitions(doscxt, &olddostb))
> >>> +                     createdostable = true;
> >>> +
> >>> +             if (!dostb) {
> >>> +                     ERROR("OOM creating new table !");
> >>> +                     ret = -ENOMEM;
> >>> +                     goto handler_exit;
> >>> +             }
> >>> +     }
> >>> +
> >>>        i = 0;
> >>>
> >>>        LIST_FOREACH(part, &priv.listparts, next) {
> >>> @@ -334,10 +404,30 @@ static int diskpart(struct img_type *img,
> >>>                        parttype = fdisk_label_get_parttype_from_string(lb, part->type);
> >>>                        if (!parttype)
> >>>                                parttype = fdisk_label_get_parttype_from_string(lb, GPT_DEFAULT_ENTRY_TYPE);
> >>> +                     if (strlen(part->dostype)) {
> >>> +                             struct fdisk_partition *newdospa;
> >>> +                             newdospa = fdisk_new_partition();
> >>> +                             dosparttype = fdisk_label_get_parttype_from_code(doslb, ustrtoull(part->dostype, 16));
> >>> +                             if (!dosparttype) {
> >>> +                                     WARN("Failed to set partition type for hybrid partition: %zu(%s)", part->partno, part->name);
> >>> +                             }
> >>> +                             ret = diskpart_set_partition(newdospa, part, sector_size, dosparttype, hybrid);
> >>> +                             if (ret) {
> >>> +                                     WARN("I cannot set all hybrid partition's parameters");
> >>> +                             }
> >>> +                             if ((ret = fdisk_table_add_partition(dostb, newdospa)) < 0) {
> >>> +                                     ERROR("I cannot add hybrid partition %zu(%s): %d", part->partno, part->name, ret);
> >>> +                             }
> >>> +                             fdisk_unref_partition(newdospa);
> >>> +                             if (ret < 0) {
> >>> +                                     fdisk_unref_partition(newpa);
> >>> +                                     goto handler_exit;
> >>> +                             }
> >>> +                     }
> >>>                } else {
> >>>                        parttype = fdisk_label_get_parttype_from_code(lb, ustrtoull(part->type, 16));
> >>>                }
> >>> -             ret = diskpart_set_partition(newpa, part, sector_size, parttype);
> >>> +             ret = diskpart_set_partition(newpa, part, sector_size, parttype, 0);
> >>>                if (ret) {
> >>>                        WARN("I cannot set all partition's parameters");
> >>>                }
> >>> @@ -350,6 +440,31 @@ static int diskpart(struct img_type *img,
> >>>                i++;
> >>>        }
> >>>
> >>> +     if (hybrid) {
> >>> +             struct fdisk_partition *newpa;
> >>> +             newpa = fdisk_new_partition();
> >>> +             part = (struct partition_data *)calloc(1, sizeof(struct partition_data));
> >>> +             if (!part) {
> >>> +                     ERROR("FAULT: no memory");
> >>> +                     ret = -ENOMEM;
> >>> +                     goto handler_exit;
> >>> +             }
> >>> +             part->start = 1;
> >>> +             part->size = 33 * sector_size;
> >>> +             dosparttype = fdisk_label_get_parttype_from_code(doslb, 0xee);
> >>> +             ret = diskpart_set_partition(newpa, part, sector_size, dosparttype, hybrid);
> >>> +             if (ret) {
> >>> +                     WARN("I cannot set all pmbr partition's parameters");
> >>> +             }
> >>> +             if ((ret = fdisk_table_add_partition(dostb, newpa)) < 0) {
> >>> +                     ERROR("I cannot add the pmbr partition");
> >>> +             }
> >>> +             fdisk_unref_partition(newpa);
> >>> +             if (ret < 0)
> >>> +                     goto handler_exit;
> >>> +             hybrid++;
> >>> +     }
> >>> +
> >>>        /*
> >>>         * A partiton table was found on disk, now compares the two tables
> >>>         * to check if they differ.
> >>> @@ -387,6 +502,69 @@ static int diskpart(struct img_type *img,
> >>>                }
> >>>        }
> >>>
> >>> +     /*
> >>> +      * A dos partiton table was found on disk, now compares the two tables
> >>> +      * to check if they differ.
> >>> +      */
> >>> +     /*
> >>> +     if (hybrid && !createdostable) {
> >>> +             size_t numpartondisk = fdisk_table_get_nents(olddostb);
> >>> +
> >>> +             i = hybrid;
> >>> +             if (numpartondisk != i) {
> >>> +                     TRACE("Number of hybrid partitions differs on disk: %lu <--> requested: %lu",
> >>> +                               (long unsigned int)numpartondisk, i);
> >>> +                     createdostable = true;
> >>> +             } else {
> >>> +                     struct fdisk_partition *pa, *newpa;
> >>> +                     struct fdisk_iter *itr   = fdisk_new_iter(FDISK_ITER_FORWARD);
> >>> +                     struct fdisk_iter *olditr = fdisk_new_iter(FDISK_ITER_FORWARD);
> >>> +
> >>> +                     i = 0;
> >>> +                     while (i < numpartondisk && !createdostable) {
> >>> +                             newpa=NULL;
> >>> +                             pa = NULL;
> >>> +                             if (fdisk_table_next_partition (dostb, itr, &newpa) ||
> >>> +                                     fdisk_table_next_partition (olddostb, olditr, &pa)) {
> >>> +                                     TRACE("Hybrid partition not found, something went wrong %lu !", i);
> >>> +                                     ret = -EFAULT;
> >>> +                                     goto handler_exit;
> >>> +                             }
> >>> +                             if (diskpart_partition_cmp("dos", pa, newpa)) {
> >>> +                                     createdostable = true;
> >>> +                             }
> >>> +
> >>> +                             fdisk_unref_partition(newpa);
> >>> +                             fdisk_unref_partition(pa);
> >>> +                             i++;
> >>> +                     }
> >>> +             }
> >>> +     }
> >>> +     */
> >>
> >> Ok, so klet the code in when it works. As first step, you can force that
> >> the table must be updated if hybrid is set.
> > I think I might try and clean up diskpart_partition_cmp first, the current
> > implementation is a little annoying to debug as it doesn't really log all the
> > differences between partitions properly.
>
> Ok, fine.
Ok, so I was able to determine that the diskpart_partition_cmp bug I was
running into is not limited to hybrid partitions and is an existing bug.

The bug seems to be that diskpart_partition_cmp does not properly handle
automatically sized partitions, here's a reproducer:
software =
{
        version = "0.1.0";

        partitions: (
        {
                type = "diskpart";
                device = "/dev/sdb";
                properties: {
                        labeltype = "gpt";
                        partition-1 = [
                                "size=550M",
                                "start=2048",
                                "name=boot",
                                "type=C12A7328-F81F-11D2-BA4B-00A0C93EC93B",
                                "fstype=vfat"
                        ];
                        partition-2 = [
                                "size=1G",
                                "name=main",
                                "type=4F68BCE3-E8CD-4DB1-96E7-FBCAF984B709",
                                "fstype=ext4"
                        ];
                        partition-3 = [
                                "size=1G",
                                "name=alt",
                                "type=4F68BCE3-E8CD-4DB1-96E7-FBCAF984B709",
                                "fstype=ext4"
                        ];
                        partition-4 = [
                                "name=srv",
                                "type=3B8F8425-20E0-4F3B-907F-1A25A76F98E8",
                                "fstype=ext4"
                        ];
                }
        });
}
partition-4 is a bulk data partition that is automatically sized to
fill the remaining
disk space, this config works fine during the initial format but the comparison
function doesn't seem to appropriately handle automatically sized partitions
for some reason on subsequent runs.
>
> >>
> >>> +     if (hybrid) {
> >>> +             // TODO fix dos table comparison
> >>> +             createdostable = true;
> >>> +     }
> >>> +
> >>> +     if (createdostable) {
> >>> +             TRACE("Hybrid partitions on disk differ, write to disk;");
> >>> +             fdisk_delete_all_partitions(doscxt);
> >>> +             ret = fdisk_apply_table(doscxt, dostb);
> >>> +             if (ret) {
> >>> +                     ERROR("Hybrid partition table cannot be applied! %d", ret);
> >>> +                     goto handler_exit;
> >>> +             }
> >>> +
> >>> +             /*
> >>> +              * Everything done, write into disk
> >>> +              */
> >>> +             ret = fdisk_write_disklabel(doscxt);
> >>> +             if (ret) {
> >>> +                     ERROR("Hybrid partition table cannot be written on disk %d", ret);
> >>> +                     goto handler_exit;
> >>> +             }
> >>> +     }
> >>> +
> >>>        if (createtable) {
> >>>                TRACE("Partitions on disk differ, write to disk;");
> >>>                fdisk_delete_all_partitions(cxt);
> >>> @@ -412,12 +590,16 @@ static int diskpart(struct img_type *img,
> >>>    handler_exit:
> >>>        if (tb)
> >>>                fdisk_unref_table(tb);
> >>> +     if (dostb)
> >>> +             fdisk_unref_table(dostb);
> >>>        if (oldtb)
> >>>                fdisk_unref_table(oldtb);
> >>>        if (fdisk_deassign_device(cxt, 0))
> >>>                WARN("Error deassign device %s", img->device);
> >>>
> >>>    handler_release:
> >>> +     if (fdisk_get_parent(doscxt))
> >>> +             fdisk_unref_context(doscxt);
> >>>        fdisk_unref_context(cxt);
> >>>
> >>>        /*
> >>>
> >>
> >> As far as I can see, this does not change the current behavior. Yes, it
> >> must be well tested (as any patch should be..)
> > Yeah, that's what I was going for.
>
> Best regards,
> Stefano Babic
>
>
> --
> =====================================================================
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de
> =====================================================================

On 14.05.21 10:34, James Hilliard wrote:
> On Thu, May 13, 2021 at 3:33 AM Stefano Babic <sbabic@denx.de> wrote:
>>
>> Hi James,
>>
>> On 13.05.21 09:40, James Hilliard wrote:
>>> On Wed, May 12, 2021 at 9:44 AM Stefano Babic <sbabic@denx.de> wrote:
>>>>
>>>> Hi James,
>>>>
>>>> On 12.05.21 06:16, James Hilliard wrote:
>>>>> This adds the ability to set dos partition entries for gpt disks.
>>>>>
>>>>> This is mostly useful for booting on GPT disks from boards that
>>>>> don't natively support GPT.
>>>>>
>>>>
>>>> In principle: even if using hybrid partition is discouraged, I have
>>>> nothing against this if it does not change the behavior for the standard
>>>> case, that is with hybrid off.
>>> I think most of the issues with hybrid partitions are due to windows dual
>>> boot setups, which is much less of an issue for embedded Linux systems.
>>
>> ok
>>
>>>>
>>>>> Config example:
>>>>> partition-1 = [
>>>>>            "size=550M",
>>>>>            "start=2048",
>>>>>            "name=boot",
>>>>>            "type=C12A7328-F81F-11D2-BA4B-00A0C93EC93B",
>>>>>            "fstype=vfat",
>>>>>            "dostype=0x0C"
>>>>> ];
>>>>>
>>>>
>>>> General remark: when you repost without RFC, you should also add
>>>> documentation for it, that is in handlers.rst, chapter "Disk partitioner".
>>> Will do.
>>>>
>>>>> Some things such as disk partition comparisons are not fully
>>>>> working yet.
>>>>>
>>>>
>>>> That is ok if it is fully documented.
>>> Yeah, I'm planning to fix these issues first, just haven't had the chance to
>>> fully track down some edge case bugs.
>>>>
>>>>> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
>>>>> ---
>>>>>     handlers/diskpart_handler.c | 208 +++++++++++++++++++++++++++++++++---
>>>>>     1 file changed, 195 insertions(+), 13 deletions(-)
>>>>>
>>>>> diff --git a/handlers/diskpart_handler.c b/handlers/diskpart_handler.c
>>>>> index 65010c1..cefe200 100644
>>>>> --- a/handlers/diskpart_handler.c
>>>>> +++ b/handlers/diskpart_handler.c
>>>>> @@ -60,7 +60,8 @@ enum partfield {
>>>>>         PART_START,
>>>>>         PART_TYPE,
>>>>>         PART_NAME,
>>>>> -     PART_FSTYPE
>>>>> +     PART_FSTYPE,
>>>>> +     PART_DOSTYPE
>>>>>     };
>>>>>
>>>>>     const char *fields[] = {
>>>>> @@ -68,7 +69,8 @@ const char *fields[] = {
>>>>>         [PART_START] = "start",
>>>>>         [PART_TYPE] = "type",
>>>>>         [PART_NAME] = "name",
>>>>> -     [PART_FSTYPE] = "fstype"
>>>>> +     [PART_FSTYPE] = "fstype",
>>>>> +     [PART_DOSTYPE] = "dostype"
>>>>
>>>>
>>>> Just a question (I have not fully read the spec for hybrid): is
>>>> "dostype" a good name ? Sure, hybrid means with have also a MBR-like
>>>> approach, but at first glance it does not say that a hybrid partition is
>>>> used. Something with "hybrid", maybe (hybridtype,..) ?
>>> Not sure exactly if this is the best name but I don't think there is really
>>> much of a defined spec for hybrid dos partitions. Keep in mind basically
>>> all GPT formatted disks have protective dos tables, this basically just
>>> allows adding additional entries to the protective dos table so that the
>>> bootloader can find the kernel(the kernel will automatically then detect
>>> the GPT table and use that instead). This GPT detection behavior is at
>>> least well defined for the Linux kernel but might not be for other operating
>>> systems.
>>
>> Ok, fine with me.
>>
>>>>
>>>>>     };
>>>>>
>>>>>     struct partition_data {
>>>>> @@ -78,6 +80,7 @@ struct partition_data {
>>>>>         char type[SWUPDATE_GENERAL_STRING_SIZE];
>>>>>         char name[SWUPDATE_GENERAL_STRING_SIZE];
>>>>>         char fstype[SWUPDATE_GENERAL_STRING_SIZE];
>>>>> +     char dostype[SWUPDATE_GENERAL_STRING_SIZE];
>>>>>         LIST_ENTRY(partition_data) next;
>>>>>     };
>>>>>     LIST_HEAD(listparts, partition_data);
>>>>> @@ -101,8 +104,17 @@ struct hnd_priv {
>>>>>     static int diskpart_set_partition(struct fdisk_partition *pa,
>>>>>                                   struct partition_data *part,
>>>>>                                   unsigned long sector_size,
>>>>> -                               struct fdisk_parttype *parttype)
>>>>> +                               struct fdisk_parttype *parttype,
>>>>> +                               unsigned long hybrid)
>>>>>     {
>>>>> +     TRACE("set partition-%zu:%s size %" PRIu64 " start %zu type %s%s%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 / sector_size : 0,
>>>>> +               part->start!= LIBFDISK_INIT_UNDEF(part->start) ? part->start : 0,
>>>>> +               part->type,
>>>>> +               strlen(part->dostype) ? " dostype " : "",
>>>>> +               strlen(part->dostype) ? part->dostype : "");
>>>>
>>>> We have already a big TRACE() in diskpart() when properties are read.
>>>> This quite duplicates the verbosity of an already verbose handler. If
>>>> you think it is still better to have, I suggest to change from TRACE to
>>>> DEBUG (or drop at all if you think it is redundant).
>>> Yeah, this is just debug code that I'll strip out after I finish testing.
>>
>> Ok
>>
>>>>>         int ret = 0;
>>>>>
>>>>>         if (!sector_size)
>>>>> @@ -114,14 +126,14 @@ static int diskpart_set_partition(struct fdisk_partition *pa,
>>>>>                 ret = fdisk_partition_set_start(pa, part->start);
>>>>>         else
>>>>>                 ret = fdisk_partition_start_follow_default(pa, 1);
>>>>> -     if (part->partno != LIBFDISK_INIT_UNDEF(part->partno))
>>>>> +     if (!hybrid && part->partno != LIBFDISK_INIT_UNDEF(part->partno))
>>>>>                 ret |= fdisk_partition_set_partno(pa, part->partno);
>>>>>         else
>>>>>                 ret |= fdisk_partition_partno_follow_default(pa, 1);
>>>>> -     if (strlen(part->name))
>>>>> -           ret |= fdisk_partition_set_name(pa, part->name);
>>>>> +     if (!hybrid && strlen(part->name))
>>>>> +               ret |= fdisk_partition_set_name(pa, part->name);
>>>>>         if (part->size != LIBFDISK_INIT_UNDEF(part->size))
>>>>> -           ret |= fdisk_partition_set_size(pa, part->size / sector_size);
>>>>> +               ret |= fdisk_partition_set_size(pa, part->size / sector_size);
>>>>>         else
>>>>>                 ret |= fdisk_partition_end_follow_default(pa, 1);
>>>>>
>>>>> @@ -146,7 +158,7 @@ static bool diskpart_partition_cmp(const char *lbtype, struct fdisk_partition *f
>>>>>                         (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) : ""))) ||
>>>>> +                                     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))) ||
>>>>> @@ -168,15 +180,23 @@ static int diskpart(struct img_type *img,
>>>>>         struct dict_list *parts;
>>>>>         struct dict_list_elem *elem;
>>>>>         struct fdisk_context *cxt;
>>>>> +     struct fdisk_context *doscxt = NULL;
>>>>>         struct partition_data *part;
>>>>>         struct partition_data *tmp;
>>>>>         struct fdisk_table *tb = NULL;
>>>>> +     struct fdisk_table *dostb = NULL;
>>>>>         struct fdisk_table *oldtb = NULL;
>>>>> +     struct fdisk_table *olddostb = NULL;
>>>>>         struct fdisk_parttype *parttype = NULL;
>>>>> +     struct fdisk_parttype *dosparttype = NULL;
>>>>>         int ret = 0;
>>>>>         unsigned long i;
>>>>> +     unsigned long hybrid = 0;
>>>>>         struct hnd_priv priv =  {FDISK_DISKLABEL_DOS};
>>>>>         bool createtable = false;
>>>>> +     bool createdostable = false;
>>>>> +
>>>>> +     //fdisk_init_debug(0xffff);
>>>>
>>>> Dead code
>>> Yeah, will remove once finished testing.
>>>>
>>>>>
>>>>>         if (!lbtype || (strcmp(lbtype, "gpt") && strcmp(lbtype, "dos"))) {
>>>>>                 ERROR("Just GPT or DOS partition table are supported");
>>>>> @@ -194,6 +214,8 @@ static int diskpart(struct img_type *img,
>>>>>                 return -ENOMEM;
>>>>>         }
>>>>>
>>>>> +     fdisk_disable_dialogs(cxt, 1);
>>>>> +
>>>>
>>>> Do we need it ? for main context was not necessary.
>>> Well there are some failures this fixes where different code paths are taken
>>> when dialogs are disabled, since we don't support interactive dialogs at all
>>> it's best to ensure they are disabled fully to avoid otherwise hitting an error.
>>>
>>> See here:
>>> https://github.com/karelzak/util-linux/blob/ad0963685b8105920abd38e6878cc2cdfa427ea4/libfdisk/src/dos.c#L1897-L1903
>>> https://github.com/karelzak/util-linux/blob/ba3b93525920f2782e12016e7444fca89fb5bb9c/libfdisk/src/partition.c#L770-L771
>>
>> Just because it appeared to me that the default is disable.
>> fdisk_new_context() zeroes via calloc the whole structure, that means
>> cxt->no_disalogs = disable = 0;
>>
>> But it is ok to explicitley disable it, yes.
>>
>> [1]
>> https://github.com/karelzak/util-linux/blob/ad0963685b8105920abd38e6878cc2cdfa427ea4/libfdisk/src/context.c#L52
>>
>>>>
>>>>>         ret = fdisk_assign_device(cxt, img->device, 0);
>>>>>         if (ret == -EACCES) {
>>>>>                 ERROR("no access to %s", img->device);
>>>>> @@ -247,18 +269,31 @@ static int diskpart(struct img_type *img,
>>>>>                                         case PART_FSTYPE:
>>>>>                                                 strncpy(part->fstype, equal, sizeof(part->fstype));
>>>>>                                                 break;
>>>>> +                                     case PART_DOSTYPE:
>>>>> +                                             strncpy(part->dostype, equal, sizeof(part->dostype));
>>>>> +                                             hybrid++;
>>>>> +                                             break;
>>>>
>>>> Is there some check we should do it on the value ? dostype cannot be any
>>>> string.
>>> I basically just copied this part from how we currently parse dos
>>> entry types, if
>>> we need to add validation we should do it for non-hybrid dos type
>>> parsing as well.
>>
>> Verify the FSTYPE adds a lot of code. Then it is ok, if we will need it,
>> it will be added in future.
>>
>>>>
>>>>>                                         }
>>>>>                                 }
>>>>>                         }
>>>>>                         elem = LIST_NEXT(elem, next);
>>>>>                 }
>>>>>
>>>>> -             TRACE("partition-%zu:%s size %" PRIu64 " start %zu type %s",
>>>>> +             if (hybrid > 3) {
>>>>> +                     ERROR("I cannot add hybrid partition %zu(%s): hybrid dos partition limit of 3 exceeded",
>>>>> +                               part->partno, strlen(part->name) ? part->name : "UNDEF NAME");
>>>>> +                     ret = -EINVAL;
>>>>> +                     goto handler_exit;
>>>>> +             }
>>>>> +
>>>>> +             TRACE("partition-%zu:%s size %" PRIu64 " start %zu type %s%s%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->dostype) ? " dostype " : "",
>>>>> +                     strlen(part->dostype) ? part->dostype : "");
>>>>
>>>> This is what I mind, there is already a long TRACE with all setup.
>>> Ok, so I should keep this and remove the other temporary debugging
>>> TRACE right?
>>
>> Right.
>>
>>>>
>>>>>
>>>>>                 /*
>>>>>                  * Partitions in sw-description start from 1,
>>>>> @@ -287,7 +322,11 @@ static int diskpart(struct img_type *img,
>>>>>         if (!fdisk_has_label(cxt)) {
>>>>>                 WARN("%s does not contain a recognized partition table",
>>>>>                      img->device);
>>>>> -             fdisk_create_disklabel(cxt, lbtype);
>>>>> +             ret = fdisk_create_disklabel(cxt, lbtype);
>>>>> +             if (ret) {
>>>>> +                     ERROR("Failed to create disk label");
>>>>> +                     goto handler_release;
>>>>> +             }
>>>>
>>>> This is a fix - I suggest you post this in a separate patch, together
>>>> with...
>>> Patch sent.
>>
>> Thanks, I'll merge it.
>>
>>>>
>>>>>                 createtable = true;
>>>>>         } else if (lbtype) {
>>>>>                 if (!strcmp(lbtype, "gpt"))
>>>>> @@ -298,12 +337,17 @@ static int diskpart(struct img_type *img,
>>>>>                 if (!fdisk_is_labeltype(cxt, priv.labeltype)) {
>>>>>                         WARN("Partition table of different type, setting to %s, all data lost !",
>>>>>                                 lbtype);
>>>>> -                     fdisk_create_disklabel(cxt, lbtype);
>>>>> +                     ret = fdisk_create_disklabel(cxt, lbtype);
>>>>> +                     if (ret) {
>>>>> +                             ERROR("Failed to create disk label");
>>>>> +                             goto handler_release;
>>>>> +                     }
>>>>
>>>> ...this one.
>>> ...with this as well.
>>>>
>>>>>                         createtable = true;
>>>>>                 }
>>>>>         }
>>>>>
>>>>>         struct fdisk_label *lb = fdisk_get_label(cxt, NULL);
>>>>> +     struct fdisk_label *doslb = NULL;
>>>>>         unsigned long sector_size = fdisk_get_sector_size(cxt);
>>>>>
>>>>>         /*
>>>>> @@ -321,6 +365,32 @@ static int diskpart(struct img_type *img,
>>>>>                 goto handler_exit;
>>>>>         }
>>>>>
>>>>> +     if (hybrid) {
>>>>> +             doscxt = fdisk_new_nested_context(cxt, "dos");
>>>>> +             fdisk_disable_dialogs(doscxt, 1);
>>>> See my question regarding this call.
>>> Yeah, we def need this to prevent failures in the codepaths I mentioned above.
>>>>
>>>>> +
>>>>> +             if (!fdisk_is_labeltype(doscxt, FDISK_DISKLABEL_DOS)) {
>>>>> +                     ret = fdisk_create_disklabel(doscxt, NULL);
>>>>> +                     if (ret) {
>>>>> +                             ERROR("Failed to create hybrid disk label");
>>>>> +                             goto handler_release;
>>>>> +                     }
>>>>> +                     createdostable = true;
>>>>> +             }
>>>>> +
>>>>> +             doslb = fdisk_get_label(doscxt, "dos");
>>>>> +             dostb = fdisk_new_table();
>>>>> +
>>>>> +             if (fdisk_get_partitions(doscxt, &olddostb))
>>>>> +                     createdostable = true;
>>>>> +
>>>>> +             if (!dostb) {
>>>>> +                     ERROR("OOM creating new table !");
>>>>> +                     ret = -ENOMEM;
>>>>> +                     goto handler_exit;
>>>>> +             }
>>>>> +     }
>>>>> +
>>>>>         i = 0;
>>>>>
>>>>>         LIST_FOREACH(part, &priv.listparts, next) {
>>>>> @@ -334,10 +404,30 @@ static int diskpart(struct img_type *img,
>>>>>                         parttype = fdisk_label_get_parttype_from_string(lb, part->type);
>>>>>                         if (!parttype)
>>>>>                                 parttype = fdisk_label_get_parttype_from_string(lb, GPT_DEFAULT_ENTRY_TYPE);
>>>>> +                     if (strlen(part->dostype)) {
>>>>> +                             struct fdisk_partition *newdospa;
>>>>> +                             newdospa = fdisk_new_partition();
>>>>> +                             dosparttype = fdisk_label_get_parttype_from_code(doslb, ustrtoull(part->dostype, 16));
>>>>> +                             if (!dosparttype) {
>>>>> +                                     WARN("Failed to set partition type for hybrid partition: %zu(%s)", part->partno, part->name);
>>>>> +                             }
>>>>> +                             ret = diskpart_set_partition(newdospa, part, sector_size, dosparttype, hybrid);
>>>>> +                             if (ret) {
>>>>> +                                     WARN("I cannot set all hybrid partition's parameters");
>>>>> +                             }
>>>>> +                             if ((ret = fdisk_table_add_partition(dostb, newdospa)) < 0) {
>>>>> +                                     ERROR("I cannot add hybrid partition %zu(%s): %d", part->partno, part->name, ret);
>>>>> +                             }
>>>>> +                             fdisk_unref_partition(newdospa);
>>>>> +                             if (ret < 0) {
>>>>> +                                     fdisk_unref_partition(newpa);
>>>>> +                                     goto handler_exit;
>>>>> +                             }
>>>>> +                     }
>>>>>                 } else {
>>>>>                         parttype = fdisk_label_get_parttype_from_code(lb, ustrtoull(part->type, 16));
>>>>>                 }
>>>>> -             ret = diskpart_set_partition(newpa, part, sector_size, parttype);
>>>>> +             ret = diskpart_set_partition(newpa, part, sector_size, parttype, 0);
>>>>>                 if (ret) {
>>>>>                         WARN("I cannot set all partition's parameters");
>>>>>                 }
>>>>> @@ -350,6 +440,31 @@ static int diskpart(struct img_type *img,
>>>>>                 i++;
>>>>>         }
>>>>>
>>>>> +     if (hybrid) {
>>>>> +             struct fdisk_partition *newpa;
>>>>> +             newpa = fdisk_new_partition();
>>>>> +             part = (struct partition_data *)calloc(1, sizeof(struct partition_data));
>>>>> +             if (!part) {
>>>>> +                     ERROR("FAULT: no memory");
>>>>> +                     ret = -ENOMEM;
>>>>> +                     goto handler_exit;
>>>>> +             }
>>>>> +             part->start = 1;
>>>>> +             part->size = 33 * sector_size;
>>>>> +             dosparttype = fdisk_label_get_parttype_from_code(doslb, 0xee);
>>>>> +             ret = diskpart_set_partition(newpa, part, sector_size, dosparttype, hybrid);
>>>>> +             if (ret) {
>>>>> +                     WARN("I cannot set all pmbr partition's parameters");
>>>>> +             }
>>>>> +             if ((ret = fdisk_table_add_partition(dostb, newpa)) < 0) {
>>>>> +                     ERROR("I cannot add the pmbr partition");
>>>>> +             }
>>>>> +             fdisk_unref_partition(newpa);
>>>>> +             if (ret < 0)
>>>>> +                     goto handler_exit;
>>>>> +             hybrid++;
>>>>> +     }
>>>>> +
>>>>>         /*
>>>>>          * A partiton table was found on disk, now compares the two tables
>>>>>          * to check if they differ.
>>>>> @@ -387,6 +502,69 @@ static int diskpart(struct img_type *img,
>>>>>                 }
>>>>>         }
>>>>>
>>>>> +     /*
>>>>> +      * A dos partiton table was found on disk, now compares the two tables
>>>>> +      * to check if they differ.
>>>>> +      */
>>>>> +     /*
>>>>> +     if (hybrid && !createdostable) {
>>>>> +             size_t numpartondisk = fdisk_table_get_nents(olddostb);
>>>>> +
>>>>> +             i = hybrid;
>>>>> +             if (numpartondisk != i) {
>>>>> +                     TRACE("Number of hybrid partitions differs on disk: %lu <--> requested: %lu",
>>>>> +                               (long unsigned int)numpartondisk, i);
>>>>> +                     createdostable = true;
>>>>> +             } else {
>>>>> +                     struct fdisk_partition *pa, *newpa;
>>>>> +                     struct fdisk_iter *itr   = fdisk_new_iter(FDISK_ITER_FORWARD);
>>>>> +                     struct fdisk_iter *olditr = fdisk_new_iter(FDISK_ITER_FORWARD);
>>>>> +
>>>>> +                     i = 0;
>>>>> +                     while (i < numpartondisk && !createdostable) {
>>>>> +                             newpa=NULL;
>>>>> +                             pa = NULL;
>>>>> +                             if (fdisk_table_next_partition (dostb, itr, &newpa) ||
>>>>> +                                     fdisk_table_next_partition (olddostb, olditr, &pa)) {
>>>>> +                                     TRACE("Hybrid partition not found, something went wrong %lu !", i);
>>>>> +                                     ret = -EFAULT;
>>>>> +                                     goto handler_exit;
>>>>> +                             }
>>>>> +                             if (diskpart_partition_cmp("dos", pa, newpa)) {
>>>>> +                                     createdostable = true;
>>>>> +                             }
>>>>> +
>>>>> +                             fdisk_unref_partition(newpa);
>>>>> +                             fdisk_unref_partition(pa);
>>>>> +                             i++;
>>>>> +                     }
>>>>> +             }
>>>>> +     }
>>>>> +     */
>>>>
>>>> Ok, so klet the code in when it works. As first step, you can force that
>>>> the table must be updated if hybrid is set.
>>> I think I might try and clean up diskpart_partition_cmp first, the current
>>> implementation is a little annoying to debug as it doesn't really log all the
>>> differences between partitions properly.
>>
>> Ok, fine.
> Ok, so I was able to determine that the diskpart_partition_cmp bug I was
> running into is not limited to hybrid partitions and is an existing bug.
> 
> The bug seems to be that diskpart_partition_cmp does not properly handle
> automatically sized partitions, here's a reproducer:
> software =
> {
>          version = "0.1.0";
> 
>          partitions: (
>          {
>                  type = "diskpart";
>                  device = "/dev/sdb";
>                  properties: {
>                          labeltype = "gpt";
>                          partition-1 = [
>                                  "size=550M",
>                                  "start=2048",
>                                  "name=boot",
>                                  "type=C12A7328-F81F-11D2-BA4B-00A0C93EC93B",
>                                  "fstype=vfat"
>                          ];
>                          partition-2 = [
>                                  "size=1G",
>                                  "name=main",
>                                  "type=4F68BCE3-E8CD-4DB1-96E7-FBCAF984B709",
>                                  "fstype=ext4"
>                          ];
>                          partition-3 = [
>                                  "size=1G",
>                                  "name=alt",
>                                  "type=4F68BCE3-E8CD-4DB1-96E7-FBCAF984B709",
>                                  "fstype=ext4"
>                          ];
>                          partition-4 = [
>                                  "name=srv",
>                                  "type=3B8F8425-20E0-4F3B-907F-1A25A76F98E8",
>                                  "fstype=ext4"
>                          ];
>                  }
>          });
> }

You are surely right, I can just say that this was not a use case of 
mine, I always set the size of partitions (and in case of different hw / 
different disk size, I set a hook to recalculate the partitions' size). 
This is an untested use case on my site.

> partition-4 is a bulk data partition that is automatically sized to
> fill the remaining
> disk space, this config works fine during the initial format but the comparison
> function doesn't seem to appropriately handle automatically sized partitions
> for some reason on subsequent runs.

Got it.

Best regards,
Stefano Babic

>>
>>>>
>>>>> +     if (hybrid) {
>>>>> +             // TODO fix dos table comparison
>>>>> +             createdostable = true;
>>>>> +     }
>>>>> +
>>>>> +     if (createdostable) {
>>>>> +             TRACE("Hybrid partitions on disk differ, write to disk;");
>>>>> +             fdisk_delete_all_partitions(doscxt);
>>>>> +             ret = fdisk_apply_table(doscxt, dostb);
>>>>> +             if (ret) {
>>>>> +                     ERROR("Hybrid partition table cannot be applied! %d", ret);
>>>>> +                     goto handler_exit;
>>>>> +             }
>>>>> +
>>>>> +             /*
>>>>> +              * Everything done, write into disk
>>>>> +              */
>>>>> +             ret = fdisk_write_disklabel(doscxt);
>>>>> +             if (ret) {
>>>>> +                     ERROR("Hybrid partition table cannot be written on disk %d", ret);
>>>>> +                     goto handler_exit;
>>>>> +             }
>>>>> +     }
>>>>> +
>>>>>         if (createtable) {
>>>>>                 TRACE("Partitions on disk differ, write to disk;");
>>>>>                 fdisk_delete_all_partitions(cxt);
>>>>> @@ -412,12 +590,16 @@ static int diskpart(struct img_type *img,
>>>>>     handler_exit:
>>>>>         if (tb)
>>>>>                 fdisk_unref_table(tb);
>>>>> +     if (dostb)
>>>>> +             fdisk_unref_table(dostb);
>>>>>         if (oldtb)
>>>>>                 fdisk_unref_table(oldtb);
>>>>>         if (fdisk_deassign_device(cxt, 0))
>>>>>                 WARN("Error deassign device %s", img->device);
>>>>>
>>>>>     handler_release:
>>>>> +     if (fdisk_get_parent(doscxt))
>>>>> +             fdisk_unref_context(doscxt);
>>>>>         fdisk_unref_context(cxt);
>>>>>
>>>>>         /*
>>>>>
>>>>
>>>> As far as I can see, this does not change the current behavior. Yes, it
>>>> must be well tested (as any patch should be..)
>>> Yeah, that's what I was going for.
>>
>> Best regards,
>> Stefano Babic
>>
>>
>> --
>> =====================================================================
>> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
>> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
>> Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de
>> =====================================================================
>