| Message ID | 20230206191615.2675373-1-kareem.zarka@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | wic/plugins/source/bootimg-efi: Skip installing kernel-image into boot. | expand |
Hello Kareem, thanks for your patch. I have a few suggestions to improve it, see below. On Mon, 6 Feb 2023 20:16:14 +0100 "Kareem Zarka" <zarkakareem@gmail.com> wrote: > The issue with installing the kernel-image to both rootfs > and boot partition is that some systems rely on the kernel-image in > rootfs and not in the boot partition. > This leads to duplication of the kernel-image, which can cause > unnecessary storage usage and potential compatibility issues. Except for the use of unnecessary storage, I don't understand exactly what problems can be created by duplication. > This patch provides a solution to this problem by adding a new > parameter "skip-kernel-install" to the wic kickstart file, which can > be passed to the plugin. > If the parameter is provided, the plugin will skip installing the > kernel-image to the boot partition, avoiding duplication and potential > issues. > > By adding this new parameter, we give the users the option to install > the kernel-image only in rootfs, or to install it in both rootfs and > boot partition, depending on their needs and preferences. > This will help to improve the system's storage usage and compatibility. > > Tests for this functionality will be added in the next patch. > > Signed-off-by: Kareem Zarka <kareem.zarka@huawei.com> > --- > scripts/lib/wic/plugins/source/bootimg-efi.py | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/scripts/lib/wic/plugins/source/bootimg-efi.py b/scripts/lib/wic/plugins/source/bootimg-efi.py > index 4b00913a70..363b9f5242 100644 > --- a/scripts/lib/wic/plugins/source/bootimg-efi.py > +++ b/scripts/lib/wic/plugins/source/bootimg-efi.py > @@ -363,9 +363,13 @@ class BootimgEFIPlugin(SourcePlugin): > objcopy_cmd += " %s %s/EFI/Linux/linux.efi" % (efi_stub, hdddir) > exec_native_cmd(objcopy_cmd, native_sysroot) > else: > - install_cmd = "install -m 0644 %s/%s %s/%s" % \ > - (staging_kernel_dir, kernel, hdddir, kernel) > - exec_cmd(install_cmd) > + # skip-kernal-install was added to source_params to conifgure installing the kernel-image. > + # set skip_kernal_install in the kickstart file to skip installing it into hdddir. > + # if not set then the kernel-image will be installed. s/conifgure/configure/ Also check underscores vs dashes. A comment in the code is welcome, but it should not include the history of why this got added. When someone will read this three years from now they don't care. So just remove the first line. > + if not source_params.get('skip-kernal-install'): s/kernal/kernel/, also on other lines. Also remove the unneeded double space. Out of personal taste, I would prefer a positive logic rather than a negative one, e.g.: if source_params.get('install-kernel-into-boot-dir') != "false":
On Tue, 2023-02-07 at 11:49 +0100, Luca Ceresoli via lists.openembedded.org wrote: > Hello Kareem, > > thanks for your patch. > > I have a few suggestions to improve it, see below. > > On Mon, 6 Feb 2023 20:16:14 +0100 > "Kareem Zarka" <zarkakareem@gmail.com> wrote: > > > The issue with installing the kernel-image to both rootfs > > and boot partition is that some systems rely on the kernel-image in > > rootfs and not in the boot partition. > > This leads to duplication of the kernel-image, which can cause > > unnecessary storage usage and potential compatibility issues. > > Except for the use of unnecessary storage, I don't understand exactly > what problems can be created by duplication. > > > This patch provides a solution to this problem by adding a new > > parameter "skip-kernel-install" to the wic kickstart file, which can > > be passed to the plugin. > > If the parameter is provided, the plugin will skip installing the > > kernel-image to the boot partition, avoiding duplication and potential > > issues. > > > > By adding this new parameter, we give the users the option to install > > the kernel-image only in rootfs, or to install it in both rootfs and > > boot partition, depending on their needs and preferences. > > This will help to improve the system's storage usage and compatibility. > > > > Tests for this functionality will be added in the next patch. > > > > Signed-off-by: Kareem Zarka <kareem.zarka@huawei.com> > > --- > > scripts/lib/wic/plugins/source/bootimg-efi.py | 10 +++++++--- > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > diff --git a/scripts/lib/wic/plugins/source/bootimg-efi.py b/scripts/lib/wic/plugins/source/bootimg-efi.py > > index 4b00913a70..363b9f5242 100644 > > --- a/scripts/lib/wic/plugins/source/bootimg-efi.py > > +++ b/scripts/lib/wic/plugins/source/bootimg-efi.py > > @@ -363,9 +363,13 @@ class BootimgEFIPlugin(SourcePlugin): > > objcopy_cmd += " %s %s/EFI/Linux/linux.efi" % (efi_stub, hdddir) > > exec_native_cmd(objcopy_cmd, native_sysroot) > > else: > > - install_cmd = "install -m 0644 %s/%s %s/%s" % \ > > - (staging_kernel_dir, kernel, hdddir, kernel) > > - exec_cmd(install_cmd) > > + # skip-kernal-install was added to source_params to conifgure installing the kernel-image. > > + # set skip_kernal_install in the kickstart file to skip installing it into hdddir. > > + # if not set then the kernel-image will be installed. > > s/conifgure/configure/ > Also check underscores vs dashes. > > A comment in the code is welcome, but it should not include the history > of why this got added. When someone will read this three years from now > they don't care. So just remove the first line. > > > + if not source_params.get('skip-kernal-install'): > > s/kernal/kernel/, also on other lines. > Also remove the unneeded double space. > > Out of personal taste, I would prefer a positive logic rather than a > negative one, e.g.: > > if source_params.get('install-kernel-into-boot-dir') != "false": Whilst I know what you mean, that isn't valid python and the original code is probably more pythonic in that "XXX != False" is a bit different to "not XXX" in python. Cheers, Richard
Dear Richard and Luca, Thank you for your feedback. I will take it into consideration. I understand that "XXX != False" and "not XXX" have different interpretations in Python. The expression "XXX != False" only evaluates to True if XXX is not equal to False. This means that if skip-kernel-install is None, the condition will be False. On the other hand, "not XXX" evaluates to True if XXX is falsy, which includes None, False, 0, empty string, etc. This is why I used it in my code, as I wanted to consider the case where skip-kernel-install is None. Therefore, if skip-kernel-install is not provided, its value will be None, and the condition will be True, which means the kernel image will be installed. However, if we want to enforce the requirement for the user to always provide the parameter skip-kernel-install with either the value true or false, we can make use (!= false , == true). Let me know what suits us best in this case . Kindest regards . Kareem Zarka.
Hello Richard, Kareem, On Tue, 07 Feb 2023 12:32:31 +0000 "Richard Purdie" <richard.purdie@linuxfoundation.org> wrote: > On Tue, 2023-02-07 at 11:49 +0100, Luca Ceresoli via > lists.openembedded.org wrote: > > Hello Kareem, > > > > thanks for your patch. > > > > I have a few suggestions to improve it, see below. > > > > On Mon, 6 Feb 2023 20:16:14 +0100 > > "Kareem Zarka" <zarkakareem@gmail.com> wrote: > > > > > The issue with installing the kernel-image to both rootfs > > > and boot partition is that some systems rely on the kernel-image in > > > rootfs and not in the boot partition. > > > This leads to duplication of the kernel-image, which can cause > > > unnecessary storage usage and potential compatibility issues. > > > > Except for the use of unnecessary storage, I don't understand exactly > > what problems can be created by duplication. > > > > > This patch provides a solution to this problem by adding a new > > > parameter "skip-kernel-install" to the wic kickstart file, which can > > > be passed to the plugin. > > > If the parameter is provided, the plugin will skip installing the > > > kernel-image to the boot partition, avoiding duplication and potential > > > issues. > > > > > > By adding this new parameter, we give the users the option to install > > > the kernel-image only in rootfs, or to install it in both rootfs and > > > boot partition, depending on their needs and preferences. > > > This will help to improve the system's storage usage and compatibility. > > > > > > Tests for this functionality will be added in the next patch. > > > > > > Signed-off-by: Kareem Zarka <kareem.zarka@huawei.com> > > > --- > > > scripts/lib/wic/plugins/source/bootimg-efi.py | 10 +++++++--- > > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > > > diff --git a/scripts/lib/wic/plugins/source/bootimg-efi.py b/scripts/lib/wic/plugins/source/bootimg-efi.py > > > index 4b00913a70..363b9f5242 100644 > > > --- a/scripts/lib/wic/plugins/source/bootimg-efi.py > > > +++ b/scripts/lib/wic/plugins/source/bootimg-efi.py > > > @@ -363,9 +363,13 @@ class BootimgEFIPlugin(SourcePlugin): > > > objcopy_cmd += " %s %s/EFI/Linux/linux.efi" % (efi_stub, hdddir) > > > exec_native_cmd(objcopy_cmd, native_sysroot) > > > else: > > > - install_cmd = "install -m 0644 %s/%s %s/%s" % \ > > > - (staging_kernel_dir, kernel, hdddir, kernel) > > > - exec_cmd(install_cmd) > > > + # skip-kernal-install was added to source_params to conifgure installing the kernel-image. > > > + # set skip_kernal_install in the kickstart file to skip installing it into hdddir. > > > + # if not set then the kernel-image will be installed. > > > > s/conifgure/configure/ > > Also check underscores vs dashes. > > > > A comment in the code is welcome, but it should not include the history > > of why this got added. When someone will read this three years from now > > they don't care. So just remove the first line. > > > > > + if not source_params.get('skip-kernal-install'): > > > > s/kernal/kernel/, also on other lines. > > Also remove the unneeded double space. > > > > Out of personal taste, I would prefer a positive logic rather than a > > negative one, e.g.: > > > > if source_params.get('install-kernel-into-boot-dir') != "false": > > Whilst I know what you mean, that isn't valid python and the original > code is probably more pythonic in that "XXX != False" is a bit > different to "not XXX" in python. Aa, sure, consider the above line just quickyl written pseudocode! :-) Regardless of the implementation, my idea is this: * install-kernel-into-boot-dir is True -> kernel is installed * install-kernel-into-boot-dir is False -> kernel is not installed * install-kernel-into-boot-dir not set -> kernel is installed (for backward compatibility) But as I said, this is out of personal taste and I have a limited perception of the whole problem.
Dear Luca Thank you for the reply . Please check https://lists.openembedded.org/g/openembedded-core/message/176829 when you have time. Kindest regards . Kareem Zarka
diff --git a/scripts/lib/wic/plugins/source/bootimg-efi.py b/scripts/lib/wic/plugins/source/bootimg-efi.py index 4b00913a70..363b9f5242 100644 --- a/scripts/lib/wic/plugins/source/bootimg-efi.py +++ b/scripts/lib/wic/plugins/source/bootimg-efi.py @@ -363,9 +363,13 @@ class BootimgEFIPlugin(SourcePlugin): objcopy_cmd += " %s %s/EFI/Linux/linux.efi" % (efi_stub, hdddir) exec_native_cmd(objcopy_cmd, native_sysroot) else: - install_cmd = "install -m 0644 %s/%s %s/%s" % \ - (staging_kernel_dir, kernel, hdddir, kernel) - exec_cmd(install_cmd) + # skip-kernal-install was added to source_params to conifgure installing the kernel-image. + # set skip_kernal_install in the kickstart file to skip installing it into hdddir. + # if not set then the kernel-image will be installed. + if not source_params.get('skip-kernal-install'): + install_cmd = "install -m 0644 %s/%s %s/%s" % \ + (staging_kernel_dir, kernel, hdddir, kernel) + exec_cmd(install_cmd) if get_bitbake_var("IMAGE_EFI_BOOT_FILES"): for src_path, dst_path in cls.install_task:
The issue with installing the kernel-image to both rootfs and boot partition is that some systems rely on the kernel-image in rootfs and not in the boot partition. This leads to duplication of the kernel-image, which can cause unnecessary storage usage and potential compatibility issues. This patch provides a solution to this problem by adding a new parameter "skip-kernel-install" to the wic kickstart file, which can be passed to the plugin. If the parameter is provided, the plugin will skip installing the kernel-image to the boot partition, avoiding duplication and potential issues. By adding this new parameter, we give the users the option to install the kernel-image only in rootfs, or to install it in both rootfs and boot partition, depending on their needs and preferences. This will help to improve the system's storage usage and compatibility. Tests for this functionality will be added in the next patch. Signed-off-by: Kareem Zarka <kareem.zarka@huawei.com> --- scripts/lib/wic/plugins/source/bootimg-efi.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)