[parted-devel] [PATCH v2 0/1] direct handling of partition type-id and -uuid

[Brian C. Lane]

On Wed, Apr 27, 2022 at 12:54:01PM +0000, Arvin Schnell wrote:
> 
> Hi,
> 
> here is an updated version of the patch to display and allow to
> set the partition type id/uuid for dos and gpt. It add a few
> fixes, documentation and two testcases. It does not address the
> problem already mentioned that setting the type-id does not set
> the flags.

Thanks for doing this, sorry it took me so long to take a look at it.
Overall I like it, but have some comments and requests (this covers both
patches):

* A single 'type' command would be nicer than making the user select the
  right one, parted should be able to figure out which function to call
  based on the disklabel.

* Document the uuid format with examples, and a pointer to a reference (wikipedia?)
  Not super-excited about pointing to the wikipedia page for gpt, but
  that may be the most stable and maintained reference we have.

* Document the id format with examples, and a pointer to a reference (wikipedia?)
  Same comment as above :)

* There should be tests for the new 'type' command, both for success and
  for failure with invalid values on all supported disklabels.

* Add PED_ASSERT to check pointers being passed into functions like dos_type_id_set_hidden
  In order to protect against other callers accidentally passing in NULL
  it looks like most of the functions have ASSERT on the pointer when
  they don't directly check for !ptr in the code.

* Consistent use of dos_data instead of dos_part_data in *_set_type_id and *_get_type_id
  There were a couple places where it was different. It should be
  consistent, in the dos disklabel code, so grep can find them all :)

Questions
 * Should this be added to the machine output?
   I think so. Now is probably the best time to break this :)
   It can also wait and be done in a later patch, so shouldn't block
   the other changes.

 * Should a column be added to plain output? Maybe with a --verbose option?
   I'm unsure about this. But it feels weird not to be able to show it
   without using --json output.
   And as with machine output, it can wait and be done later, if at all.


And finally, probably the biggest issue, is the change in behavior for
msdos_partition_set_system. As you mentioned in your cover email for
that patch, it no longer has flags to check, and it used to have a bunch
of logic that would keep it from clearing things like LVM or DIAG if the
filesystem was set after setting a flag.

I like the new behavior better, but it is a potentially surprising
change to API users so if we do this we need to bump the API version and
clearly document the change.

I did a github search for code that looked like it was calling
ped_partition_set_system and found a few (including gparted) but they
were calling it after clearing the lvm flag. I don't think we can know
what strange corners the change might break, but it looks like it won't
break popular libparted users at least :)

Comments and discussion welcome from others as well.

Thanks,

Brian

-- 
Brian C. Lane (PST8PDT) - weldr.io - lorax - parted - pykickstart