| Message ID | 20250606071915.2680182-2-jamin_lin@aspeedtech.com |
|---|---|
| State | New |
| Headers | show |
| Series | uboot-sign: Make SPL DTB public key injection optional | expand |
On Fri, 2025-06-06 at 15:19 +0800, Jamin Lin via lists.openembedded.org wrote: > Fix potential errors during do_deploy and deploy_spl_dtb when > SPL_DTB_BINARY or > SPL_BINARY is not present. > > Wrapped install command in deploy_spl_dtb() with a file existence > check. > Improved condition in do_deploy to check for actual existence of > ${SPL_DIR}/${SPL_DTB_BINARY} instead of only relying on variable non- > emptiness. > Prevents "install: missing destination file operand" and invalid > symlink > creation. The commit message describes the code changes, but lacks the crucial context of why these changes are necessary. Could you please add some background information explaining the motivation or problem this change is solving? What's the use case where the code should proceed without finding the ${SPL_BINARY} file? This would help future maintainers better understand the rationale behind this modification. > > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com> > --- > meta/classes-recipe/uboot-sign.bbclass | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/meta/classes-recipe/uboot-sign.bbclass b/meta/classes- > recipe/uboot-sign.bbclass > index 73e9ce3f11..6ee1874cd6 100644 > --- a/meta/classes-recipe/uboot-sign.bbclass > +++ b/meta/classes-recipe/uboot-sign.bbclass > @@ -275,7 +275,9 @@ deploy_spl_dtb() { > fi > > # For backwards compatibility... > - install -Dm644 ${SPL_BINARY} ${DEPLOYDIR}/${SPL_IMAGE} > + if [ -e "${SPL_BINARY}" ]; then If possible, these kinds of checks should rely solely on BitBake variables. Checking for files in the filesystem is very fragile. There are many scenarios where a file might not be in its expected location due to a bug. When this happens, debugging becomes much harder if the code silently continues running instead of failing with a clear error message about the missing file. While file-based checks might allow code to continue running with wrong assumptions, variable-based checks will fail immediately with clear error conditions, making debugging more straightforward and issues more apparent. I know there are similar examples in the existing code. > + install -Dm644 ${SPL_BINARY} > ${DEPLOYDIR}/${SPL_IMAGE} > + fi > } > > do_uboot_generate_rsa_keys() { > @@ -600,7 +602,7 @@ do_deploy:prepend() { > ln -sf ${UBOOT_FITIMAGE_IMAGE} > ${DEPLOYDIR}/${UBOOT_FITIMAGE_SYMLINK} > fi > > - if [ "${SPL_SIGN_ENABLE}" = "1" -a -n "${SPL_DTB_BINARY}" ] > ; then > + if [ "${SPL_SIGN_ENABLE}" = "1" -a -e > "${SPL_DIR}/${SPL_DTB_BINARY}" ] ; then Same as above. Adrian > ln -sf ${SPL_DTB_IMAGE} > ${DEPLOYDIR}/${SPL_DTB_SYMLINK} > ln -sf ${SPL_DTB_IMAGE} > ${DEPLOYDIR}/${SPL_DTB_BINARY} > ln -sf ${SPL_NODTB_IMAGE} > ${DEPLOYDIR}/${SPL_NODTB_SYMLINK} > > -=-=-=-=-=-=-=-=-=-=-=- > Links: You receive all messages sent to this group. > View/Reply Online (#218113): > https://lists.openembedded.org/g/openembedded-core/message/218113 > Mute This Topic: https://lists.openembedded.org/mt/113499584/3616858 > Group Owner: openembedded-core+owner@lists.openembedded.org > Unsubscribe: > https://lists.openembedded.org/g/openembedded-core/unsub [ > adrian.freihofer@siemens.com] > -=-=-=-=-=-=-=-=-=-=-=- >
Hi Adrian, > Subject: Re: [OE-core] [PATCH v1 1/2] uboot-sign: Avoid symlink and install > errors when SPL DTB is missing > > On Fri, 2025-06-06 at 15:19 +0800, Jamin Lin via lists.openembedded.org > wrote: > > Fix potential errors during do_deploy and deploy_spl_dtb when > > SPL_DTB_BINARY or SPL_BINARY is not present. > > > > Wrapped install command in deploy_spl_dtb() with a file existence > > check. > > Improved condition in do_deploy to check for actual existence of > > ${SPL_DIR}/${SPL_DTB_BINARY} instead of only relying on variable non- > > emptiness. > > Prevents "install: missing destination file operand" and invalid > > symlink creation. > > The commit message describes the code changes, but lacks the crucial context > of why these changes are necessary. Could you please add some background > information explaining the motivation or problem this change is solving? > What's the use case where the code should proceed without finding the > ${SPL_BINARY} file? > This would help future maintainers better understand the rationale behind this > modification. > Thanks for suggestion. How about the following commit messages? Commit Message: Fix potential errors during do_deploy and deploy_spl_dtb when SPL_DTB_BINARY or SPL_BINARY is not present. Wrapped the install command in deploy_spl_dtb() with a file existence check. Improved the condition in do_deploy to check for actual existence of ${SPL_DIR}/${SPL_DTB_BINARY} instead of only relying on variable non-emptiness. This prevents issues such as "install: missing destination file operand" and creation of invalid symbolic links. Motivation and Use Case In certain scenarios, users may want to sign only the U-Boot FIT image, without using SPL. In such cases, SPL_SIGN_ENABLE should be set to 1 to enable generation of a FIT ITS file with signature nodes. However, when only U-Boot is built (no SPL), the following problems can occur: 1. SPL_BINARY is never set. The following line fails because ${SPL_BINARY} is empty ``` install -Dm644 ${SPL_BINARY} ${DEPLOYDIR}/${SPL_IMAGE} ``` This results in a build error. 2. This condition: will evaluate to true, because the default value of SPL_DTB_BINARY is u-boot-spl.dtb. But in a U-Boot-only build, this file does not exist, which leads to creation invalid symlinks. ``` if [ "${SPL_SIGN_ENABLE}" = "1" -a -n "${SPL_DTB_BINARY}" ] ``` Fix Summary The updated logic now: Skips the install step unless ${SPL_BINARY} exists. Avoids creating symlinks unless ${SPL_DTB_BINARY} actually exists on disk. This ensures that U-Boot-only builds with signature support do not fail due to missing SPL artifacts. Thanks-Jamin > > > > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com> > > --- > > meta/classes-recipe/uboot-sign.bbclass | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/meta/classes-recipe/uboot-sign.bbclass b/meta/classes- > > recipe/uboot-sign.bbclass index 73e9ce3f11..6ee1874cd6 100644 > > --- a/meta/classes-recipe/uboot-sign.bbclass > > +++ b/meta/classes-recipe/uboot-sign.bbclass > > @@ -275,7 +275,9 @@ deploy_spl_dtb() { > > fi > > > > # For backwards compatibility... > > - install -Dm644 ${SPL_BINARY} ${DEPLOYDIR}/${SPL_IMAGE} > > + if [ -e "${SPL_BINARY}" ]; then > If possible, these kinds of checks should rely solely on BitBake variables. > Checking for files in the filesystem is very fragile. There are many scenarios > where a file might not be in its expected location due to a bug. When this > happens, debugging becomes much harder if the code silently continues > running instead of failing with a clear error message about the missing file. > While file-based checks might allow code to continue running with wrong > assumptions, variable-based checks will fail immediately with clear error > conditions, making debugging more straightforward and issues more apparent. > I know there are similar examples in the existing code. > > > + install -Dm644 ${SPL_BINARY} > > ${DEPLOYDIR}/${SPL_IMAGE} > > + fi > > } > > > > do_uboot_generate_rsa_keys() { > > @@ -600,7 +602,7 @@ do_deploy:prepend() { > > ln -sf ${UBOOT_FITIMAGE_IMAGE} > > ${DEPLOYDIR}/${UBOOT_FITIMAGE_SYMLINK} > > fi > > > > - if [ "${SPL_SIGN_ENABLE}" = "1" -a -n "${SPL_DTB_BINARY}" ] > > ; then > > + if [ "${SPL_SIGN_ENABLE}" = "1" -a -e > > "${SPL_DIR}/${SPL_DTB_BINARY}" ] ; then > Same as above. > > Adrian > > > ln -sf ${SPL_DTB_IMAGE} > > ${DEPLOYDIR}/${SPL_DTB_SYMLINK} > > ln -sf ${SPL_DTB_IMAGE} > > ${DEPLOYDIR}/${SPL_DTB_BINARY} > > ln -sf ${SPL_NODTB_IMAGE} > > ${DEPLOYDIR}/${SPL_NODTB_SYMLINK} > > > > -=-=-=-=-=-=-=-=-=-=-=- > > Links: You receive all messages sent to this group. > > View/Reply Online (#218113): > > https://lists.openembedded.org/g/openembedded-core/message/218113 > > Mute This Topic: https://lists.openembedded.org/mt/113499584/3616858 > > Group Owner: openembedded-core+owner@lists.openembedded.org > > Unsubscribe: > > https://lists.openembedded.org/g/openembedded-core/unsub [ > > adrian.freihofer@siemens.com] > > -=-=-=-=-=-=-=-=-=-=-=- > >
diff --git a/meta/classes-recipe/uboot-sign.bbclass b/meta/classes-recipe/uboot-sign.bbclass index 73e9ce3f11..6ee1874cd6 100644 --- a/meta/classes-recipe/uboot-sign.bbclass +++ b/meta/classes-recipe/uboot-sign.bbclass @@ -275,7 +275,9 @@ deploy_spl_dtb() { fi # For backwards compatibility... - install -Dm644 ${SPL_BINARY} ${DEPLOYDIR}/${SPL_IMAGE} + if [ -e "${SPL_BINARY}" ]; then + install -Dm644 ${SPL_BINARY} ${DEPLOYDIR}/${SPL_IMAGE} + fi } do_uboot_generate_rsa_keys() { @@ -600,7 +602,7 @@ do_deploy:prepend() { ln -sf ${UBOOT_FITIMAGE_IMAGE} ${DEPLOYDIR}/${UBOOT_FITIMAGE_SYMLINK} fi - if [ "${SPL_SIGN_ENABLE}" = "1" -a -n "${SPL_DTB_BINARY}" ] ; then + if [ "${SPL_SIGN_ENABLE}" = "1" -a -e "${SPL_DIR}/${SPL_DTB_BINARY}" ] ; then ln -sf ${SPL_DTB_IMAGE} ${DEPLOYDIR}/${SPL_DTB_SYMLINK} ln -sf ${SPL_DTB_IMAGE} ${DEPLOYDIR}/${SPL_DTB_BINARY} ln -sf ${SPL_NODTB_IMAGE} ${DEPLOYDIR}/${SPL_NODTB_SYMLINK}
Fix potential errors during do_deploy and deploy_spl_dtb when SPL_DTB_BINARY or SPL_BINARY is not present. Wrapped install command in deploy_spl_dtb() with a file existence check. Improved condition in do_deploy to check for actual existence of ${SPL_DIR}/${SPL_DTB_BINARY} instead of only relying on variable non-emptiness. Prevents "install: missing destination file operand" and invalid symlink creation. Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com> --- meta/classes-recipe/uboot-sign.bbclass | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)