[PATCH] kernel: fitimage: Repair misuse of shell test command

[PATCH] kernel: fitimage: Repair misuse of shell test command

[Marek Vasut]

The kernel fitImage must be amended with signature if and only if
UBOOT_SIGN_ENABLE = 1 . In the current case, the UBOOT_SIGN_ENABLE
could be either 0 (default) or 1 , which test -n always correctly
interprets as non-empty string, thus always true. This does not
match the logic above though, so replace the test with check which
passes only for UBOOT_SIGN_ENABLE = 1 .

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Yannick Gicquel <yannick.gicquel@iot.bzh>
Cc: Richard Purdie <richard.purdie@linuxfoundation.org>
---
 meta/classes/kernel-fitimage.bbclass | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
---
NOTE: It'd be real nice if I was CCed on the original patches
NOTE: I'm not convinced that UBOOT_SIGN_ENABLE is the right name
      for this variable, since the signed object is really the
      fitImage and U-Boot only verifies the signature. Maybe we
      should rename it, is it still possible ?

diff --git a/meta/classes/kernel-fitimage.bbclass b/meta/classes/kernel-fitimage.bbclass
index 809bd4d..298eda2 100644
--- a/meta/classes/kernel-fitimage.bbclass
+++ b/meta/classes/kernel-fitimage.bbclass
@@ -250,7 +250,7 @@ do_assemble_fitimage() {
 		#
 		# Step 5: Sign the image and add public key to U-Boot dtb
 		#
-		if test -n "${UBOOT_SIGN_ENABLE}"; then
+		if [ "x${UBOOT_SIGN_ENABLE}" = "x1" ] ; then
 			uboot-mkimage \
 				${@'-D "${UBOOT_MKIMAGE_DTCOPTS}"' if len('${UBOOT_MKIMAGE_DTCOPTS}') else ''} \
 				-F -k "${UBOOT_SIGN_KEYDIR}" \
-- 
2.7.0

Re: [PATCH] kernel: fitimage: Repair misuse of shell test command

[Andreas Oberritter]

Hi Marek,

On 08.05.2016 13:21, Marek Vasut wrote:
> The kernel fitImage must be amended with signature if and only if
> UBOOT_SIGN_ENABLE = 1 . In the current case, the UBOOT_SIGN_ENABLE
> could be either 0 (default) or 1 , which test -n always correctly
> interprets as non-empty string, thus always true. This does not
> match the logic above though, so replace the test with check which
> passes only for UBOOT_SIGN_ENABLE = 1 .
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Yannick Gicquel <yannick.gicquel@iot.bzh>
> Cc: Richard Purdie <richard.purdie@linuxfoundation.org>
> ---
>  meta/classes/kernel-fitimage.bbclass | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> ---
> NOTE: It'd be real nice if I was CCed on the original patches
> NOTE: I'm not convinced that UBOOT_SIGN_ENABLE is the right name
>       for this variable, since the signed object is really the
>       fitImage and U-Boot only verifies the signature. Maybe we
>       should rename it, is it still possible ?
> 
> diff --git a/meta/classes/kernel-fitimage.bbclass b/meta/classes/kernel-fitimage.bbclass
> index 809bd4d..298eda2 100644
> --- a/meta/classes/kernel-fitimage.bbclass
> +++ b/meta/classes/kernel-fitimage.bbclass
> @@ -250,7 +250,7 @@ do_assemble_fitimage() {
>  		#
>  		# Step 5: Sign the image and add public key to U-Boot dtb
>  		#
> -		if test -n "${UBOOT_SIGN_ENABLE}"; then
> +		if [ "x${UBOOT_SIGN_ENABLE}" = "x1" ] ; then

just a nitpick about coding style, but I think you should remove the
extra x. Most people add it, because they have seen it somewhere else,
but it doesn't make much sense if the variable is both enclosed in
quotes and has very little likeliness to get set to a valid option for
'test' returning true.

Leaving the extra x out improves readability for shell novices and, in
general (but not in this case), the ability to grep using whole word
matches.

Regards,
Andreas