Re: [cbootimage PATCH v1] Add more features

[Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>]

On 04/08/2016 08:11 PM, Jimmy Zhang wrote:
> 1. use parameter <soc> to specify boot image type. ie, tegra124, tegra210
> 2. Along signing bootimage, also generate signed bct, ie, tegra124.bct,
>     tegra210.bct. User should use this signed bct when flashing target.
>
> Example:
>
>     $ ./sign.sh tegra124 t124.img rsa_priv.pem

> diff --git a/samples/sign.sh b/samples/sign.sh

> -# Copyright (c) 2015, NVIDIA CORPORATION.  All rights reserved.
> +# Copyright (c) 2016, NVIDIA CORPORATION.  All rights reserved.

Copyright years should be added to the list, not replace the list. See 
our internal copyright wiki. Put another way, "2015-2016" not "2016".

> +usage ()
> +{
> +	echo -e "
> +Usage: ./sign.sh <soc> <boot_image> <rsa_priv_key>
> +  Where,
> +	soc: tegra124, tegra210
> +	boot_image: image generated by cbootimage,
> +	priv_key: rsa key file in .pem format."
> +
> +	exit 1;
> +}

Interesting; I don't think I've ever seen multi-line text passed to 
echo. I guess if it works, then it's fine. I think the following syntax 
is more common though:

cat <<EOF
Usage: ...
...
EOF

> +sign_image ()
> +{
> +	local bct_length=$(($3 + $4));

Rather than using $1..$4, can you do this:

something=$1
other=$2
...

and then use ${something} and ${other} etc. throughout. That will make 
the code a bit more readable, as well as document the function signature 
a little.

> +soc=$1		# tegra124, tegra210
> +
> +if [[ "${soc}" == tegra124 ]]; then
> +	bl_block_offset=16384;  # emmc: 16384, spi_flash: 32768: default: emmc
> +	bct_signed_offset=1712;
> +	bct_signed_length=6480;
> +elif [ "${soc}" = tegra210 ]; then
> +	bl_block_offset=32768;  # emmc: 16384, spi_flash: 32768: default: spi
> +	bct_signed_offset=1296;
> +	bct_signed_length=8944;
> +elif [[ "${soc}" != tegra124 && \
> +        "${soc}" != tegra210 ]]; then

You know that's not true given the if/elif conditions. Why not just use 
"else" here?

> +echo "Sign ${soc} ${IMAGE_FILE} with key ${KEY_FILE}"
> +sign_image "$soc" "$bl_block_offset" "$bct_signed_offset" "$bct_signed_length"

This patch would be a bit easier to read if it didn't convert everything 
to a function at the same time, which introduces another indentation 
level and hence makes the entire script into a big diff. Is there a 
reason to convert everything to a function? I could understand this if 
the code were split up into a bunch of functions and they were called 
multiple times, but as far as I can tell that isn't the case here.

If you want to convert everything to a function, I suggest a separate 
patch for that (which is basically just a whitespace change) v.s. adding 
new functionality.