| Message ID | 20230218103038.3005105-4-felix.moessbauer@siemens.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Fix data-race in deployment of initrd | expand |
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}"...
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 > > > >
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'
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 > > > > > > > > > > >
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 > > > > > > > > > > > > > > > > > > > > > >
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'
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(-)