Hi Lars, On 09/30/16 23:50, Lars Kruse wrote: > Hi Jan-Tarek, > > first of all: I am very happy to see your patches!
Thanks :)
> 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!
I just do some small stuff :) Butt I plan to continiure work on it.
> 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.
Ah right, this kind are very special. Hm yes we can sed a comment on top.
>
>> 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.
Thanks I'll fix it.
>> 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?
bacause there just nummeric.
>> - 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?
That command is to check the result of echo $spec
> I would assume that:
> for pattern in $spec; do
> behaves exactly like:
> for pattern in $(eval echo $spec); do
are you shure ?
>
>> - 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.
Oh my mistake you are right.
>> 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.
Ah right I see the problem.
Thanks
Cheers
Tarek
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Lede-dev mailing list Lede-dev@lists.infradead.org http://lists.infradead.org/mailman/listinfo/lede-dev
