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

["Moessbauer, Felix" <felix.moessbauer@siemens.com>]

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'