[PATCH v2 0/3] Fix data-race in deployment of initrd

[PATCH v2 0/3] Fix data-race in deployment of initrd

[Felix Moessbauer]

Changes since v1:

- add API changelog entry
- fix expression in INITRD_DEPLOY_FILE
- do not append ${PN} to deploy-dir in image.bb

This patch fixes a data race in the deployment of the initrd, as discussed on
the ML in "Issues creating images with custom initramfs". As a measure against
further races on these files, we also use the sstate cache for deployment.

Please note, that imaging with wic + grub or systemd-boot with a custom
initramfs is still broken. This is due to the wic logic, which reads the
initrd in the rootfs, but not the one in the deploy dir.

Best regards,
Felix Moessbauer
Siemens AG

Felix Moessbauer (3):
  add initramfs to sstate-cache
  deploy boot files via sstate-cache
  fix race-cond between default and custom initrd

 RECIPE-API-CHANGELOG.md        |  8 ++++++++
 meta/classes/image.bbclass     | 36 ++++++++++++++++++++++------------
 meta/classes/initramfs.bbclass | 18 ++++++++++++-----
 scripts/start_vm               |  4 ++--
 testsuite/start_vm.py          |  2 +-
 5 files changed, 48 insertions(+), 20 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] add initramfs to sstate-cache

[Felix Moessbauer]

This patch changes how we deploy the initramfs.
Instead of manually deploying, we use the sstate infrastructure for
that. By that, accidental overrides of the artifact can be
automatically detected. On clean, the artifact is also cleaned.

Signed-off-by: Felix Moessbauer <felix.moessbauer@siemens.com>
---
 meta/classes/initramfs.bbclass | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/meta/classes/initramfs.bbclass b/meta/classes/initramfs.bbclass
index a5141a53..183d1933 100644
--- a/meta/classes/initramfs.bbclass
+++ b/meta/classes/initramfs.bbclass
@@ -2,16 +2,19 @@
 
 # Make workdir and stamps machine-specific without changing common PN target
 WORKDIR = "${TMPDIR}/work/${DISTRO}-${DISTRO_ARCH}/${PN}-${MACHINE}/${PV}-${PR}"
+DEPLOYDIR = "${WORKDIR}/deploy"
 STAMP = "${STAMPS_DIR}/${DISTRO}-${DISTRO_ARCH}/${PN}-${MACHINE}/${PV}-${PR}"
 STAMPCLEAN = "${STAMPS_DIR}/${DISTRO}-${DISTRO_ARCH}/${PN}-${MACHINE}/*-*"
 
 # Sstate also needs to be machine-specific
 SSTATE_MANIFESTS = "${TMPDIR}/sstate-control/${MACHINE}-${DISTRO}-${DISTRO_ARCH}"
+SSTATETASKS += "do_generate_initramfs"
 
 INITRAMFS_INSTALL ?= ""
 INITRAMFS_PREINSTALL ?= ""
 INITRAMFS_ROOTFS ?= "${WORKDIR}/rootfs"
-INITRAMFS_IMAGE_FILE = "${DEPLOY_DIR_IMAGE}/${INITRAMFS_FULLNAME}.initrd.img"
+INITRAMFS_IMAGE_NAME = "${INITRAMFS_FULLNAME}.initrd.img"
+INITRAMFS_IMAGE_FILE = "${DEPLOY_DIR_IMAGE}/${INITRAMFS_IMAGE_NAME}"
 
 # Install proper kernel
 INITRAMFS_INSTALL += "${@ ("linux-image-" + d.getVar("KERNEL_NAME", True)) if d.getVar("KERNEL_NAME", True) else ""}"
@@ -27,8 +30,10 @@ ROOTFS_PACKAGES = "initramfs-tools ${INITRAMFS_PREINSTALL} ${INITRAMFS_INSTALL}"
 
 inherit rootfs
 
-do_generate_initramfs[dirs] = "${DEPLOY_DIR_IMAGE}"
 do_generate_initramfs[network] = "${TASK_USE_SUDO}"
+do_generate_initramfs[cleandirs] += "${DEPLOYDIR}"
+do_generate_initramfs[sstate-inputdirs] = "${DEPLOYDIR}"
+do_generate_initramfs[sstate-outputdirs] = "${DEPLOY_DIR_IMAGE}"
 do_generate_initramfs() {
     rootfs_do_mounts
     rootfs_do_qemu
@@ -44,8 +49,11 @@ do_generate_initramfs() {
     if [ ! -e "${INITRAMFS_ROOTFS}/initrd.img" ]; then
         die "No initramfs was found after generation!"
     fi
-
-    rm -rf "${INITRAMFS_IMAGE_FILE}"
-    cp "${INITRAMFS_ROOTFS}/initrd.img" "${INITRAMFS_IMAGE_FILE}"
+    cp ${INITRAMFS_ROOTFS}/initrd.img ${DEPLOYDIR}/${INITRAMFS_IMAGE_NAME}
 }
 addtask generate_initramfs after do_rootfs before do_build
+
+python do_generate_initramfs_setscene () {
+    sstate_setscene(d)
+}
+addtask do_generate_initramfs_setscene
-- 
2.34.1

[PATCH v2 2/3] deploy boot files via sstate-cache

[Felix Moessbauer]

This patch changes how we deploy the boot files.
Instead of manually deploying, we use the sstate infrastructure for
that. By that, accidental overrides of the artifacts can be
automatically detected. On clean, the artifacts are also cleaned.

Signed-off-by: Felix Moessbauer <felix.moessbauer@siemens.com>
---
 meta/classes/image.bbclass | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
index ef7d5a2a..6277069f 100644
--- a/meta/classes/image.bbclass
+++ b/meta/classes/image.bbclass
@@ -3,11 +3,13 @@
 
 # Make workdir and stamps machine-specific without changing common PN target
 WORKDIR = "${TMPDIR}/work/${DISTRO}-${DISTRO_ARCH}/${PN}-${MACHINE}/${PV}-${PR}"
+DEPLOYDIR = "${WORKDIR}/deploy"
 STAMP = "${STAMPS_DIR}/${DISTRO}-${DISTRO_ARCH}/${PN}-${MACHINE}/${PV}-${PR}"
 STAMPCLEAN = "${STAMPS_DIR}/${DISTRO}-${DISTRO_ARCH}/${PN}-${MACHINE}/*-*"
 
 # Sstate also needs to be machine-specific
 SSTATE_MANIFESTS = "${TMPDIR}/sstate-control/${MACHINE}-${DISTRO}-${DISTRO_ARCH}"
+SSTATETASKS += "do_copy_boot_files"
 
 IMAGE_INSTALL ?= ""
 IMAGE_FSTYPES ?= "${@ d.getVar("IMAGE_TYPE", True) if d.getVar("IMAGE_TYPE", True) else "ext4"}"
@@ -355,8 +357,9 @@ INITRD_IMG = "${PP_DEPLOY}/${INITRD_IMAGE}"
 # only one dtb file supported, pick the first
 DTB_IMG = "${PP_DEPLOY}/${@(d.getVar('DTB_FILES').split() or [''])[0]}"
 
-do_copy_boot_files[dirs] = "${DEPLOY_DIR_IMAGE}"
-do_copy_boot_files[lockfiles] += "${DEPLOY_DIR_IMAGE}/isar.lock"
+do_copy_boot_files[cleandirs] += "${DEPLOYDIR}"
+do_copy_boot_files[sstate-inputdirs] = "${DEPLOYDIR}"
+do_copy_boot_files[sstate-outputdirs] = "${DEPLOY_DIR_IMAGE}"
 do_copy_boot_files[network] = "${TASK_USE_SUDO}"
 do_copy_boot_files() {
     kernel="$(realpath -q '${IMAGE_ROOTFS}'/vmlinu[xz])"
@@ -364,7 +367,7 @@ do_copy_boot_files() {
         kernel="$(realpath -q '${IMAGE_ROOTFS}'/boot/vmlinu[xz])"
     fi
     if [ -f "$kernel" ]; then
-        sudo cat "$kernel" > "${DEPLOY_DIR_IMAGE}/${KERNEL_IMAGE}"
+        sudo cat "$kernel" > "${DEPLOYDIR}/${KERNEL_IMAGE}"
     fi
 
     initrd="$(realpath -q '${IMAGE_ROOTFS}/initrd.img')"
@@ -372,7 +375,7 @@ do_copy_boot_files() {
         initrd="$(realpath -q '${IMAGE_ROOTFS}/boot/initrd.img')"
     fi
     if [ -f "$initrd" ]; then
-        cp -f "$initrd" '${DEPLOY_DIR_IMAGE}/${INITRD_IMAGE}'
+        cp -f "$initrd" '${DEPLOYDIR}/${INITRD_IMAGE}'
     fi
 
     for file in ${DTB_FILES}; do
@@ -383,11 +386,16 @@ do_copy_boot_files() {
             die "${file} not found"
         fi
 
-        cp -f "$dtb" "${DEPLOY_DIR_IMAGE}/"
+        cp -f "$dtb" "${DEPLOYDIR}/"
     done
 }
 addtask copy_boot_files before do_rootfs_postprocess after do_rootfs_install
 
+python do_copy_boot_files_setscene () {
+    sstate_setscene(d)
+}
+addtask do_copy_boot_files_setscene
+
 python do_image_tools() {
     """Virtual task"""
     pass
-- 
2.34.1

[PATCH v2 3/3] fix race-cond between default and custom initrd

[Felix Moessbauer]

This patch fixes a data race happening when building a custom initrd.
Previously, both custom and default initrds were deployed to the image
deploy dir. The race is fixed by conditionally deploying either the
custom or the default one. For that, we introduce a new variable
INITRD_DEPLOY_FILE which provides the name of the initrd in the deploy
directory. The existing INITRD_IMAGE variable is defaulted to the empty
string and used to control if a custom initrd is requrested. Only if
this variable is empty, the default one is deployed.

Signed-off-by: Felix Moessbauer <felix.moessbauer@siemens.com>
---
 RECIPE-API-CHANGELOG.md    |  8 ++++++++
 meta/classes/image.bbclass | 20 ++++++++++++--------
 scripts/start_vm           |  4 ++--
 testsuite/start_vm.py      |  2 +-
 4 files changed, 23 insertions(+), 11 deletions(-)

diff --git a/RECIPE-API-CHANGELOG.md b/RECIPE-API-CHANGELOG.md
index e48c98c7..1e8dbfc8 100644
--- a/RECIPE-API-CHANGELOG.md
+++ b/RECIPE-API-CHANGELOG.md
@@ -476,3 +476,11 @@ Bitbake 2.0 for better performance. It also requires isar-sstate script to be
 migrated to zstd.
 Mixing old Gzip-based and new ZStandatd-based sstate cache is not recommended
 and should be avoid for correct compatibility.
+
+### Working with a custom initramfs
+
+The existing `INITRD_IMAGE` variable is defaulted to the empty string and used to
+control if a custom initrd is requrested. Only if this variable is empty, the
+default one is deployed. By that, the variable cannot be used to get the name of
+the images initramfs. Instead, the variable `INITRD_DEPLOY_FILE` is provided which
+always povides the name of the initrd file (also when the default one is used).
diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
index 6277069f..7b3551b0 100644
--- a/meta/classes/image.bbclass
+++ b/meta/classes/image.bbclass
@@ -23,7 +23,8 @@ IMAGE_FULLNAME = "${PN}-${DISTRO}-${MACHINE}"
 
 # These variables are used by wic and start_vm
 KERNEL_IMAGE ?= "${IMAGE_FULLNAME}-${KERNEL_FILE}"
-INITRD_IMAGE ?= "${IMAGE_FULLNAME}-initrd.img"
+INITRD_IMAGE ?= ""
+INITRD_DEPLOY_FILE = "${@ d.getVar('INITRD_IMAGE', True) or '${IMAGE_FULLNAME}-initrd.img'}"
 
 # This defines the deployed dtbs for reuse by imagers
 DTB_FILES ?= ""
@@ -353,7 +354,7 @@ EOF
 
 # Default kernel, initrd and dtb image deploy paths (inside imager)
 KERNEL_IMG = "${PP_DEPLOY}/${KERNEL_IMAGE}"
-INITRD_IMG = "${PP_DEPLOY}/${INITRD_IMAGE}"
+INITRD_IMG = "${PP_DEPLOY}/${INITRD_DEPLOY_FILE}"
 # only one dtb file supported, pick the first
 DTB_IMG = "${PP_DEPLOY}/${@(d.getVar('DTB_FILES').split() or [''])[0]}"
 
@@ -370,12 +371,15 @@ do_copy_boot_files() {
         sudo cat "$kernel" > "${DEPLOYDIR}/${KERNEL_IMAGE}"
     fi
 
-    initrd="$(realpath -q '${IMAGE_ROOTFS}/initrd.img')"
-    if [ ! -f "$initrd" ]; then
-        initrd="$(realpath -q '${IMAGE_ROOTFS}/boot/initrd.img')"
-    fi
-    if [ -f "$initrd" ]; then
-        cp -f "$initrd" '${DEPLOYDIR}/${INITRD_IMAGE}'
+    if [ -e "${INITRD_IMAGE}" ]; then
+        # deploy default initrd if no custom one is build
+        initrd="$(realpath -q '${IMAGE_ROOTFS}/initrd.img')"
+        if [ ! -f "$initrd" ]; then
+            initrd="$(realpath -q '${IMAGE_ROOTFS}/boot/initrd.img')"
+        fi
+        if [ -f "$initrd" ]; then
+            cp -f "$initrd" '${DEPLOYDIR}/${INITRD_DEPLOY_FILE}'
+        fi
     fi
 
     for file in ${DTB_FILES}; do
diff --git a/scripts/start_vm b/scripts/start_vm
index 17091d72..8c696a4a 100755
--- a/scripts/start_vm
+++ b/scripts/start_vm
@@ -125,10 +125,10 @@ case "$IMAGE_FSTYPES" in
     readonly ROOTFS_IMAGE=$IMAGE_FULLNAME.ext4
 
     eval $(bitbake -e mc:qemu$ARCH-$DISTRO:isar-image-base | grep "^KERNEL_IMAGE=")
-    eval $(bitbake -e mc:qemu$ARCH-$DISTRO:isar-image-base | grep "^INITRD_IMAGE=")
+    eval $(bitbake -e mc:qemu$ARCH-$DISTRO:isar-image-base | grep "^INITRD_DEPLOY_FILE=")
     QKERNEL=$IMAGE_DIR/${KERNEL_IMAGE}
     QINITRD=/dev/null
-    [ -n "$INITRD_IMAGE" ] && QINITRD=$IMAGE_DIR/${INITRD_IMAGE}
+    [ -n "$INITRD_DEPLOY_FILE" ] && QINITRD=$IMAGE_DIR/${INITRD_DEPLOY_FILE}
     if [ "$ARCH" = "riscv64" ]; then
         EXTRA_ARGS="$EXTRA_ARGS -device loader,file=$QKERNEL,addr=0x80200000"
         QKERNEL="/usr/lib/riscv64-linux-gnu/opensbi/qemu/virt/fw_jump.elf"
diff --git a/testsuite/start_vm.py b/testsuite/start_vm.py
index 82ecc17d..ba1ba127 100755
--- a/testsuite/start_vm.py
+++ b/testsuite/start_vm.py
@@ -35,7 +35,7 @@ def format_qemu_cmdline(arch, build, distro, out, pid, enforce_pcbios=False):
     if image_type == 'ext4':
         rootfs_image = 'isar-image-base-' + base + '-' + distro + '-qemu' + arch + '.ext4'
         kernel_image = deploy_dir_image + '/' + get_bitbake_var(bb_output, 'KERNEL_IMAGE')
-        initrd_image = get_bitbake_var(bb_output, 'INITRD_IMAGE')
+        initrd_image = get_bitbake_var(bb_output, 'INITRD_DEPLOY_FILE')
 
         if not initrd_image:
             initrd_image = '/dev/null'
-- 
2.34.1

Re: [PATCH v2 0/3] Fix data-race in deployment of initrd

[Uladzimir Bely]

In the email from Saturday, 18 February 2023 13:30:35 +03 user Felix 
Moessbauer wrote:
> Changes since v1:
> 
> - add API changelog entry
> - fix expression in INITRD_DEPLOY_FILE
> - do not append ${PN} to deploy-dir in image.bb
> 
> This patch fixes a data race in the deployment of the initrd, as discussed
> on the ML in "Issues creating images with custom initramfs". As a measure
> against further races on these files, we also use the sstate cache for
> deployment.
> 
> Please note, that imaging with wic + grub or systemd-boot with a custom
> initramfs is still broken. This is due to the wic logic, which reads the
> initrd in the rootfs, but not the one in the deploy dir.
> 
> Best regards,
> Felix Moessbauer
> Siemens AG
> 
> Felix Moessbauer (3):
>   add initramfs to sstate-cache
>   deploy boot files via sstate-cache
>   fix race-cond between default and custom initrd
> 
>  RECIPE-API-CHANGELOG.md        |  8 ++++++++
>  meta/classes/image.bbclass     | 36 ++++++++++++++++++++++------------
>  meta/classes/initramfs.bbclass | 18 ++++++++++++-----
>  scripts/start_vm               |  4 ++--
>  testsuite/start_vm.py          |  2 +-
>  5 files changed, 48 insertions(+), 20 deletions(-)

While testing this patchset, discovered that initrd files for some targets 
(like qemuarm-buillseye, qemuarm64-bullseye) are not deployed, so run tests 
for these qemu machines fail.

The issue can is also easily reproducible with kas.

Re: [PATCH v2 3/3] fix race-cond between default and custom initrd

[Uladzimir Bely]

In the email from Saturday, 18 February 2023 13:30:38 +03 user Felix Moessbauer wrote:
> This patch fixes a data race happening when building a custom initrd.
> Previously, both custom and default initrds were deployed to the image
> deploy dir. The race is fixed by conditionally deploying either the
> custom or the default one. For that, we introduce a new variable
> INITRD_DEPLOY_FILE which provides the name of the initrd in the deploy
> directory. The existing INITRD_IMAGE variable is defaulted to the empty
> string and used to control if a custom initrd is requrested. Only if
> this variable is empty, the default one is deployed.
> 
> Signed-off-by: Felix Moessbauer <felix.moessbauer@siemens.com>
> ---
>  RECIPE-API-CHANGELOG.md    |  8 ++++++++
>  meta/classes/image.bbclass | 20 ++++++++++++--------
>  scripts/start_vm           |  4 ++--
>  testsuite/start_vm.py      |  2 +-
>  4 files changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/RECIPE-API-CHANGELOG.md b/RECIPE-API-CHANGELOG.md
> index e48c98c7..1e8dbfc8 100644
> --- a/RECIPE-API-CHANGELOG.md
> +++ b/RECIPE-API-CHANGELOG.md
> @@ -476,3 +476,11 @@ Bitbake 2.0 for better performance. It also requires isar-sstate script to be
>  migrated to zstd.
>  Mixing old Gzip-based and new ZStandatd-based sstate cache is not recommended
>  and should be avoid for correct compatibility.
> +
> +### Working with a custom initramfs
> +
> +The existing `INITRD_IMAGE` variable is defaulted to the empty string and used to
> +control if a custom initrd is requrested. Only if this variable is empty, the
> +default one is deployed. By that, the variable cannot be used to get the name of
> +the images initramfs. Instead, the variable `INITRD_DEPLOY_FILE` is provided which
> +always povides the name of the initrd file (also when the default one is used).
> diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
> index 6277069f..7b3551b0 100644
> --- a/meta/classes/image.bbclass
> +++ b/meta/classes/image.bbclass
> @@ -23,7 +23,8 @@ IMAGE_FULLNAME = "${PN}-${DISTRO}-${MACHINE}"
>  
>  # These variables are used by wic and start_vm
>  KERNEL_IMAGE ?= "${IMAGE_FULLNAME}-${KERNEL_FILE}"
> -INITRD_IMAGE ?= "${IMAGE_FULLNAME}-initrd.img"
> +INITRD_IMAGE ?= ""
> +INITRD_DEPLOY_FILE = "${@ d.getVar('INITRD_IMAGE', True) or '${IMAGE_FULLNAME}-initrd.img'}"
>  
>  # This defines the deployed dtbs for reuse by imagers
>  DTB_FILES ?= ""
> @@ -353,7 +354,7 @@ EOF
>  
>  # Default kernel, initrd and dtb image deploy paths (inside imager)
>  KERNEL_IMG = "${PP_DEPLOY}/${KERNEL_IMAGE}"
> -INITRD_IMG = "${PP_DEPLOY}/${INITRD_IMAGE}"
> +INITRD_IMG = "${PP_DEPLOY}/${INITRD_DEPLOY_FILE}"
>  # only one dtb file supported, pick the first
>  DTB_IMG = "${PP_DEPLOY}/${@(d.getVar('DTB_FILES').split() or [''])[0]}"
>  
> @@ -370,12 +371,15 @@ do_copy_boot_files() {
>          sudo cat "$kernel" > "${DEPLOYDIR}/${KERNEL_IMAGE}"
>      fi
>  
> -    initrd="$(realpath -q '${IMAGE_ROOTFS}/initrd.img')"
> -    if [ ! -f "$initrd" ]; then
> -        initrd="$(realpath -q '${IMAGE_ROOTFS}/boot/initrd.img')"
> -    fi
> -    if [ -f "$initrd" ]; then
> -        cp -f "$initrd" '${DEPLOYDIR}/${INITRD_IMAGE}'
> +    if [ -e "${INITRD_IMAGE}" ]; then
> +        # deploy default initrd if no custom one is build
> +        initrd="$(realpath -q '${IMAGE_ROOTFS}/initrd.img')"
> +        if [ ! -f "$initrd" ]; then
> +            initrd="$(realpath -q '${IMAGE_ROOTFS}/boot/initrd.img')"
> +        fi
> +        if [ -f "$initrd" ]; then
> +            cp -f "$initrd" '${DEPLOYDIR}/${INITRD_DEPLOY_FILE}'
> +        fi
>      fi
>  
>      for file in ${DTB_FILES}; do
> diff --git a/scripts/start_vm b/scripts/start_vm
> index 17091d72..8c696a4a 100755
> --- a/scripts/start_vm
> +++ b/scripts/start_vm
> @@ -125,10 +125,10 @@ case "$IMAGE_FSTYPES" in
>      readonly ROOTFS_IMAGE=$IMAGE_FULLNAME.ext4
>  
>      eval $(bitbake -e mc:qemu$ARCH-$DISTRO:isar-image-base | grep "^KERNEL_IMAGE=")
> -    eval $(bitbake -e mc:qemu$ARCH-$DISTRO:isar-image-base | grep "^INITRD_IMAGE=")
> +    eval $(bitbake -e mc:qemu$ARCH-$DISTRO:isar-image-base | grep "^INITRD_DEPLOY_FILE=")
>      QKERNEL=$IMAGE_DIR/${KERNEL_IMAGE}
>      QINITRD=/dev/null
> -    [ -n "$INITRD_IMAGE" ] && QINITRD=$IMAGE_DIR/${INITRD_IMAGE}
> +    [ -n "$INITRD_DEPLOY_FILE" ] && QINITRD=$IMAGE_DIR/${INITRD_DEPLOY_FILE}
>      if [ "$ARCH" = "riscv64" ]; then
>          EXTRA_ARGS="$EXTRA_ARGS -device loader,file=$QKERNEL,addr=0x80200000"
>          QKERNEL="/usr/lib/riscv64-linux-gnu/opensbi/qemu/virt/fw_jump.elf"
> diff --git a/testsuite/start_vm.py b/testsuite/start_vm.py
> index 82ecc17d..ba1ba127 100755
> --- a/testsuite/start_vm.py
> +++ b/testsuite/start_vm.py
> @@ -35,7 +35,7 @@ def format_qemu_cmdline(arch, build, distro, out, pid, enforce_pcbios=False):
>      if image_type == 'ext4':
>          rootfs_image = 'isar-image-base-' + base + '-' + distro + '-qemu' + arch + '.ext4'
>          kernel_image = deploy_dir_image + '/' + get_bitbake_var(bb_output, 'KERNEL_IMAGE')
> -        initrd_image = get_bitbake_var(bb_output, 'INITRD_IMAGE')
> +        initrd_image = get_bitbake_var(bb_output, 'INITRD_DEPLOY_FILE')

This seems to be not enough to change the name without directory, since it's later concatenated with old one (deploy_dir_image)

>  
>          if not initrd_image:
>              initrd_image = '/dev/null'
> 
        else:
            initrd_image = deploy_dir_image + '/' + initrd_image`

Moreover, I would say, it's not good that we should now look for initrd file somewhere in DEPLOYDIR="${WORKDIR}/deploy", while everyone would expect to find it DEPLOY_DIR_IMAGE="${DEPLOY_DIR}/images/${MACHINE}"...

Re: [PATCH v2 3/3] fix race-cond between default and custom initrd

[Moessbauer, Felix]

On Wed, 2023-02-22 at 20:40 +0300, Uladzimir Bely wrote:
> In the email from Saturday, 18 February 2023 13:30:38 +03 user Felix
> Moessbauer wrote:
> > This patch fixes a data race happening when building a custom
> > initrd.
> > Previously, both custom and default initrds were deployed to the
> > image
> > deploy dir. The race is fixed by conditionally deploying either the
> > custom or the default one. For that, we introduce a new variable
> > INITRD_DEPLOY_FILE which provides the name of the initrd in the
> > deploy
> > directory. The existing INITRD_IMAGE variable is defaulted to the
> > empty
> > string and used to control if a custom initrd is requrested. Only
> > if
> > this variable is empty, the default one is deployed.
> > 
> > Signed-off-by: Felix Moessbauer <felix.moessbauer@siemens.com>
> > ---
> >  RECIPE-API-CHANGELOG.md    |  8 ++++++++
> >  meta/classes/image.bbclass | 20 ++++++++++++--------
> >  scripts/start_vm           |  4 ++--
> >  testsuite/start_vm.py      |  2 +-
> >  4 files changed, 23 insertions(+), 11 deletions(-)
> > 
> > diff --git a/RECIPE-API-CHANGELOG.md b/RECIPE-API-CHANGELOG.md
> > index e48c98c7..1e8dbfc8 100644
> > --- a/RECIPE-API-CHANGELOG.md
> > +++ b/RECIPE-API-CHANGELOG.md
> > @@ -476,3 +476,11 @@ Bitbake 2.0 for better performance. It also
> > requires isar-sstate script to be
> >  migrated to zstd.
> >  Mixing old Gzip-based and new ZStandatd-based sstate cache is not
> > recommended
> >  and should be avoid for correct compatibility.
> > +
> > +### Working with a custom initramfs
> > +
> > +The existing `INITRD_IMAGE` variable is defaulted to the empty
> > string and used to
> > +control if a custom initrd is requrested. Only if this variable is
> > empty, the
> > +default one is deployed. By that, the variable cannot be used to
> > get the name of
> > +the images initramfs. Instead, the variable `INITRD_DEPLOY_FILE`
> > is provided which
> > +always povides the name of the initrd file (also when the default
> > one is used).
> > diff --git a/meta/classes/image.bbclass
> > b/meta/classes/image.bbclass
> > index 6277069f..7b3551b0 100644
> > --- a/meta/classes/image.bbclass
> > +++ b/meta/classes/image.bbclass
> > @@ -23,7 +23,8 @@ IMAGE_FULLNAME = "${PN}-${DISTRO}-${MACHINE}"
> >  
> >  # These variables are used by wic and start_vm
> >  KERNEL_IMAGE ?= "${IMAGE_FULLNAME}-${KERNEL_FILE}"
> > -INITRD_IMAGE ?= "${IMAGE_FULLNAME}-initrd.img"
> > +INITRD_IMAGE ?= ""
> > +INITRD_DEPLOY_FILE = "${@ d.getVar('INITRD_IMAGE', True) or
> > '${IMAGE_FULLNAME}-initrd.img'}"
> >  
> >  # This defines the deployed dtbs for reuse by imagers
> >  DTB_FILES ?= ""
> > @@ -353,7 +354,7 @@ EOF
> >  
> >  # Default kernel, initrd and dtb image deploy paths (inside
> > imager)
> >  KERNEL_IMG = "${PP_DEPLOY}/${KERNEL_IMAGE}"
> > -INITRD_IMG = "${PP_DEPLOY}/${INITRD_IMAGE}"
> > +INITRD_IMG = "${PP_DEPLOY}/${INITRD_DEPLOY_FILE}"
> >  # only one dtb file supported, pick the first
> >  DTB_IMG = "${PP_DEPLOY}/${@(d.getVar('DTB_FILES').split() or
> > [''])[0]}"
> >  
> > @@ -370,12 +371,15 @@ do_copy_boot_files() {
> >          sudo cat "$kernel" > "${DEPLOYDIR}/${KERNEL_IMAGE}"
> >      fi
> >  
> > -    initrd="$(realpath -q '${IMAGE_ROOTFS}/initrd.img')"
> > -    if [ ! -f "$initrd" ]; then
> > -        initrd="$(realpath -q '${IMAGE_ROOTFS}/boot/initrd.img')"
> > -    fi
> > -    if [ -f "$initrd" ]; then
> > -        cp -f "$initrd" '${DEPLOYDIR}/${INITRD_IMAGE}'
> > +    if [ -e "${INITRD_IMAGE}" ]; then
> > +        # deploy default initrd if no custom one is build
> > +        initrd="$(realpath -q '${IMAGE_ROOTFS}/initrd.img')"
> > +        if [ ! -f "$initrd" ]; then
> > +            initrd="$(realpath -q
> > '${IMAGE_ROOTFS}/boot/initrd.img')"
> > +        fi
> > +        if [ -f "$initrd" ]; then
> > +            cp -f "$initrd" '${DEPLOYDIR}/${INITRD_DEPLOY_FILE}'
> > +        fi
> >      fi
> >  
> >      for file in ${DTB_FILES}; do
> > diff --git a/scripts/start_vm b/scripts/start_vm
> > index 17091d72..8c696a4a 100755
> > --- a/scripts/start_vm
> > +++ b/scripts/start_vm
> > @@ -125,10 +125,10 @@ case "$IMAGE_FSTYPES" in
> >      readonly ROOTFS_IMAGE=$IMAGE_FULLNAME.ext4
> >  
> >      eval $(bitbake -e mc:qemu$ARCH-$DISTRO:isar-image-base | grep
> > "^KERNEL_IMAGE=")
> > -    eval $(bitbake -e mc:qemu$ARCH-$DISTRO:isar-image-base | grep
> > "^INITRD_IMAGE=")
> > +    eval $(bitbake -e mc:qemu$ARCH-$DISTRO:isar-image-base | grep
> > "^INITRD_DEPLOY_FILE=")
> >      QKERNEL=$IMAGE_DIR/${KERNEL_IMAGE}
> >      QINITRD=/dev/null
> > -    [ -n "$INITRD_IMAGE" ] && QINITRD=$IMAGE_DIR/${INITRD_IMAGE}
> > +    [ -n "$INITRD_DEPLOY_FILE" ] &&
> > QINITRD=$IMAGE_DIR/${INITRD_DEPLOY_FILE}
> >      if [ "$ARCH" = "riscv64" ]; then
> >          EXTRA_ARGS="$EXTRA_ARGS -device
> > loader,file=$QKERNEL,addr=0x80200000"
> >          QKERNEL="/usr/lib/riscv64-linux-
> > gnu/opensbi/qemu/virt/fw_jump.elf"
> > diff --git a/testsuite/start_vm.py b/testsuite/start_vm.py
> > index 82ecc17d..ba1ba127 100755
> > --- a/testsuite/start_vm.py
> > +++ b/testsuite/start_vm.py
> > @@ -35,7 +35,7 @@ def format_qemu_cmdline(arch, build, distro, out,
> > pid, enforce_pcbios=False):
> >      if image_type == 'ext4':
> >          rootfs_image = 'isar-image-base-' + base + '-' + distro +
> > '-qemu' + arch + '.ext4'
> >          kernel_image = deploy_dir_image + '/' +
> > get_bitbake_var(bb_output, 'KERNEL_IMAGE')
> > -        initrd_image = get_bitbake_var(bb_output, 'INITRD_IMAGE')
> > +        initrd_image = get_bitbake_var(bb_output,
> > 'INITRD_DEPLOY_FILE')
> 
> This seems to be not enough to change the name without directory,
> since it's later concatenated with old one (deploy_dir_image)

Why would that make a difference? The new INITRD_DEPLOY_FILE variable
should behave exactly the same as the previous INITRD_IMAGE, when set.
If not, then this is a bug in my patch.

> 
> >  
> >          if not initrd_image:
> >              initrd_image = '/dev/null'
> > 
>         else:
>             initrd_image = deploy_dir_image + '/' + initrd_image`
> 
> Moreover, I would say, it's not good that we should now look for
> initrd file somewhere in DEPLOYDIR="${WORKDIR}/deploy", while
> everyone would expect to find it
> DEPLOY_DIR_IMAGE="${DEPLOY_DIR}/images/${MACHINE}"...

This should also not be the case. The deploy location should be exactly
the same as before. In my local tests this also always was the case.
I'll have a second look.

With all these changes to the testsuite I have to admit that I simply
did not run it. I lost track on how to use it after the changes.

I will have a second look at the patch series.

Felix

> 
> 
> 
> 

Re: [PATCH v2 3/3] fix race-cond between default and custom initrd

[Moessbauer, Felix]

On Sat, 2023-02-18 at 10:30 +0000, Felix Moessbauer wrote:
> This patch fixes a data race happening when building a custom initrd.
> Previously, both custom and default initrds were deployed to the
> image
> deploy dir. The race is fixed by conditionally deploying either the
> custom or the default one. For that, we introduce a new variable
> INITRD_DEPLOY_FILE which provides the name of the initrd in the
> deploy
> directory. The existing INITRD_IMAGE variable is defaulted to the
> empty
> string and used to control if a custom initrd is requrested. Only if
> this variable is empty, the default one is deployed.
> 
> Signed-off-by: Felix Moessbauer <felix.moessbauer@siemens.com>
> ---
>  RECIPE-API-CHANGELOG.md    |  8 ++++++++
>  meta/classes/image.bbclass | 20 ++++++++++++--------
>  scripts/start_vm           |  4 ++--
>  testsuite/start_vm.py      |  2 +-
>  4 files changed, 23 insertions(+), 11 deletions(-)
> 
> diff --git a/RECIPE-API-CHANGELOG.md b/RECIPE-API-CHANGELOG.md
> index e48c98c7..1e8dbfc8 100644
> --- a/RECIPE-API-CHANGELOG.md
> +++ b/RECIPE-API-CHANGELOG.md
> @@ -476,3 +476,11 @@ Bitbake 2.0 for better performance. It also
> requires isar-sstate script to be
>  migrated to zstd.
>  Mixing old Gzip-based and new ZStandatd-based sstate cache is not
> recommended
>  and should be avoid for correct compatibility.
> +
> +### Working with a custom initramfs
> +
> +The existing `INITRD_IMAGE` variable is defaulted to the empty
> string and used to
> +control if a custom initrd is requrested. Only if this variable is
> empty, the
> +default one is deployed. By that, the variable cannot be used to get
> the name of
> +the images initramfs. Instead, the variable `INITRD_DEPLOY_FILE` is
> provided which
> +always povides the name of the initrd file (also when the default
> one is used).
> diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
> index 6277069f..7b3551b0 100644
> --- a/meta/classes/image.bbclass
> +++ b/meta/classes/image.bbclass
> @@ -23,7 +23,8 @@ IMAGE_FULLNAME = "${PN}-${DISTRO}-${MACHINE}"
>  
>  # These variables are used by wic and start_vm
>  KERNEL_IMAGE ?= "${IMAGE_FULLNAME}-${KERNEL_FILE}"
> -INITRD_IMAGE ?= "${IMAGE_FULLNAME}-initrd.img"
> +INITRD_IMAGE ?= ""
> +INITRD_DEPLOY_FILE = "${@ d.getVar('INITRD_IMAGE', True) or
> '${IMAGE_FULLNAME}-initrd.img'}"
>  
>  # This defines the deployed dtbs for reuse by imagers
>  DTB_FILES ?= ""
> @@ -353,7 +354,7 @@ EOF
>  
>  # Default kernel, initrd and dtb image deploy paths (inside imager)
>  KERNEL_IMG = "${PP_DEPLOY}/${KERNEL_IMAGE}"
> -INITRD_IMG = "${PP_DEPLOY}/${INITRD_IMAGE}"
> +INITRD_IMG = "${PP_DEPLOY}/${INITRD_DEPLOY_FILE}"
>  # only one dtb file supported, pick the first
>  DTB_IMG = "${PP_DEPLOY}/${@(d.getVar('DTB_FILES').split() or
> [''])[0]}"
>  
> @@ -370,12 +371,15 @@ do_copy_boot_files() {
>          sudo cat "$kernel" > "${DEPLOYDIR}/${KERNEL_IMAGE}"
>      fi
>  
> -    initrd="$(realpath -q '${IMAGE_ROOTFS}/initrd.img')"
> -    if [ ! -f "$initrd" ]; then
> -        initrd="$(realpath -q '${IMAGE_ROOTFS}/boot/initrd.img')"
> -    fi
> -    if [ -f "$initrd" ]; then
> -        cp -f "$initrd" '${DEPLOYDIR}/${INITRD_IMAGE}'
> +    if [ -e "${INITRD_IMAGE}" ]; then

This check is broken. Instead it has to be -z "${INITRD_IMAGE}".
Will send a v2.

Felix

> +        # deploy default initrd if no custom one is build
> +        initrd="$(realpath -q '${IMAGE_ROOTFS}/initrd.img')"
> +        if [ ! -f "$initrd" ]; then
> +            initrd="$(realpath -q
> '${IMAGE_ROOTFS}/boot/initrd.img')"
> +        fi
> +        if [ -f "$initrd" ]; then
> +            cp -f "$initrd" '${DEPLOYDIR}/${INITRD_DEPLOY_FILE}'
> +        fi
>      fi
>  
>      for file in ${DTB_FILES}; do
> diff --git a/scripts/start_vm b/scripts/start_vm
> index 17091d72..8c696a4a 100755
> --- a/scripts/start_vm
> +++ b/scripts/start_vm
> @@ -125,10 +125,10 @@ case "$IMAGE_FSTYPES" in
>      readonly ROOTFS_IMAGE=$IMAGE_FULLNAME.ext4
>  
>      eval $(bitbake -e mc:qemu$ARCH-$DISTRO:isar-image-base | grep
> "^KERNEL_IMAGE=")
> -    eval $(bitbake -e mc:qemu$ARCH-$DISTRO:isar-image-base | grep
> "^INITRD_IMAGE=")
> +    eval $(bitbake -e mc:qemu$ARCH-$DISTRO:isar-image-base | grep
> "^INITRD_DEPLOY_FILE=")
>      QKERNEL=$IMAGE_DIR/${KERNEL_IMAGE}
>      QINITRD=/dev/null
> -    [ -n "$INITRD_IMAGE" ] && QINITRD=$IMAGE_DIR/${INITRD_IMAGE}
> +    [ -n "$INITRD_DEPLOY_FILE" ] &&
> QINITRD=$IMAGE_DIR/${INITRD_DEPLOY_FILE}
>      if [ "$ARCH" = "riscv64" ]; then
>          EXTRA_ARGS="$EXTRA_ARGS -device
> loader,file=$QKERNEL,addr=0x80200000"
>          QKERNEL="/usr/lib/riscv64-linux-
> gnu/opensbi/qemu/virt/fw_jump.elf"
> diff --git a/testsuite/start_vm.py b/testsuite/start_vm.py
> index 82ecc17d..ba1ba127 100755
> --- a/testsuite/start_vm.py
> +++ b/testsuite/start_vm.py
> @@ -35,7 +35,7 @@ def format_qemu_cmdline(arch, build, distro, out,
> pid, enforce_pcbios=False):
>      if image_type == 'ext4':
>          rootfs_image = 'isar-image-base-' + base + '-' + distro + '-
> qemu' + arch + '.ext4'
>          kernel_image = deploy_dir_image + '/' +
> get_bitbake_var(bb_output, 'KERNEL_IMAGE')
> -        initrd_image = get_bitbake_var(bb_output, 'INITRD_IMAGE')
> +        initrd_image = get_bitbake_var(bb_output,
> 'INITRD_DEPLOY_FILE')
>  
>          if not initrd_image:
>              initrd_image = '/dev/null'

Re: [PATCH v2 3/3] fix race-cond between default and custom initrd

[Uladzimir Bely]

In the email from Thursday, 23 February 2023 06:36:00 +03 user Moessbauer, Felix wrote:
> On Wed, 2023-02-22 at 20:40 +0300, Uladzimir Bely wrote:
> > In the email from Saturday, 18 February 2023 13:30:38 +03 user Felix
> > Moessbauer wrote:
> > > This patch fixes a data race happening when building a custom
> > > initrd.
> > > Previously, both custom and default initrds were deployed to the
> > > image
> > > deploy dir. The race is fixed by conditionally deploying either the
> > > custom or the default one. For that, we introduce a new variable
> > > INITRD_DEPLOY_FILE which provides the name of the initrd in the
> > > deploy
> > > directory. The existing INITRD_IMAGE variable is defaulted to the
> > > empty
> > > string and used to control if a custom initrd is requrested. Only
> > > if
> > > this variable is empty, the default one is deployed.
> > > 
> > > Signed-off-by: Felix Moessbauer <felix.moessbauer@siemens.com>
> > > ---
> > >  RECIPE-API-CHANGELOG.md    |  8 ++++++++
> > >  meta/classes/image.bbclass | 20 ++++++++++++--------
> > >  scripts/start_vm           |  4 ++--
> > >  testsuite/start_vm.py      |  2 +-
> > >  4 files changed, 23 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/RECIPE-API-CHANGELOG.md b/RECIPE-API-CHANGELOG.md
> > > index e48c98c7..1e8dbfc8 100644
> > > --- a/RECIPE-API-CHANGELOG.md
> > > +++ b/RECIPE-API-CHANGELOG.md
> > > @@ -476,3 +476,11 @@ Bitbake 2.0 for better performance. It also
> > > requires isar-sstate script to be
> > >  migrated to zstd.
> > >  Mixing old Gzip-based and new ZStandatd-based sstate cache is not
> > > recommended
> > >  and should be avoid for correct compatibility.
> > > +
> > > +### Working with a custom initramfs
> > > +
> > > +The existing `INITRD_IMAGE` variable is defaulted to the empty
> > > string and used to
> > > +control if a custom initrd is requrested. Only if this variable is
> > > empty, the
> > > +default one is deployed. By that, the variable cannot be used to
> > > get the name of
> > > +the images initramfs. Instead, the variable `INITRD_DEPLOY_FILE`
> > > is provided which
> > > +always povides the name of the initrd file (also when the default
> > > one is used).
> > > diff --git a/meta/classes/image.bbclass
> > > b/meta/classes/image.bbclass
> > > index 6277069f..7b3551b0 100644
> > > --- a/meta/classes/image.bbclass
> > > +++ b/meta/classes/image.bbclass
> > > @@ -23,7 +23,8 @@ IMAGE_FULLNAME = "${PN}-${DISTRO}-${MACHINE}"
> > >  
> > >  # These variables are used by wic and start_vm
> > >  KERNEL_IMAGE ?= "${IMAGE_FULLNAME}-${KERNEL_FILE}"
> > > -INITRD_IMAGE ?= "${IMAGE_FULLNAME}-initrd.img"
> > > +INITRD_IMAGE ?= ""
> > > +INITRD_DEPLOY_FILE = "${@ d.getVar('INITRD_IMAGE', True) or
> > > '${IMAGE_FULLNAME}-initrd.img'}"
> > >  
> > >  # This defines the deployed dtbs for reuse by imagers
> > >  DTB_FILES ?= ""
> > > @@ -353,7 +354,7 @@ EOF
> > >  
> > >  # Default kernel, initrd and dtb image deploy paths (inside
> > > imager)
> > >  KERNEL_IMG = "${PP_DEPLOY}/${KERNEL_IMAGE}"
> > > -INITRD_IMG = "${PP_DEPLOY}/${INITRD_IMAGE}"
> > > +INITRD_IMG = "${PP_DEPLOY}/${INITRD_DEPLOY_FILE}"
> > >  # only one dtb file supported, pick the first
> > >  DTB_IMG = "${PP_DEPLOY}/${@(d.getVar('DTB_FILES').split() or
> > > [''])[0]}"
> > >  
> > > @@ -370,12 +371,15 @@ do_copy_boot_files() {
> > >          sudo cat "$kernel" > "${DEPLOYDIR}/${KERNEL_IMAGE}"
> > >      fi
> > >  
> > > -    initrd="$(realpath -q '${IMAGE_ROOTFS}/initrd.img')"
> > > -    if [ ! -f "$initrd" ]; then
> > > -        initrd="$(realpath -q '${IMAGE_ROOTFS}/boot/initrd.img')"
> > > -    fi
> > > -    if [ -f "$initrd" ]; then
> > > -        cp -f "$initrd" '${DEPLOYDIR}/${INITRD_IMAGE}'
> > > +    if [ -e "${INITRD_IMAGE}" ]; then
> > > +        # deploy default initrd if no custom one is build
> > > +        initrd="$(realpath -q '${IMAGE_ROOTFS}/initrd.img')"
> > > +        if [ ! -f "$initrd" ]; then
> > > +            initrd="$(realpath -q
> > > '${IMAGE_ROOTFS}/boot/initrd.img')"
> > > +        fi
> > > +        if [ -f "$initrd" ]; then
> > > +            cp -f "$initrd" '${DEPLOYDIR}/${INITRD_DEPLOY_FILE}'
> > > +        fi
> > >      fi
> > >  
> > >      for file in ${DTB_FILES}; do
> > > diff --git a/scripts/start_vm b/scripts/start_vm
> > > index 17091d72..8c696a4a 100755
> > > --- a/scripts/start_vm
> > > +++ b/scripts/start_vm
> > > @@ -125,10 +125,10 @@ case "$IMAGE_FSTYPES" in
> > >      readonly ROOTFS_IMAGE=$IMAGE_FULLNAME.ext4
> > >  
> > >      eval $(bitbake -e mc:qemu$ARCH-$DISTRO:isar-image-base | grep
> > > "^KERNEL_IMAGE=")
> > > -    eval $(bitbake -e mc:qemu$ARCH-$DISTRO:isar-image-base | grep
> > > "^INITRD_IMAGE=")
> > > +    eval $(bitbake -e mc:qemu$ARCH-$DISTRO:isar-image-base | grep
> > > "^INITRD_DEPLOY_FILE=")
> > >      QKERNEL=$IMAGE_DIR/${KERNEL_IMAGE}
> > >      QINITRD=/dev/null
> > > -    [ -n "$INITRD_IMAGE" ] && QINITRD=$IMAGE_DIR/${INITRD_IMAGE}
> > > +    [ -n "$INITRD_DEPLOY_FILE" ] &&
> > > QINITRD=$IMAGE_DIR/${INITRD_DEPLOY_FILE}
> > >      if [ "$ARCH" = "riscv64" ]; then
> > >          EXTRA_ARGS="$EXTRA_ARGS -device
> > > loader,file=$QKERNEL,addr=0x80200000"
> > >          QKERNEL="/usr/lib/riscv64-linux-
> > > gnu/opensbi/qemu/virt/fw_jump.elf"
> > > diff --git a/testsuite/start_vm.py b/testsuite/start_vm.py
> > > index 82ecc17d..ba1ba127 100755
> > > --- a/testsuite/start_vm.py
> > > +++ b/testsuite/start_vm.py
> > > @@ -35,7 +35,7 @@ def format_qemu_cmdline(arch, build, distro, out,
> > > pid, enforce_pcbios=False):
> > >      if image_type == 'ext4':
> > >          rootfs_image = 'isar-image-base-' + base + '-' + distro +
> > > '-qemu' + arch + '.ext4'
> > >          kernel_image = deploy_dir_image + '/' +
> > > get_bitbake_var(bb_output, 'KERNEL_IMAGE')
> > > -        initrd_image = get_bitbake_var(bb_output, 'INITRD_IMAGE')
> > > +        initrd_image = get_bitbake_var(bb_output,
> > > 'INITRD_DEPLOY_FILE')
> > 
> > This seems to be not enough to change the name without directory,
> > since it's later concatenated with old one (deploy_dir_image)
> 
> Why would that make a difference? The new INITRD_DEPLOY_FILE variable
> should behave exactly the same as the previous INITRD_IMAGE, when set.
> If not, then this is a bug in my patch.
> 
> > 
> > >  
> > >          if not initrd_image:
> > >              initrd_image = '/dev/null'
> > > 
> >         else:
> >             initrd_image = deploy_dir_image + '/' + initrd_image`
> > 
> > Moreover, I would say, it's not good that we should now look for
> > initrd file somewhere in DEPLOYDIR="${WORKDIR}/deploy", while
> > everyone would expect to find it
> > DEPLOY_DIR_IMAGE="${DEPLOY_DIR}/images/${MACHINE}"...
> 
> This should also not be the case. The deploy location should be exactly
> the same as before. In my local tests this also always was the case.
> I'll have a second look.
> 

I've rechecked with `bitbake -e mc:qemuarm-bullseye:isar-image-base`.

Before patchset, these lines in `do_copy_boot_files`
```
if [ -f "$initrd" ]; then
    cp -f "$initrd" '${DEPLOY_DIR_IMAGE}/${INITRD_IMAGE}'
fi
```
are converted to:
```
if [ -f "$initrd" ]; then
    cp -f "$initrd" '/build/tmp/deploy/images/qemuarm/isar-image-base-debian-bullseye-qemuarm-initrd.img'
fi
```

while with the patchset we have a new transition from
```
if [ -f "$initrd" ]; then
    cp -f "$initrd" '${DEPLOYDIR}/${INITRD_DEPLOY_FILE}'
fi
```
to
```
if [ -f "$initrd" ]; then
    cp -f "$initrd" '/build/tmp/work/debian-bullseye-armhf/isar-image-base-qemuarm/1.0-r0/deploy/isar-image-base-debian-bullseye-qemuarm-initrd.img'
fi
```

So, the issue caused not by ${INITRD_DEPLOY_FILE}, but by ${DEPLOYDIR}

> With all these changes to the testsuite I have to admit that I simply
> did not run it. I lost track on how to use it after the changes.
> 

Yes, there have been many changes in testsuite recently (and more are upcoming), but related part in `start_vm.py / format_qemu_cmdline` is "stable" for years.

> I will have a second look at the patch series.
> 
> Felix
> 
> > 
> > 
> > 
> > 
> 
> 

Re: [PATCH v2 3/3] fix race-cond between default and custom initrd

[Moessbauer, Felix]

On Thu, 2023-02-23 at 08:57 +0300, Uladzimir Bely wrote:
> In the email from Thursday, 23 February 2023 06:36:00 +03 user
> Moessbauer, Felix wrote:
> > On Wed, 2023-02-22 at 20:40 +0300, Uladzimir Bely wrote:
> > > In the email from Saturday, 18 February 2023 13:30:38 +03 user
> > > Felix
> > > Moessbauer wrote:
> > > > This patch fixes a data race happening when building a custom
> > > > initrd.
> > > > Previously, both custom and default initrds were deployed to
> > > > the
> > > > image
> > > > deploy dir. The race is fixed by conditionally deploying either
> > > > the
> > > > custom or the default one. For that, we introduce a new
> > > > variable
> > > > INITRD_DEPLOY_FILE which provides the name of the initrd in the
> > > > deploy
> > > > directory. The existing INITRD_IMAGE variable is defaulted to
> > > > the
> > > > empty
> > > > string and used to control if a custom initrd is requrested.
> > > > Only
> > > > if
> > > > this variable is empty, the default one is deployed.
> > > > 
> > > > Signed-off-by: Felix Moessbauer <felix.moessbauer@siemens.com>
> > > > ---
> > > >  RECIPE-API-CHANGELOG.md    |  8 ++++++++
> > > >  meta/classes/image.bbclass | 20 ++++++++++++--------
> > > >  scripts/start_vm           |  4 ++--
> > > >  testsuite/start_vm.py      |  2 +-
> > > >  4 files changed, 23 insertions(+), 11 deletions(-)
> > > > 
> > > > diff --git a/RECIPE-API-CHANGELOG.md b/RECIPE-API-CHANGELOG.md
> > > > index e48c98c7..1e8dbfc8 100644
> > > > --- a/RECIPE-API-CHANGELOG.md
> > > > +++ b/RECIPE-API-CHANGELOG.md
> > > > @@ -476,3 +476,11 @@ Bitbake 2.0 for better performance. It
> > > > also
> > > > requires isar-sstate script to be
> > > >  migrated to zstd.
> > > >  Mixing old Gzip-based and new ZStandatd-based sstate cache is
> > > > not
> > > > recommended
> > > >  and should be avoid for correct compatibility.
> > > > +
> > > > +### Working with a custom initramfs
> > > > +
> > > > +The existing `INITRD_IMAGE` variable is defaulted to the empty
> > > > string and used to
> > > > +control if a custom initrd is requrested. Only if this
> > > > variable is
> > > > empty, the
> > > > +default one is deployed. By that, the variable cannot be used
> > > > to
> > > > get the name of
> > > > +the images initramfs. Instead, the variable
> > > > `INITRD_DEPLOY_FILE`
> > > > is provided which
> > > > +always povides the name of the initrd file (also when the
> > > > default
> > > > one is used).
> > > > diff --git a/meta/classes/image.bbclass
> > > > b/meta/classes/image.bbclass
> > > > index 6277069f..7b3551b0 100644
> > > > --- a/meta/classes/image.bbclass
> > > > +++ b/meta/classes/image.bbclass
> > > > @@ -23,7 +23,8 @@ IMAGE_FULLNAME = "${PN}-${DISTRO}-${MACHINE}"
> > > >  
> > > >  # These variables are used by wic and start_vm
> > > >  KERNEL_IMAGE ?= "${IMAGE_FULLNAME}-${KERNEL_FILE}"
> > > > -INITRD_IMAGE ?= "${IMAGE_FULLNAME}-initrd.img"
> > > > +INITRD_IMAGE ?= ""
> > > > +INITRD_DEPLOY_FILE = "${@ d.getVar('INITRD_IMAGE', True) or
> > > > '${IMAGE_FULLNAME}-initrd.img'}"
> > > >  
> > > >  # This defines the deployed dtbs for reuse by imagers
> > > >  DTB_FILES ?= ""
> > > > @@ -353,7 +354,7 @@ EOF
> > > >  
> > > >  # Default kernel, initrd and dtb image deploy paths (inside
> > > > imager)
> > > >  KERNEL_IMG = "${PP_DEPLOY}/${KERNEL_IMAGE}"
> > > > -INITRD_IMG = "${PP_DEPLOY}/${INITRD_IMAGE}"
> > > > +INITRD_IMG = "${PP_DEPLOY}/${INITRD_DEPLOY_FILE}"
> > > >  # only one dtb file supported, pick the first
> > > >  DTB_IMG = "${PP_DEPLOY}/${@(d.getVar('DTB_FILES').split() or
> > > > [''])[0]}"
> > > >  
> > > > @@ -370,12 +371,15 @@ do_copy_boot_files() {
> > > >          sudo cat "$kernel" > "${DEPLOYDIR}/${KERNEL_IMAGE}"
> > > >      fi
> > > >  
> > > > -    initrd="$(realpath -q '${IMAGE_ROOTFS}/initrd.img')"
> > > > -    if [ ! -f "$initrd" ]; then
> > > > -        initrd="$(realpath -q
> > > > '${IMAGE_ROOTFS}/boot/initrd.img')"
> > > > -    fi
> > > > -    if [ -f "$initrd" ]; then
> > > > -        cp -f "$initrd" '${DEPLOYDIR}/${INITRD_IMAGE}'
> > > > +    if [ -e "${INITRD_IMAGE}" ]; then
> > > > +        # deploy default initrd if no custom one is build
> > > > +        initrd="$(realpath -q '${IMAGE_ROOTFS}/initrd.img')"
> > > > +        if [ ! -f "$initrd" ]; then
> > > > +            initrd="$(realpath -q
> > > > '${IMAGE_ROOTFS}/boot/initrd.img')"
> > > > +        fi
> > > > +        if [ -f "$initrd" ]; then
> > > > +            cp -f "$initrd"
> > > > '${DEPLOYDIR}/${INITRD_DEPLOY_FILE}'
> > > > +        fi
> > > >      fi
> > > >  
> > > >      for file in ${DTB_FILES}; do
> > > > diff --git a/scripts/start_vm b/scripts/start_vm
> > > > index 17091d72..8c696a4a 100755
> > > > --- a/scripts/start_vm
> > > > +++ b/scripts/start_vm
> > > > @@ -125,10 +125,10 @@ case "$IMAGE_FSTYPES" in
> > > >      readonly ROOTFS_IMAGE=$IMAGE_FULLNAME.ext4
> > > >  
> > > >      eval $(bitbake -e mc:qemu$ARCH-$DISTRO:isar-image-base |
> > > > grep
> > > > "^KERNEL_IMAGE=")
> > > > -    eval $(bitbake -e mc:qemu$ARCH-$DISTRO:isar-image-base |
> > > > grep
> > > > "^INITRD_IMAGE=")
> > > > +    eval $(bitbake -e mc:qemu$ARCH-$DISTRO:isar-image-base |
> > > > grep
> > > > "^INITRD_DEPLOY_FILE=")
> > > >      QKERNEL=$IMAGE_DIR/${KERNEL_IMAGE}
> > > >      QINITRD=/dev/null
> > > > -    [ -n "$INITRD_IMAGE" ] &&
> > > > QINITRD=$IMAGE_DIR/${INITRD_IMAGE}
> > > > +    [ -n "$INITRD_DEPLOY_FILE" ] &&
> > > > QINITRD=$IMAGE_DIR/${INITRD_DEPLOY_FILE}
> > > >      if [ "$ARCH" = "riscv64" ]; then
> > > >          EXTRA_ARGS="$EXTRA_ARGS -device
> > > > loader,file=$QKERNEL,addr=0x80200000"
> > > >          QKERNEL="/usr/lib/riscv64-linux-
> > > > gnu/opensbi/qemu/virt/fw_jump.elf"
> > > > diff --git a/testsuite/start_vm.py b/testsuite/start_vm.py
> > > > index 82ecc17d..ba1ba127 100755
> > > > --- a/testsuite/start_vm.py
> > > > +++ b/testsuite/start_vm.py
> > > > @@ -35,7 +35,7 @@ def format_qemu_cmdline(arch, build, distro,
> > > > out,
> > > > pid, enforce_pcbios=False):
> > > >      if image_type == 'ext4':
> > > >          rootfs_image = 'isar-image-base-' + base + '-' +
> > > > distro +
> > > > '-qemu' + arch + '.ext4'
> > > >          kernel_image = deploy_dir_image + '/' +
> > > > get_bitbake_var(bb_output, 'KERNEL_IMAGE')
> > > > -        initrd_image = get_bitbake_var(bb_output,
> > > > 'INITRD_IMAGE')
> > > > +        initrd_image = get_bitbake_var(bb_output,
> > > > 'INITRD_DEPLOY_FILE')
> > > 
> > > This seems to be not enough to change the name without directory,
> > > since it's later concatenated with old one (deploy_dir_image)
> > 
> > Why would that make a difference? The new INITRD_DEPLOY_FILE
> > variable
> > should behave exactly the same as the previous INITRD_IMAGE, when
> > set.
> > If not, then this is a bug in my patch.
> > 
> > > 
> > > >  
> > > >          if not initrd_image:
> > > >              initrd_image = '/dev/null'
> > > > 
> > >         else:
> > >             initrd_image = deploy_dir_image + '/' + initrd_image`
> > > 
> > > Moreover, I would say, it's not good that we should now look for
> > > initrd file somewhere in DEPLOYDIR="${WORKDIR}/deploy", while
> > > everyone would expect to find it
> > > DEPLOY_DIR_IMAGE="${DEPLOY_DIR}/images/${MACHINE}"...
> > 
> > This should also not be the case. The deploy location should be
> > exactly
> > the same as before. In my local tests this also always was the
> > case.
> > I'll have a second look.
> > 
> 
> I've rechecked with `bitbake -e mc:qemuarm-bullseye:isar-image-base`.
> 
> Before patchset, these lines in `do_copy_boot_files`
> ```
> if [ -f "$initrd" ]; then
>     cp -f "$initrd" '${DEPLOY_DIR_IMAGE}/${INITRD_IMAGE}'
> fi
> ```
> are converted to:
> ```
> if [ -f "$initrd" ]; then
>     cp -f "$initrd" '/build/tmp/deploy/images/qemuarm/isar-image-
> base-debian-bullseye-qemuarm-initrd.img'
> fi
> ```
> 
> while with the patchset we have a new transition from
> ```
> if [ -f "$initrd" ]; then
>     cp -f "$initrd" '${DEPLOYDIR}/${INITRD_DEPLOY_FILE}'
> fi
> ```
> to
> ```
> if [ -f "$initrd" ]; then
>     cp -f "$initrd" '/build/tmp/work/debian-bullseye-armhf/isar-
> image-base-qemuarm/1.0-r0/deploy/isar-image-base-debian-bullseye-
> qemuarm-initrd.img'
> fi
> ```
> 
> So, the issue caused not by ${INITRD_DEPLOY_FILE}, but by
> ${DEPLOYDIR}

While this might look like a change, we still deploy to
${DEPLOY_DIR_IMAGE} as previously. The only difference is that we now
deploy via the sstate-cache, that means we deploy to DEPLOYDIR and
sstate copies these then to ${DEPLOY_DIR_IMAGE}. From an external point
of view, there is no difference at all.

For details, see p2, mainly:
+do_copy_boot_files[cleandirs] += "${DEPLOYDIR}"
+do_copy_boot_files[sstate-inputdirs] = "${DEPLOYDIR}"
+do_copy_boot_files[sstate-outputdirs] = "${DEPLOY_DIR_IMAGE}"

Best regards,
Felix

> 
> > With all these changes to the testsuite I have to admit that I
> > simply
> > did not run it. I lost track on how to use it after the changes.
> > 
> 
> Yes, there have been many changes in testsuite recently (and more are
> upcoming), but related part in `start_vm.py / format_qemu_cmdline` is
> "stable" for years.
> 
> > I will have a second look at the patch series.
> > 
> > Felix
> > 
> > > 
> > > 
> > > 
> > > 
> > 
> > 
> 
> 
> 
>