diff mbox series

[v3] wic/plugins: gate root= with creator.rootdev for efi-isar and pcbios-isar

Message ID 20260409093358.3438728-1-gouravsingh@siemens.com
State Under Review
Headers show
Series [v3] wic/plugins: gate root= with creator.rootdev for efi-isar and pcbios-isar | expand

Commit Message

Singh, Gourav April 9, 2026, 9:33 a.m. UTC 
Checks for creator.rootdev not being None were missing and would cause
the kernel command line to read "root=None". When using the Discoverable
Partitions Specification, we really want no root= parameter on the
kernel command line (and root=None is anyhow not a valid option).

Signed-off-by: Cedric Hombourger <cedric.hombourger@siemens.com>
Signed-off-by: Gourav Singh <gouravsingh@siemens.com>
---
 .../lib/wic/plugins/source/bootimg-efi-isar.py  |  7 ++++---
 .../wic/plugins/source/bootimg-pcbios-isar.py   | 17 ++++++++++++-----
 2 files changed, 16 insertions(+), 8 deletions(-)

--
2.39.5

Comments

Jan Kiszka April 9, 2026, 10:05 a.m. UTC | #1 
On 09.04.26 11:33, 'Gourav Singh' via isar-users wrote:
> Checks for creator.rootdev not being None were missing and would cause
> the kernel command line to read "root=None". When using the Discoverable
> Partitions Specification, we really want no root= parameter on the
> kernel command line (and root=None is anyhow not a valid option).

Is this a backport from upstream wic? Or are we touching code that does
not exist in the original version, is Isar-specific?

> 
> Signed-off-by: Cedric Hombourger <cedric.hombourger@siemens.com>
> Signed-off-by: Gourav Singh <gouravsingh@siemens.com>
> ---
>  .../lib/wic/plugins/source/bootimg-efi-isar.py  |  7 ++++---
>  .../wic/plugins/source/bootimg-pcbios-isar.py   | 17 ++++++++++++-----
>  2 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/meta/scripts/lib/wic/plugins/source/bootimg-efi-isar.py b/meta/scripts/lib/wic/plugins/source/bootimg-efi-isar.py
> index 6bc78d42..6c6698d6 100644
> --- a/meta/scripts/lib/wic/plugins/source/bootimg-efi-isar.py
> +++ b/meta/scripts/lib/wic/plugins/source/bootimg-efi-isar.py
> @@ -104,7 +104,7 @@ class BootimgEFIPlugin(SourcePlugin):
>                          (get_bitbake_var("KERNEL_IMAGETYPE"), get_bitbake_var("INITRAMFS_LINK_NAME"))
> 
>              label = source_params.get('label')
> -            label_conf = "root=%s" % creator.rootdev
> +            label_conf = f" root={creator.rootdev}" if creator.rootdev else ""
                             ^^^
Extra space, not needed.

>              if label:
>                  label_conf = "LABEL=%s" % label
> 
> @@ -201,7 +201,8 @@ class BootimgEFIPlugin(SourcePlugin):
>              boot_conf += "linux /%s\n" % kernel
> 
>              label = source_params.get('label')
> -            label_conf = "LABEL=Boot root=%s" % creator.rootdev
> +            label_conf = "LABEL=Boot"
> +            label_conf += f" root={creator.rootdev}" if creator.rootdev else ""
>              if label:
>                  label_conf = "LABEL=%s" % label
> 
> @@ -366,7 +367,7 @@ class BootimgEFIPlugin(SourcePlugin):
> 
>              with tempfile.TemporaryDirectory() as tmp_dir:
>                  label = source_params.get('label')
> -                label_conf = "root=%s" % creator.rootdev
> +                label_conf = f" root={creator.rootdev}" if creator.rootdev else ""

Also here: leading whitespace is not needed.

>                  if label:
>                      label_conf = "LABEL=%s" % label
> 
> diff --git a/meta/scripts/lib/wic/plugins/source/bootimg-pcbios-isar.py b/meta/scripts/lib/wic/plugins/source/bootimg-pcbios-isar.py
> index d5040b72..5dd76761 100644
> --- a/meta/scripts/lib/wic/plugins/source/bootimg-pcbios-isar.py
> +++ b/meta/scripts/lib/wic/plugins/source/bootimg-pcbios-isar.py
> @@ -140,14 +140,21 @@ class BootimgPcbiosIsarPlugin(SourcePlugin):
> 
>              syslinux_conf += "KERNEL " + kernel + "\n"
> 
> -            syslinux_conf += "APPEND label=boot root=%s %s\n" % \
> -                             (creator.rootdev, bootloader.append)
> +            # Check if rootdev exists
> +            root_param = f"root={creator.rootdev}" if creator.rootdev else ""
> +
> +            syslinux_conf += f"APPEND label=boot {root_param} {bootloader.append}\n"
> 
>              # if we are using an initrd, smuggle it in
>              if initrd:
> -                syslinux_conf = syslinux_conf.replace(
> -                    " root=%s " % (creator.rootdev),
> -                    " root=%s initrd=%s " % (creator.rootdev, initrd))
> +                if root_param:
> +                    syslinux_conf = syslinux_conf.replace(
> +                        root_param,
> +                        f" {root_param} initrd={initrd} ")

And more extra whitespaces, leading and trainling.

> +                else:
> +                    syslinux_conf = syslinux_conf.replace(
> +                        " label=boot ",
> +                        f" label=boot initrd={initrd} ")
> 
>          logger.debug("Writing syslinux config %s/hdd/boot/syslinux.cfg",
>                       cr_workdir)
> --
> 2.39.5
> 

Jan
Singh, Gourav April 9, 2026, 10:24 a.m. UTC | #2 
Hi Jan,

Thanks for the review. These changes are currently specific to ISAR. A patch will be submitted to upstream OE to incorporate these changes there as well.

Thanks,
Gourav
-----Original Message-----
From: Kiszka, Jan (FT RPD CED) <jan.kiszka@siemens.com> 
Sent: 09 April 2026 15:36
To: Singh, Gourav (FT FDS CES LX PBU 2) <gouravsingh@siemens.com>; isar-users@googlegroups.com
Cc: Hombourger, Cedric (FT FDS CES LX) <cedric.hombourger@siemens.com>
Subject: Re: [PATCH v3] wic/plugins: gate root= with creator.rootdev for efi-isar and pcbios-isar

On 09.04.26 11:33, 'Gourav Singh' via isar-users wrote:
> Checks for creator.rootdev not being None were missing and would cause 
> the kernel command line to read "root=None". When using the 
> Discoverable Partitions Specification, we really want no root= 
> parameter on the kernel command line (and root=None is anyhow not a valid option).

Is this a backport from upstream wic? Or are we touching code that does not exist in the original version, is Isar-specific?

> 
> Signed-off-by: Cedric Hombourger <cedric.hombourger@siemens.com>
> Signed-off-by: Gourav Singh <gouravsingh@siemens.com>
> ---
>  .../lib/wic/plugins/source/bootimg-efi-isar.py  |  7 ++++---
>  .../wic/plugins/source/bootimg-pcbios-isar.py   | 17 ++++++++++++-----
>  2 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/meta/scripts/lib/wic/plugins/source/bootimg-efi-isar.py 
> b/meta/scripts/lib/wic/plugins/source/bootimg-efi-isar.py
> index 6bc78d42..6c6698d6 100644
> --- a/meta/scripts/lib/wic/plugins/source/bootimg-efi-isar.py
> +++ b/meta/scripts/lib/wic/plugins/source/bootimg-efi-isar.py
> @@ -104,7 +104,7 @@ class BootimgEFIPlugin(SourcePlugin):
>                          (get_bitbake_var("KERNEL_IMAGETYPE"), 
> get_bitbake_var("INITRAMFS_LINK_NAME"))
> 
>              label = source_params.get('label')
> -            label_conf = "root=%s" % creator.rootdev
> +            label_conf = f" root={creator.rootdev}" if creator.rootdev else ""
                             ^^^
Extra space, not needed.

>              if label:
>                  label_conf = "LABEL=%s" % label
> 
> @@ -201,7 +201,8 @@ class BootimgEFIPlugin(SourcePlugin):
>              boot_conf += "linux /%s\n" % kernel
> 
>              label = source_params.get('label')
> -            label_conf = "LABEL=Boot root=%s" % creator.rootdev
> +            label_conf = "LABEL=Boot"
> +            label_conf += f" root={creator.rootdev}" if creator.rootdev else ""
>              if label:
>                  label_conf = "LABEL=%s" % label
> 
> @@ -366,7 +367,7 @@ class BootimgEFIPlugin(SourcePlugin):
> 
>              with tempfile.TemporaryDirectory() as tmp_dir:
>                  label = source_params.get('label')
> -                label_conf = "root=%s" % creator.rootdev
> +                label_conf = f" root={creator.rootdev}" if creator.rootdev else ""

Also here: leading whitespace is not needed.

>                  if label:
>                      label_conf = "LABEL=%s" % label
> 
> diff --git 
> a/meta/scripts/lib/wic/plugins/source/bootimg-pcbios-isar.py 
> b/meta/scripts/lib/wic/plugins/source/bootimg-pcbios-isar.py
> index d5040b72..5dd76761 100644
> --- a/meta/scripts/lib/wic/plugins/source/bootimg-pcbios-isar.py
> +++ b/meta/scripts/lib/wic/plugins/source/bootimg-pcbios-isar.py
> @@ -140,14 +140,21 @@ class BootimgPcbiosIsarPlugin(SourcePlugin):
> 
>              syslinux_conf += "KERNEL " + kernel + "\n"
> 
> -            syslinux_conf += "APPEND label=boot root=%s %s\n" % \
> -                             (creator.rootdev, bootloader.append)
> +            # Check if rootdev exists
> +            root_param = f"root={creator.rootdev}" if creator.rootdev else ""
> +
> +            syslinux_conf += f"APPEND label=boot {root_param} {bootloader.append}\n"
> 
>              # if we are using an initrd, smuggle it in
>              if initrd:
> -                syslinux_conf = syslinux_conf.replace(
> -                    " root=%s " % (creator.rootdev),
> -                    " root=%s initrd=%s " % (creator.rootdev, initrd))
> +                if root_param:
> +                    syslinux_conf = syslinux_conf.replace(
> +                        root_param,
> +                        f" {root_param} initrd={initrd} ")

And more extra whitespaces, leading and trainling.

> +                else:
> +                    syslinux_conf = syslinux_conf.replace(
> +                        " label=boot ",
> +                        f" label=boot initrd={initrd} ")
> 
>          logger.debug("Writing syslinux config %s/hdd/boot/syslinux.cfg",
>                       cr_workdir)
> --
> 2.39.5
> 

Jan

--
Siemens AG, Foundational Technologies
Linux Expert Center
Jan Kiszka April 9, 2026, 10:32 a.m. UTC | #3 
On 09.04.26 12:24, Singh, Gourav (FT FDS CES LX PBU 2) wrote:
> Hi Jan,
> 
> Thanks for the review. These changes are currently specific to ISAR. A patch will be submitted to upstream OE to incorporate these changes there as well.
> 

Then let's wait for upstream first.

My whitespace comments likely apply to the upstream version as well, so
have a look at them in that context, please.

Jan

> Thanks,
> Gourav
> -----Original Message-----
> From: Kiszka, Jan (FT RPD CED) <jan.kiszka@siemens.com> 
> Sent: 09 April 2026 15:36
> To: Singh, Gourav (FT FDS CES LX PBU 2) <gouravsingh@siemens.com>; isar-users@googlegroups.com
> Cc: Hombourger, Cedric (FT FDS CES LX) <cedric.hombourger@siemens.com>
> Subject: Re: [PATCH v3] wic/plugins: gate root= with creator.rootdev for efi-isar and pcbios-isar
> 
> On 09.04.26 11:33, 'Gourav Singh' via isar-users wrote:
>> Checks for creator.rootdev not being None were missing and would cause 
>> the kernel command line to read "root=None". When using the 
>> Discoverable Partitions Specification, we really want no root= 
>> parameter on the kernel command line (and root=None is anyhow not a valid option).
> 
> Is this a backport from upstream wic? Or are we touching code that does not exist in the original version, is Isar-specific?
> 
>>
>> Signed-off-by: Cedric Hombourger <cedric.hombourger@siemens.com>
>> Signed-off-by: Gourav Singh <gouravsingh@siemens.com>
>> ---
>>  .../lib/wic/plugins/source/bootimg-efi-isar.py  |  7 ++++---
>>  .../wic/plugins/source/bootimg-pcbios-isar.py   | 17 ++++++++++++-----
>>  2 files changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/meta/scripts/lib/wic/plugins/source/bootimg-efi-isar.py 
>> b/meta/scripts/lib/wic/plugins/source/bootimg-efi-isar.py
>> index 6bc78d42..6c6698d6 100644
>> --- a/meta/scripts/lib/wic/plugins/source/bootimg-efi-isar.py
>> +++ b/meta/scripts/lib/wic/plugins/source/bootimg-efi-isar.py
>> @@ -104,7 +104,7 @@ class BootimgEFIPlugin(SourcePlugin):
>>                          (get_bitbake_var("KERNEL_IMAGETYPE"), 
>> get_bitbake_var("INITRAMFS_LINK_NAME"))
>>
>>              label = source_params.get('label')
>> -            label_conf = "root=%s" % creator.rootdev
>> +            label_conf = f" root={creator.rootdev}" if creator.rootdev else ""
>                              ^^^
> Extra space, not needed.
> 
>>              if label:
>>                  label_conf = "LABEL=%s" % label
>>
>> @@ -201,7 +201,8 @@ class BootimgEFIPlugin(SourcePlugin):
>>              boot_conf += "linux /%s\n" % kernel
>>
>>              label = source_params.get('label')
>> -            label_conf = "LABEL=Boot root=%s" % creator.rootdev
>> +            label_conf = "LABEL=Boot"
>> +            label_conf += f" root={creator.rootdev}" if creator.rootdev else ""
>>              if label:
>>                  label_conf = "LABEL=%s" % label
>>
>> @@ -366,7 +367,7 @@ class BootimgEFIPlugin(SourcePlugin):
>>
>>              with tempfile.TemporaryDirectory() as tmp_dir:
>>                  label = source_params.get('label')
>> -                label_conf = "root=%s" % creator.rootdev
>> +                label_conf = f" root={creator.rootdev}" if creator.rootdev else ""
> 
> Also here: leading whitespace is not needed.
> 
>>                  if label:
>>                      label_conf = "LABEL=%s" % label
>>
>> diff --git 
>> a/meta/scripts/lib/wic/plugins/source/bootimg-pcbios-isar.py 
>> b/meta/scripts/lib/wic/plugins/source/bootimg-pcbios-isar.py
>> index d5040b72..5dd76761 100644
>> --- a/meta/scripts/lib/wic/plugins/source/bootimg-pcbios-isar.py
>> +++ b/meta/scripts/lib/wic/plugins/source/bootimg-pcbios-isar.py
>> @@ -140,14 +140,21 @@ class BootimgPcbiosIsarPlugin(SourcePlugin):
>>
>>              syslinux_conf += "KERNEL " + kernel + "\n"
>>
>> -            syslinux_conf += "APPEND label=boot root=%s %s\n" % \
>> -                             (creator.rootdev, bootloader.append)
>> +            # Check if rootdev exists
>> +            root_param = f"root={creator.rootdev}" if creator.rootdev else ""
>> +
>> +            syslinux_conf += f"APPEND label=boot {root_param} {bootloader.append}\n"
>>
>>              # if we are using an initrd, smuggle it in
>>              if initrd:
>> -                syslinux_conf = syslinux_conf.replace(
>> -                    " root=%s " % (creator.rootdev),
>> -                    " root=%s initrd=%s " % (creator.rootdev, initrd))
>> +                if root_param:
>> +                    syslinux_conf = syslinux_conf.replace(
>> +                        root_param,
>> +                        f" {root_param} initrd={initrd} ")
> 
> And more extra whitespaces, leading and trainling.
> 
>> +                else:
>> +                    syslinux_conf = syslinux_conf.replace(
>> +                        " label=boot ",
>> +                        f" label=boot initrd={initrd} ")
>>
>>          logger.debug("Writing syslinux config %s/hdd/boot/syslinux.cfg",
>>                       cr_workdir)
>> --
>> 2.39.5
>>
> 
> Jan
> 
> --
> Siemens AG, Foundational Technologies
> Linux Expert Center
diff mbox series

Patch

diff --git a/meta/scripts/lib/wic/plugins/source/bootimg-efi-isar.py b/meta/scripts/lib/wic/plugins/source/bootimg-efi-isar.py
index 6bc78d42..6c6698d6 100644
--- a/meta/scripts/lib/wic/plugins/source/bootimg-efi-isar.py
+++ b/meta/scripts/lib/wic/plugins/source/bootimg-efi-isar.py
@@ -104,7 +104,7 @@  class BootimgEFIPlugin(SourcePlugin):
                         (get_bitbake_var("KERNEL_IMAGETYPE"), get_bitbake_var("INITRAMFS_LINK_NAME"))

             label = source_params.get('label')
-            label_conf = "root=%s" % creator.rootdev
+            label_conf = f" root={creator.rootdev}" if creator.rootdev else ""
             if label:
                 label_conf = "LABEL=%s" % label

@@ -201,7 +201,8 @@  class BootimgEFIPlugin(SourcePlugin):
             boot_conf += "linux /%s\n" % kernel

             label = source_params.get('label')
-            label_conf = "LABEL=Boot root=%s" % creator.rootdev
+            label_conf = "LABEL=Boot"
+            label_conf += f" root={creator.rootdev}" if creator.rootdev else ""
             if label:
                 label_conf = "LABEL=%s" % label

@@ -366,7 +367,7 @@  class BootimgEFIPlugin(SourcePlugin):

             with tempfile.TemporaryDirectory() as tmp_dir:
                 label = source_params.get('label')
-                label_conf = "root=%s" % creator.rootdev
+                label_conf = f" root={creator.rootdev}" if creator.rootdev else ""
                 if label:
                     label_conf = "LABEL=%s" % label

diff --git a/meta/scripts/lib/wic/plugins/source/bootimg-pcbios-isar.py b/meta/scripts/lib/wic/plugins/source/bootimg-pcbios-isar.py
index d5040b72..5dd76761 100644
--- a/meta/scripts/lib/wic/plugins/source/bootimg-pcbios-isar.py
+++ b/meta/scripts/lib/wic/plugins/source/bootimg-pcbios-isar.py
@@ -140,14 +140,21 @@  class BootimgPcbiosIsarPlugin(SourcePlugin):

             syslinux_conf += "KERNEL " + kernel + "\n"

-            syslinux_conf += "APPEND label=boot root=%s %s\n" % \
-                             (creator.rootdev, bootloader.append)
+            # Check if rootdev exists
+            root_param = f"root={creator.rootdev}" if creator.rootdev else ""
+
+            syslinux_conf += f"APPEND label=boot {root_param} {bootloader.append}\n"

             # if we are using an initrd, smuggle it in
             if initrd:
-                syslinux_conf = syslinux_conf.replace(
-                    " root=%s " % (creator.rootdev),
-                    " root=%s initrd=%s " % (creator.rootdev, initrd))
+                if root_param:
+                    syslinux_conf = syslinux_conf.replace(
+                        root_param,
+                        f" {root_param} initrd={initrd} ")
+                else:
+                    syslinux_conf = syslinux_conf.replace(
+                        " label=boot ",
+                        f" label=boot initrd={initrd} ")

         logger.debug("Writing syslinux config %s/hdd/boot/syslinux.cfg",
                      cr_workdir)