From mboxrd@z Thu Jan 1 00:00:00 1970 X-GM-THRID: 7201062888117633024 X-Received: by 2002:a05:6902:1183:b0:a27:3ecc:ffe7 with SMTP id m3-20020a056902118300b00a273eccffe7mr1044959ybu.3.1677131868832; Wed, 22 Feb 2023 21:57:48 -0800 (PST) X-BeenThere: isar-users@googlegroups.com Received: by 2002:a25:353:0:b0:95e:47b9:1bec with SMTP id 80-20020a250353000000b0095e47b91becls5107350ybd.1.-pod-prod-gmail; Wed, 22 Feb 2023 21:57:48 -0800 (PST) X-Google-Smtp-Source: AK7set/fQ2eg3XjW2i5JdPCGhbek9CTypVPu1O9VPqg5fs8+OwcrO7uyn1eJ6E6qUHV3ZoiRZfO9 X-Received: by 2002:a25:fb11:0:b0:a02:c2eb:65f1 with SMTP id j17-20020a25fb11000000b00a02c2eb65f1mr5362058ybe.50.1677131867987; Wed, 22 Feb 2023 21:57:47 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1677131867; cv=none; d=google.com; s=arc-20160816; b=Lt2V1tuSMNvWSZwXBFqbec4yjz63vAFtU+SHG1Z/nqbkvO87y6xJJKwg/uA6flqj7m nDu+QJZCa0+E/oqHdeI6jAsDdeSfI88gAqdhSLjp2dw3VVnxxndebh8BQbmlLPXNKsSt SAB2AYdw2NY/MfJKXO0GPLLytc3SHk76p9j2HXJj/jsxOvpePhgmumamkX/lkk3P8JUk 90CBCSF/K9OXJOseUHXDkZ6OIDo/89X7KWkRkGoAC4dt4IAJT37A0kmu6NV6PFzCaj0u fAO7tkt1Oik9hrPXScHg/ZYwScs/AqYmYyRoj//6U71s+MpYX93iKNHJ1qTHpP7cGheW MUgA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:to:from; bh=UgRaQu5OKXhFInhMA2kcaYWaQCJT8Ry1LbBr1KE3V60=; b=Lgf14R0JtrdfzkQdUhSB8+QQiQISxJomxhD8kPghPIOGrVe799ZragBJi7k4rVQCYG VLpiSm17axfJEUuaO+FLyYMytqZTKLgAxfEr9vBt9OPxKiQ/KXuB+R3lateCAwyK95W7 Pes0l0gNmMYJe8ssIUNFiD9q+TvIp6HRVJt1OYKeGWYo7mk33AaVfLjvOJAxm2iyw9lg /xOEOh98gdRCUwtowfDcj+jtYpncG4L1Rlv9MSNyKzXI7bAEsTeJPzm7vbq7Qtr0zdaH nR2CEQRDnuTHlnyh4OJWpKe8JVMWifa6X1Y+tbWN8YHN4EOPQBx1vC/lHnAeKK8nUZd+ hLZA== ARC-Authentication-Results: i=1; gmr-mx.google.com; spf=pass (google.com: domain of ubely@ilbers.de designates 85.214.156.166 as permitted sender) smtp.mailfrom=ubely@ilbers.de Return-Path: Received: from shymkent.ilbers.de (shymkent.ilbers.de. [85.214.156.166]) by gmr-mx.google.com with ESMTPS id k188-20020a2524c5000000b0090621221d35si825749ybk.2.2023.02.22.21.57.47 for (version=TLS1_2 cipher=ECDHE-ECDSA-CHACHA20-POLY1305 bits=256/256); Wed, 22 Feb 2023 21:57:47 -0800 (PST) Received-SPF: pass (google.com: domain of ubely@ilbers.de designates 85.214.156.166 as permitted sender) client-ip=85.214.156.166; Authentication-Results: gmr-mx.google.com; spf=pass (google.com: domain of ubely@ilbers.de designates 85.214.156.166 as permitted sender) smtp.mailfrom=ubely@ilbers.de Received: from home.localnet (44-208-124-178-static.mgts.by [178.124.208.44] (may be forged)) (authenticated bits=0) by shymkent.ilbers.de (8.15.2/8.15.2/Debian-8+deb9u1) with ESMTPSA id 31N5vjVM018973 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 23 Feb 2023 06:57:45 +0100 From: Uladzimir Bely To: "isar-users@googlegroups.com" , "Moessbauer, Felix" Subject: Re: [PATCH v2 3/3] fix race-cond between default and custom initrd Date: Thu, 23 Feb 2023 08:57:46 +0300 Message-ID: <3403305.sQuhbGJ8Bu@home> In-Reply-To: <94d2ed745950ee73ff97648e7caa9b0f9296bfed.camel@siemens.com> References: <20230218103038.3005105-1-felix.moessbauer@siemens.com> <5500062.ZASKD2KPVS@home> <94d2ed745950ee73ff97648e7caa9b0f9296bfed.camel@siemens.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Spam-Status: No, score=-1.0 required=5.0 tests=ALL_TRUSTED autolearn=unavailable autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on shymkent.ilbers.de X-TUID: MjqDaO1jSBce 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 > > > --- > > > 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 > > > > > > > > > > >