Re: [RFC PATCH] image: Reorder do_copy_boot_files task

[vijai kumar <vijaikumar.kanagarajan@gmail.com>]

Hi Henning,

On Fri, Feb 25, 2022 at 2:01 PM Henning Schild
<henning.schild@siemens.com> wrote:
>
> Am Fri, 18 Feb 2022 15:24:28 +0530
> schrieb Vijai Kumar K <Vijaikumar_Kanagarajan@mentor.com>:
>
> > There might be cases where in there are some initramfs changes in
> > postprocess. For example, via the distro config script.
>
> Postprocessing is a hack reserved for very few special cases, most of
> which should read and not write.
>
> That smells like you are abusing postprocess, and whatever you do there
> might also break kernel updates with "apt-get".
>
> Please give us the example, i bet it can be moved out of postprocess
> into a much more sustainable place (like a package).
>
> With no good example, NACK from me.

Thank you for the quick review comments. Let me try to provide more information.

Postprocess abuse has always been a concern, we have ways to inject
stuff and we also have undocumented rules on
what should or should not be done as part of certain tasks.
We could capture it. Happy to do that.

The end user accidentally steps over this and something breaks.

The original issue was that plymouth theme was changed at a later stage.
The issue was fixed by having it not done as part of postprocess.

We have an option to pass a custom distro configuration script. It is
run as part of postprocess[1].
At best the user could run plymouth-set-default-theme[2] as part of
this script that changes the initramfs during postprocess.

We could not completely rule out modifications in postprocess. And the
rootfs is not complete till rootfs_finalize where we remove the
ability to chroot.
Changes could even then be injected, but that does not happen that
often and the user needs to create a task or append one to do that.
Not as simple as a variable append.

We should take the images from a finalized rootfs, and then deploy it,
so that the images in both deploy and rootfs are the same no matter
whether we use it or not.

Despite the fact that what should or should not be done in rootfs
postprocess, It only makes sense to copy the files to deploy directory
after finalizing the rootfs.

[1]https://github.com/ilbers/isar/blob/ffdd1b0ce026d21c8b62c06c926d205aad3078b6/meta/classes/image-postproc-extension.bbclass#L36
[2]https://manpages.debian.org/buster/plymouth/plymouth-set-default-theme.1

>
> > In such a scenario we would have an outdated initramfs file in deploy
> > directory. Certain downstream Wic plugins directly consume the image
> > from deploy directory. It then uses the outdated initramfs for
> > creating the wic image.
>
> Maybe those downstream wic plugins deserve fixing as well, take the
> initrd and kernel out of root/boot.
>
> For image type wic there should in fact be no reason to copy the boot
> files to deploy. That is something for ext4 and others going to "qemu
> --kernel ..."

Maybe, but ideally deploy should have the final kernel and initrd
shipped, no matter whether its a wic image or ext4img.
We could have a scenario where we build multiple image types, both
ends up with different initramfs because we used the file from
different places(IMAGE_ROOTFS/DEPLOY_DIR)

I hope this gives some justification to the patch?

Thanks,
Vijai Kumar K

> While those files are there for any image i would not suggest using
> them. We have lava setups where we convert a wic into
> kernel+initrd+nfsrootfs+cmdline, all taken only out of that wic. No
> side channels. I guess one could also look into ".wic.img.p1" but the
> boot files have no place as output for wic images, let alone as input
> for wic.
>
> regards,
> Henning
>
> > Copy boot files after rootfs postprocess but before finalizing the
> > rootfs.
> >
> > Signed-off-by: Vijai Kumar K <Vijaikumar_Kanagarajan@mentor.com>
> > ---
> >  RECIPE-API-CHANGELOG.md    | 7 ++++++-
> >  meta/classes/image.bbclass | 2 +-
> >  2 files changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/RECIPE-API-CHANGELOG.md b/RECIPE-API-CHANGELOG.md
> > index cad15a8..ef53b1a 100644
> > --- a/RECIPE-API-CHANGELOG.md
> > +++ b/RECIPE-API-CHANGELOG.md
> > @@ -342,4 +342,9 @@ The bitbake variable defines the respective
> > environment variable which is availa When cross compiling, `cross` is
> > added to the `DEB_BUILD_PROFILES` environment variable. Please note,
> > that manually exported versions of the variables are overwritten.
> > -For a list of well-known Debian build profiles and common practices,
> > we refer to Debian's BuildProfileSpec. \ No newline at end of file
> > +For a list of well-known Debian build profiles and common practices,
> > we refer to Debian's BuildProfileSpec. +
> > +### Move do_copy_boot_files task after do_rootfs_postprocess
> > +
> > +The boot-files(kernel, initrd, dtbs) are now shipped to
> > tmp/deploy/images after +do_rootfs_postprocess task and before
> > do_rootfs_finalize task. diff --git a/meta/classes/image.bbclass
> > b/meta/classes/image.bbclass index 6d77243..d70a93b 100644
> > --- a/meta/classes/image.bbclass
> > +++ b/meta/classes/image.bbclass
> > @@ -170,7 +170,7 @@ do_copy_boot_files() {
> >          cp -f "$dtb" "${DEPLOY_DIR_IMAGE}/"
> >      done
> >  }
> > -addtask copy_boot_files before do_rootfs_postprocess after
> > do_rootfs_install +addtask copy_boot_files before do_rootfs_finalize
> > after do_rootfs_postprocess
> >  python do_image_tools() {
> >      """Virtual task"""
>
> --
> You received this message because you are subscribed to the Google Groups "isar-users" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to isar-users+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/isar-users/20220225093137.54487e31%40md1za8fc.ad001.siemens.net.