Hi Jan-Tarek,
first of all: I am very happy to see your patches!
Whenever I took a look at some shell scripts in openwrt, I felt the strong urge
to change many of the things you went out to fix. But until now I was always
stopped by the sheer amount of changes necessary. Thank you!
Am Fri, 30 Sep 2016 22:02:15 +0200
schrieb Jan-Tarek Butt <ta...@ring0.de>:
> [..]
> diff --git a/scripts/arm-magic.sh b/scripts/arm-magic.sh
> index 29ec88a..61ba098 100755
> --- a/scripts/arm-magic.sh
> +++ b/scripts/arm-magic.sh
> @@ -24,19 +24,19 @@
> # list of supported boards, in "boardname machtypeid" format
> for board in "avila 526" "gateway7001 731" "nslu2 597" "nas100d 865"
> "wg302v1 889" "wg302v2 890" "pronghorn 928" "pronghornmetro 1040" "compex
> 1273" "wrt300nv2 1077" "loft 849" "dsmg600 964" "fsg3 1091" "ap1000 1543"
> "tw2662 1658" "tw5334 1664" "ixdpg425 604" "cambria 1468" "sidewinder 1041"
> "ap42x 4418" do
> - set -- $board
> - hexid=$(printf %x\\n $2)
> + set -- "$board"
> + hexid=$(printf %x\\n "$2")
I think, exactly here the missing quotes were left out on purpose.
The "set -- ..." line was supposed to assign the string pairs above (e.g.
"avila 526" to $1 and $2. With your change both are squashed into $1 instead.
I do not see a good way use quoting here, thus I would recommend to add a
comment above explaining this (from my point of view: very exotic) "set"
statement.
> if [ "$2" -lt "256" ]; then
> # we have a low machtypeid, we just need a "mov" (e3a)
> - printf "\xe3\xa0\x10\x$hexid" > $BIN_DIR/$IMG_PREFIX-$1-zImage
> + printf "\xe3\xa0\x10\x$hexid" > "$BIN_DIR"/"$IMG_PREFIX"-"$1"-zImage
I would prefer the following:
"$BIN_DIR/$IMG_PREFIX-$1-zImage"
over this:
"$BIN_DIR"/"$IMG_PREFIX"-"$1"-zImage
But this is surely just a question of taste.
> diff --git a/scripts/combined-ext-image.sh b/scripts/combined-ext-image.sh
> index 374fe6e..b752aae 100755
> --- a/scripts/combined-ext-image.sh
> +++ b/scripts/combined-ext-image.sh
> @@ -38,7 +38,7 @@ IMG_OUT=$1; shift
> FILE_NUM=$(($# / 2))
> FILES=""
>
> -printf "CE%02x%-32s%02x" $CE_VERSION "$IMG_TYPE" $FILE_NUM > $IMG_OUT
> +printf "CE%02x%-32s%02x" $CE_VERSION "$IMG_TYPE" $FILE_NUM > "$IMG_OUT"
Is there a reason for not adding quotes for CE_VERSION and FILE_NUM?
> - for pattern in $(eval echo $spec); do
> - find $libdirs -name "$pattern.so*" | sort -u
> + for pattern in $(eval echo "$spec"); do
> + find "$libdirs" -name "$pattern.so*" | sort -u
Just out of curiosity: do you know, what could be the purpose of the "eval"
construct above?
I would assume that:
for pattern in $spec; do
behaves exactly like:
for pattern in $(eval echo $spec); do
> - exec "$CC" $CFLAGS -dumpmachine
> + exec "$CC" "$CFLAGS" -dumpmachine
I think, that this change would squash all CFLAGS into a single parameter,
which should fail, I guess.
I cannot think of a way to add quotes here.
> diff --git a/scripts/strip-kmod.sh b/scripts/strip-kmod.sh
> index 313015b..ef35b82 100755
> --- a/scripts/strip-kmod.sh
> +++ b/scripts/strip-kmod.sh
> @@ -22,7 +22,7 @@ if [ -z "$KEEP_BUILD_ID" ]; then
> ARGS="$ARGS -R .note.gnu.build-id"
> fi
>
> -${CROSS}objcopy \
> +"${CROSS}"objcopy \
> -R .comment \
> -R .pdr \
> -R .mdebug.abi32 \
> @@ -30,7 +30,7 @@ ${CROSS}objcopy \
> -R .reginfo \
> -R .MIPS.abiflags \
> -R .note.GNU-stack \
> - $ARGS \
> + "$ARGS" \
Based on the name "ARGS" I could imagine, that this variable contains more than
one argument. Thus the quotes would create a problem, I think.
The rest of these changes looked fine to me.
Cheers,
Lars
_______________________________________________
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev
