| Message ID | 20220405182906.57588-1-abongwabonalais@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Added double quotes aound variables in bitbake/source to prevent gobbling or splitting | expand |
Hi, Thanks for the patch, I have a question below. On Tue, 2022-04-05 at 19:29 +0100, Abongwa Amahnui Bonalais wrote: > Signed-off-by: Abongwa Bonalais Amahnui <abongwabonalais@gmail.com> > --- > bitbake/bin/toaster | 65 +++++++++++++++++++++++---------------------- > 1 file changed, 33 insertions(+), 32 deletions(-) > > diff --git a/bitbake/bin/toaster b/bitbake/bin/toaster > index 558a819570..d98c4c69f0 100755 > --- a/bitbake/bin/toaster > +++ b/bitbake/bin/toaster > @@ -1,4 +1,5 @@ > -#!/bin/echo ERROR: This script needs to be sourced. Please run as . > +#!/bin/sh > +echo ERROR: This script needs to be sourced. Please run as . > > # toaster - shell script to start Toaster > Changing /bin/echo to /bin/sh probably isn't intentional? We do intend the script to be sourced rather than run directly. How did you test the changes? Cheers, Richard
I tested the changes by running shellcheck toaster. I did so the first time and it returned "^-- SC1008: This shebang was unrecognized. ShellCheck only supports sh/bash/dash/ksh. Add a 'shell' directive to specify. ^-- SC2096: On most OS, shebangs can only specify a single parameter. " as shown in the attachment below.
[ Since you seem new to the process of reviewing via mail, please note that I have added comments throughout the entire patch below. ] > -----Original Message----- > From: bitbake-devel@lists.openembedded.org <bitbake- > devel@lists.openembedded.org> On Behalf Of Abongwa Amahnui Bonalais > Sent: den 5 april 2022 20:29 > To: bitbake-devel@lists.openembedded.org > Cc: Abongwa Bonalais Amahnui <abongwabonalais@gmail.com> > Subject: [bitbake-devel] [PATCH] Added double quotes aound variables in bitbake/source to prevent gobbling or splitting Always prefix the subject line with the affected recipe name or similar. Also keep the subject short, typically shorter than 72 characters. A better subject in this case would be: toaster: Add quoting of variables to avoid splitting Then there should be a description that gives a motivation for why this is needed and any other relevant information. > > Signed-off-by: Abongwa Bonalais Amahnui <abongwabonalais@gmail.com> > --- > bitbake/bin/toaster | 65 +++++++++++++++++++++++---------------------- > 1 file changed, 33 insertions(+), 32 deletions(-) > > diff --git a/bitbake/bin/toaster b/bitbake/bin/toaster > index 558a819570..d98c4c69f0 100755 > --- a/bitbake/bin/toaster > +++ b/bitbake/bin/toaster > @@ -1,4 +1,5 @@ > -#!/bin/echo ERROR: This script needs to be sourced. Please run as . > +#!/bin/sh > +echo ERROR: This script needs to be sourced. Please run as . Leave as it was. It is explicitly _not_ using #!/bin/sh as shebang line to catch misuse. > > # toaster - shell script to start Toaster > > @@ -20,8 +21,8 @@ Usage 2: source toaster manage [createsuperuser|lsupdates|migrate|makemigrations > custom_extention() > { > > custom_extension=$BBBASEDIR/lib/toaster/orm/fixtures/custom_toaster_append.sh > - if [ -f $custom_extension ] ; then > - $custom_extension $* > + if [ -f "$custom_extension" ] ; then > + $custom_extension "$*" > fi > } > > @@ -51,19 +52,19 @@ databaseCheck() > webserverKillAll() > { > local pidfile > - if [ -f ${BUILDDIR}/.toastermain.pid ] ; then > + if [ -f "${BUILDDIR}"/.toastermain.pid ] ; then While it is technically correct, please put the quotes around the entire string to make it more readable, i.e.: if [ -f "${BUILDDIR}/.toastermain.pid" ] ; then This applies to the rest of the changes in this file. > custom_extention web_stop_postpend > else > custom_extention noweb_stop_postpend > fi > - for pidfile in ${BUILDDIR}/.toastermain.pid ${BUILDDIR}/.runbuilds.pid; do > - if [ -f ${pidfile} ]; then > - pid=`cat ${pidfile}` > - while kill -0 $pid 2>/dev/null; do > - kill -SIGTERM $pid 2>/dev/null > + for pidfile in "${BUILDDIR}"/.toastermain.pid "${BUILDDIR}"/.runbuilds.pid; do > + if [ -f "${pidfile}" ]; then > + pid=`cat "${pidfile}"` > + while kill -0 "$pid" 2>/dev/null; do > + kill -SIGTERM "$pid" 2>/dev/null > sleep 1 > done > - rm ${pidfile} > + rm "${pidfile}" > fi > done > } > @@ -84,8 +85,8 @@ webserverStartAll() > echo "Starting webserver..." > > $MANAGE runserver --noreload "$ADDR_PORT" \ > - </dev/null >>${BUILDDIR}/toaster_web.log 2>&1 \ > - & echo $! >${BUILDDIR}/.toastermain.pid > + </dev/null >>"${BUILDDIR}"/toaster_web.log 2>&1 \ > + & echo "$!" >"${BUILDDIR}"/.toastermain.pid > > sleep 1 > > @@ -95,7 +96,7 @@ webserverStartAll() > else > echo "Toaster development webserver started at http://$ADDR_PORT" > echo -e "\nYou can now run 'bitbake <target>' on the command line and monitor your build in Toaster.\nYou can also use a Toaster project to configure and run a build.\n" > - custom_extention web_start_postpend $ADDR_PORT > + custom_extention web_start_postpend "$ADDR_PORT" > fi > > return $retval > @@ -131,8 +132,8 @@ verify_prereq() { > exp=$exp'vmax=["%02d" % int(n) for n in "\4".split(".")];' > exp=$exp'sys.exit(not (version \1 vmin and version \3 vmax))' > exp=$exp'/p' > - if ! sed -n "$exp" $reqfile | python3 - ; then > - req=`grep ^Django $reqfile` > + if ! sed -n "$exp" "$reqfile" | python3 - ; then > + req=`grep ^Django "$reqfile"` > echo "This program needs $req" > echo "Please install with pip3 install -r $reqfile" > return 2 > @@ -150,10 +151,10 @@ else > TOASTER=$0 > fi > > -export BBBASEDIR=`dirname $TOASTER`/.. > +export BBBASEDIR=`dirname "$TOASTER"`/.. > MANAGE="python3 $BBBASEDIR/lib/toaster/manage.py" > if [ -z "$OE_ROOT" ]; then > - OE_ROOT=`dirname $TOASTER`/../.. > + OE_ROOT=`dirname "$TOASTER"`/../.. > fi > > # this is the configuraton file we are using for toaster > @@ -164,7 +165,7 @@ fi > # in the local layers that currently make using an arbitrary > # toasterconf.json difficult. > > -. $OE_ROOT/.templateconf > +. "$OE_ROOT"/.templateconf > if [ -n "$TEMPLATECONF" ]; then > if [ ! -d "$TEMPLATECONF" ]; then > # Allow TEMPLATECONF=meta-xyz/conf as a shortcut > @@ -199,10 +200,10 @@ for param in $*; do > webport=*) > ADDR_PORT="${param#*=}" > # Split the addr:port string > - ADDR=`echo $ADDR_PORT | cut -f 1 -d ':'` > - PORT=`echo $ADDR_PORT | cut -f 2 -d ':'` > + ADDR=`echo "$ADDR_PORT" | cut -f 1 -d ':'` > + PORT=`echo "$ADDR_PORT" | cut -f 2 -d ':'` > # If only a port has been speified then set address to localhost. > - if [ $ADDR = $PORT ] ; then > + if [ "$ADDR" = "$PORT" ] ; then > ADDR_PORT="localhost:$PORT" > fi > ;; > @@ -229,7 +230,7 @@ for param in $*; do > esac > done > > -if [ `basename \"$0\"` = `basename \"${TOASTER}\"` ]; then > +if [ `basename \""$0"\"` = `basename \""${TOASTER}"\"` ]; then That is wrong (as the original code was wrong). Change it to: if [ `basename "$0"` = `basename "${TOASTER}"` ]; then > echo "Error: This script needs to be sourced. Please run as . $TOASTER" > return 1 > fi > @@ -265,7 +266,7 @@ fi > echo "The system will $CMD." > > # Execute the commands > -custom_extention toaster_prepend $CMD $ADDR_PORT > +custom_extention toaster_prepend "$CMD" "$ADDR_PORT" > > case $CMD in > start ) > @@ -279,7 +280,7 @@ case $CMD in > # Create configuration file > conf=${BUILDDIR}/conf/local.conf > line='INHERIT+="toaster buildhistory"' > - grep -q "$line" $conf || echo $line >> $conf > + grep -q "$line" "$conf" || echo "$line" >> "$conf" > > if [ $WEBSERVER -eq 0 ] ; then > # Do not update the database for "noweb" unless > @@ -290,17 +291,17 @@ case $CMD in > return 4 > fi > fi > - custom_extention noweb_start_postpend $ADDR_PORT > + custom_extention noweb_start_postpend "$ADDR_PORT" > fi > - if [ $WEBSERVER -gt 0 ] && ! webserverStartAll; then > + if [ "$WEBSERVER" -gt 0 ] && ! webserverStartAll; then > echo "Failed ${CMD}." > return 4 > fi > export BITBAKE_UI='toasterui' > if [ $TOASTER_BUILDSERVER -eq 1 ] ; then > $MANAGE runbuilds \ > - </dev/null >>${BUILDDIR}/toaster_runbuilds.log 2>&1 \ > - & echo $! >${BUILDDIR}/.runbuilds.pid > + </dev/null >>"${BUILDDIR}"/toaster_runbuilds.log 2>&1 \ > + & echo $! >"${BUILDDIR}"/.runbuilds.pid > else > echo "Toaster build server not started." > fi > @@ -308,7 +309,7 @@ case $CMD in > # set fail safe stop system on terminal exit > trap stop_system SIGHUP > echo "Successful ${CMD}." > - custom_extention toaster_postpend $CMD $ADDR_PORT > + custom_extention toaster_postpend "$CMD" "$ADDR_PORT" > return 0 > ;; > stop ) > @@ -316,9 +317,9 @@ case $CMD in > echo "Successful ${CMD}." > ;; > manage ) > - cd $BBBASEDIR/lib/toaster > - $MANAGE $manage_cmd > + cd "$BBBASEDIR"/lib/toaster > + "$MANAGE $manage_cmd" > ;; > esac > -custom_extention toaster_postpend $CMD $ADDR_PORT > +custom_extention toaster_postpend "$CMD" "$ADDR_PORT" > > -- > 2.25.1 //Peter
Hi Peter, Thanks alot for the review, I will do the changes and send a patch for it.
Hi Peter this is the link to the new patch I created. https://lists.openembedded.org/g/bitbake-devel/message/13591
diff --git a/bitbake/bin/toaster b/bitbake/bin/toaster index 558a819570..d98c4c69f0 100755 --- a/bitbake/bin/toaster +++ b/bitbake/bin/toaster @@ -1,4 +1,5 @@ -#!/bin/echo ERROR: This script needs to be sourced. Please run as . +#!/bin/sh +echo ERROR: This script needs to be sourced. Please run as . # toaster - shell script to start Toaster @@ -20,8 +21,8 @@ Usage 2: source toaster manage [createsuperuser|lsupdates|migrate|makemigrations custom_extention() { custom_extension=$BBBASEDIR/lib/toaster/orm/fixtures/custom_toaster_append.sh - if [ -f $custom_extension ] ; then - $custom_extension $* + if [ -f "$custom_extension" ] ; then + $custom_extension "$*" fi } @@ -51,19 +52,19 @@ databaseCheck() webserverKillAll() { local pidfile - if [ -f ${BUILDDIR}/.toastermain.pid ] ; then + if [ -f "${BUILDDIR}"/.toastermain.pid ] ; then custom_extention web_stop_postpend else custom_extention noweb_stop_postpend fi - for pidfile in ${BUILDDIR}/.toastermain.pid ${BUILDDIR}/.runbuilds.pid; do - if [ -f ${pidfile} ]; then - pid=`cat ${pidfile}` - while kill -0 $pid 2>/dev/null; do - kill -SIGTERM $pid 2>/dev/null + for pidfile in "${BUILDDIR}"/.toastermain.pid "${BUILDDIR}"/.runbuilds.pid; do + if [ -f "${pidfile}" ]; then + pid=`cat "${pidfile}"` + while kill -0 "$pid" 2>/dev/null; do + kill -SIGTERM "$pid" 2>/dev/null sleep 1 done - rm ${pidfile} + rm "${pidfile}" fi done } @@ -84,8 +85,8 @@ webserverStartAll() echo "Starting webserver..." $MANAGE runserver --noreload "$ADDR_PORT" \ - </dev/null >>${BUILDDIR}/toaster_web.log 2>&1 \ - & echo $! >${BUILDDIR}/.toastermain.pid + </dev/null >>"${BUILDDIR}"/toaster_web.log 2>&1 \ + & echo "$!" >"${BUILDDIR}"/.toastermain.pid sleep 1 @@ -95,7 +96,7 @@ webserverStartAll() else echo "Toaster development webserver started at http://$ADDR_PORT" echo -e "\nYou can now run 'bitbake <target>' on the command line and monitor your build in Toaster.\nYou can also use a Toaster project to configure and run a build.\n" - custom_extention web_start_postpend $ADDR_PORT + custom_extention web_start_postpend "$ADDR_PORT" fi return $retval @@ -131,8 +132,8 @@ verify_prereq() { exp=$exp'vmax=["%02d" % int(n) for n in "\4".split(".")];' exp=$exp'sys.exit(not (version \1 vmin and version \3 vmax))' exp=$exp'/p' - if ! sed -n "$exp" $reqfile | python3 - ; then - req=`grep ^Django $reqfile` + if ! sed -n "$exp" "$reqfile" | python3 - ; then + req=`grep ^Django "$reqfile"` echo "This program needs $req" echo "Please install with pip3 install -r $reqfile" return 2 @@ -150,10 +151,10 @@ else TOASTER=$0 fi -export BBBASEDIR=`dirname $TOASTER`/.. +export BBBASEDIR=`dirname "$TOASTER"`/.. MANAGE="python3 $BBBASEDIR/lib/toaster/manage.py" if [ -z "$OE_ROOT" ]; then - OE_ROOT=`dirname $TOASTER`/../.. + OE_ROOT=`dirname "$TOASTER"`/../.. fi # this is the configuraton file we are using for toaster @@ -164,7 +165,7 @@ fi # in the local layers that currently make using an arbitrary # toasterconf.json difficult. -. $OE_ROOT/.templateconf +. "$OE_ROOT"/.templateconf if [ -n "$TEMPLATECONF" ]; then if [ ! -d "$TEMPLATECONF" ]; then # Allow TEMPLATECONF=meta-xyz/conf as a shortcut @@ -199,10 +200,10 @@ for param in $*; do webport=*) ADDR_PORT="${param#*=}" # Split the addr:port string - ADDR=`echo $ADDR_PORT | cut -f 1 -d ':'` - PORT=`echo $ADDR_PORT | cut -f 2 -d ':'` + ADDR=`echo "$ADDR_PORT" | cut -f 1 -d ':'` + PORT=`echo "$ADDR_PORT" | cut -f 2 -d ':'` # If only a port has been speified then set address to localhost. - if [ $ADDR = $PORT ] ; then + if [ "$ADDR" = "$PORT" ] ; then ADDR_PORT="localhost:$PORT" fi ;; @@ -229,7 +230,7 @@ for param in $*; do esac done -if [ `basename \"$0\"` = `basename \"${TOASTER}\"` ]; then +if [ `basename \""$0"\"` = `basename \""${TOASTER}"\"` ]; then echo "Error: This script needs to be sourced. Please run as . $TOASTER" return 1 fi @@ -265,7 +266,7 @@ fi echo "The system will $CMD." # Execute the commands -custom_extention toaster_prepend $CMD $ADDR_PORT +custom_extention toaster_prepend "$CMD" "$ADDR_PORT" case $CMD in start ) @@ -279,7 +280,7 @@ case $CMD in # Create configuration file conf=${BUILDDIR}/conf/local.conf line='INHERIT+="toaster buildhistory"' - grep -q "$line" $conf || echo $line >> $conf + grep -q "$line" "$conf" || echo "$line" >> "$conf" if [ $WEBSERVER -eq 0 ] ; then # Do not update the database for "noweb" unless @@ -290,17 +291,17 @@ case $CMD in return 4 fi fi - custom_extention noweb_start_postpend $ADDR_PORT + custom_extention noweb_start_postpend "$ADDR_PORT" fi - if [ $WEBSERVER -gt 0 ] && ! webserverStartAll; then + if [ "$WEBSERVER" -gt 0 ] && ! webserverStartAll; then echo "Failed ${CMD}." return 4 fi export BITBAKE_UI='toasterui' if [ $TOASTER_BUILDSERVER -eq 1 ] ; then $MANAGE runbuilds \ - </dev/null >>${BUILDDIR}/toaster_runbuilds.log 2>&1 \ - & echo $! >${BUILDDIR}/.runbuilds.pid + </dev/null >>"${BUILDDIR}"/toaster_runbuilds.log 2>&1 \ + & echo $! >"${BUILDDIR}"/.runbuilds.pid else echo "Toaster build server not started." fi @@ -308,7 +309,7 @@ case $CMD in # set fail safe stop system on terminal exit trap stop_system SIGHUP echo "Successful ${CMD}." - custom_extention toaster_postpend $CMD $ADDR_PORT + custom_extention toaster_postpend "$CMD" "$ADDR_PORT" return 0 ;; stop ) @@ -316,9 +317,9 @@ case $CMD in echo "Successful ${CMD}." ;; manage ) - cd $BBBASEDIR/lib/toaster - $MANAGE $manage_cmd + cd "$BBBASEDIR"/lib/toaster + "$MANAGE $manage_cmd" ;; esac -custom_extention toaster_postpend $CMD $ADDR_PORT +custom_extention toaster_postpend "$CMD" "$ADDR_PORT"
Signed-off-by: Abongwa Bonalais Amahnui <abongwabonalais@gmail.com> --- bitbake/bin/toaster | 65 +++++++++++++++++++++++---------------------- 1 file changed, 33 insertions(+), 32 deletions(-)