| Message ID | 20230322155430.31594-1-dit.kozmaj@kynetics.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] wic: use part_name when defined | expand |
Hi, We tested a few combinations of --part-name and --label parameters when creating a WIC image. When only the --part-name is defined, the GPT partition label is correctly set, and filesystem label is empty. If both are defined, or only --label is defined, both the filesystem label and the GPT partition label are set to the value of --label. As it makes sense to have a GPT partition label which is different from the contained filesystem label, our proposal is to fix the case when both are set, and keep the current behavior when only --label is set for compatibility reasons. If this proposal is ok, we will patch the documentation to document that the GPT partition label is set even when only --label is defined. Regards On Wed, Mar 22, 2023 at 4:54 PM <dit.kozmaj@kynetics.com> wrote: > From: Dit Kozmaj <dit.kozmaj@kynetics.com> > > So far part.label has been used to define GPT partition label even if > part.part_name was defined. > Fix the code to use part.part_name whenever available, as it makes sense > to have a GPT partition label which is different from the contained > filesystem label. > > Signed-off-by: Dit Kozmaj <dit.kozmaj@kynetics.com> > Signed-off-by: Diego Rondini <diego.rondini@kynetics.com> > --- > scripts/lib/wic/plugins/imager/direct.py | 14 ++++---------- > 1 file changed, 4 insertions(+), 10 deletions(-) > > diff --git a/scripts/lib/wic/plugins/imager/direct.py > b/scripts/lib/wic/plugins/imager/direct.py > index dfaa901567..1f06b89f8e 100644 > --- a/scripts/lib/wic/plugins/imager/direct.py > +++ b/scripts/lib/wic/plugins/imager/direct.py > @@ -571,11 +571,12 @@ class PartitionedImage(): > self._create_partition(self.path, part.type, > parted_fs_type, part.start, > part.size_sec) > > - if part.part_name: > + if part.part_name or part.label: > + partition_label = part.part_name if part.part_name else > part.label > logger.debug("partition %d: set name to %s", > - part.num, part.part_name) > + part.num, partition_label) > exec_native_cmd("sgdisk --change-name=%d:%s %s" % \ > - (part.num, part.part_name, > + (part.num, partition_label, > self.path), self.native_sysroot) > > if part.part_type: > @@ -592,13 +593,6 @@ class PartitionedImage(): > (part.num, part.uuid, self.path), > self.native_sysroot) > > - if part.label and self.ptable_format == "gpt": > - logger.debug("partition %d: set name to %s", > - part.num, part.label) > - exec_native_cmd("parted -s %s name %d %s" % \ > - (self.path, part.num, part.label), > - self.native_sysroot) > - > if part.active: > flag_name = "legacy_boot" if self.ptable_format == 'gpt' > else "boot" > logger.debug("Set '%s' flag for partition '%s' on disk > '%s'", > -- > 2.34.1 > >
On Wed, 2023-03-22 at 16:54 +0100, dit.kozmaj@kynetics.com wrote: > From: Dit Kozmaj <dit.kozmaj@kynetics.com> > > So far part.label has been used to define GPT partition label even if > part.part_name was defined. > Fix the code to use part.part_name whenever available, as it makes sense > to have a GPT partition label which is different from the contained > filesystem label. > > Signed-off-by: Dit Kozmaj <dit.kozmaj@kynetics.com> > Signed-off-by: Diego Rondini <diego.rondini@kynetics.com> > --- > scripts/lib/wic/plugins/imager/direct.py | 14 ++++---------- > 1 file changed, 4 insertions(+), 10 deletions(-) > > diff --git a/scripts/lib/wic/plugins/imager/direct.py b/scripts/lib/wic/plugins/imager/direct.py > index dfaa901567..1f06b89f8e 100644 > --- a/scripts/lib/wic/plugins/imager/direct.py > +++ b/scripts/lib/wic/plugins/imager/direct.py > @@ -571,11 +571,12 @@ class PartitionedImage(): > self._create_partition(self.path, part.type, > parted_fs_type, part.start, part.size_sec) > > - if part.part_name: > + if part.part_name or part.label: > + partition_label = part.part_name if part.part_name else part.label > logger.debug("partition %d: set name to %s", > - part.num, part.part_name) > + part.num, partition_label) > exec_native_cmd("sgdisk --change-name=%d:%s %s" % \ > - (part.num, part.part_name, > + (part.num, partition_label, > self.path), self.native_sysroot) > > if part.part_type: > @@ -592,13 +593,6 @@ class PartitionedImage(): > (part.num, part.uuid, self.path), > self.native_sysroot) > > - if part.label and self.ptable_format == "gpt": > - logger.debug("partition %d: set name to %s", > - part.num, part.label) > - exec_native_cmd("parted -s %s name %d %s" % \ > - (self.path, part.num, part.label), > - self.native_sysroot) > - > if part.active: > flag_name = "legacy_boot" if self.ptable_format == 'gpt' else "boot" > logger.debug("Set '%s' flag for partition '%s' on disk '%s'", This was flagged on irc as not having a response. We're struggling with a lack of a maintainer for wic at the moment so review is slow and I'm not really a wic expert but I'm all we have :/. As mentioned, changes to code like this are much more likely if there is a test case included. The existing tests can be run with "oe- selftest -r wic". Can you confirm this patch doesn't regress the current tests? Would you be able to add a test so this change doesn't regress in future? Cheers, Richard
Hi, On Wed, Mar 29, 2023 at 5:55 PM Richard Purdie < richard.purdie@linuxfoundation.org> wrote: > On Wed, 2023-03-22 at 16:54 +0100, dit.kozmaj@kynetics.com wrote: > > From: Dit Kozmaj <dit.kozmaj@kynetics.com> > > > > So far part.label has been used to define GPT partition label even if > > part.part_name was defined. > > Fix the code to use part.part_name whenever available, as it makes sense > > to have a GPT partition label which is different from the contained > > filesystem label. > > > > Signed-off-by: Dit Kozmaj <dit.kozmaj@kynetics.com> > > Signed-off-by: Diego Rondini <diego.rondini@kynetics.com> > > --- > > scripts/lib/wic/plugins/imager/direct.py | 14 ++++---------- > > 1 file changed, 4 insertions(+), 10 deletions(-) > > > > diff --git a/scripts/lib/wic/plugins/imager/direct.py > b/scripts/lib/wic/plugins/imager/direct.py > > index dfaa901567..1f06b89f8e 100644 > > --- a/scripts/lib/wic/plugins/imager/direct.py > > +++ b/scripts/lib/wic/plugins/imager/direct.py > > @@ -571,11 +571,12 @@ class PartitionedImage(): > > self._create_partition(self.path, part.type, > > parted_fs_type, part.start, > part.size_sec) > > > > - if part.part_name: > > + if part.part_name or part.label: > > + partition_label = part.part_name if part.part_name else > part.label > > logger.debug("partition %d: set name to %s", > > - part.num, part.part_name) > > + part.num, partition_label) > > exec_native_cmd("sgdisk --change-name=%d:%s %s" % \ > > - (part.num, part.part_name, > > + (part.num, partition_label, > > self.path), > self.native_sysroot) > > > > if part.part_type: > > @@ -592,13 +593,6 @@ class PartitionedImage(): > > (part.num, part.uuid, self.path), > > self.native_sysroot) > > > > - if part.label and self.ptable_format == "gpt": > > - logger.debug("partition %d: set name to %s", > > - part.num, part.label) > > - exec_native_cmd("parted -s %s name %d %s" % \ > > - (self.path, part.num, part.label), > > - self.native_sysroot) > > - > > if part.active: > > flag_name = "legacy_boot" if self.ptable_format == > 'gpt' else "boot" > > logger.debug("Set '%s' flag for partition '%s' on disk > '%s'", > > This was flagged on irc as not having a response. We're struggling with > a lack of a maintainer for wic at the moment so review is slow and I'm > not really a wic expert but I'm all we have :/. > > As mentioned, changes to code like this are much more likely if there > is a test case included. The existing tests can be run with "oe- > selftest -r wic". > > Can you confirm this patch doesn't regress the current tests? > We've sent a new patchset that doesn't regress the current tests: https://patchwork.yoctoproject.org/project/oe-core/list/?series=11822 > Would you be able to add a test so this change doesn't regress in > future? > The aforementioned patchset includes a new test for that purpose and updates the documentation to reflect the changes and explain what happens in each case. This RFC is superseded by the patchset. Cheers, > > Richard > > Cheers, Diego Rondini Sr. Embedded Engineer Kynetics www.kynetics.com
diff --git a/scripts/lib/wic/plugins/imager/direct.py b/scripts/lib/wic/plugins/imager/direct.py index dfaa901567..1f06b89f8e 100644 --- a/scripts/lib/wic/plugins/imager/direct.py +++ b/scripts/lib/wic/plugins/imager/direct.py @@ -571,11 +571,12 @@ class PartitionedImage(): self._create_partition(self.path, part.type, parted_fs_type, part.start, part.size_sec) - if part.part_name: + if part.part_name or part.label: + partition_label = part.part_name if part.part_name else part.label logger.debug("partition %d: set name to %s", - part.num, part.part_name) + part.num, partition_label) exec_native_cmd("sgdisk --change-name=%d:%s %s" % \ - (part.num, part.part_name, + (part.num, partition_label, self.path), self.native_sysroot) if part.part_type: @@ -592,13 +593,6 @@ class PartitionedImage(): (part.num, part.uuid, self.path), self.native_sysroot) - if part.label and self.ptable_format == "gpt": - logger.debug("partition %d: set name to %s", - part.num, part.label) - exec_native_cmd("parted -s %s name %d %s" % \ - (self.path, part.num, part.label), - self.native_sysroot) - if part.active: flag_name = "legacy_boot" if self.ptable_format == 'gpt' else "boot" logger.debug("Set '%s' flag for partition '%s' on disk '%s'",