Re: [Buildroot] [PATCH 3/3] package/ssdp-responder: fix warnings from check-package and shellcheck

["Yann E. MORIN" <yann.morin.1998@free.fr>]

Joachim, All,

On 2022-11-01 08:30 +0100, Joachim Wiberg spake thusly:
> On Mon, Oct 31, 2022 at 21:49, Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote:
> > On Mon, 31 Oct 2022 18:46:32 +0100
> > Joachim Wiberg <troglobit@gmail.com> wrote:
> >> +cmd()
> >> +{
> >> +	start-stop-daemon -q -p "$PIDFILE" -x "$DAEMON" "$@"
> >> +	status=$?
> >> +	[ $status -eq 0 ] && echo "OK" || echo "FAIL"
> >> +
> >> +	return $status
> >> +}
> > I don't think we're using this cmd construct anywhere else in the tree,
> > or did I miss some change in our coding style/policy?
> 
> I mentioned it in the cover letter, but that information should have
> been here in this patch.  Sorry about that.
> 
> It all started out with utils/check-package telling me I used $DAEMON
> wrong.  While changing that I ended up with a final comment from it
> that said I should also "run shellcheck and fix the warnings".
> 
> It in turn had several grievances which I took one by one.  In this one
> I used the same construct as in package/smcroute/S41smcroute to work
> around a warning about using `$?` instead of using an `if cmd; then ...`

We hanve cmd() in only two pacjages so far, smcroute and watchdogd, both
proided by you, so I can see you are aiming for some consistency! :-)

However, the majority of our SNNfoo startup scripts do not use this
cmd() wrapper construct, so I am not a fan of it.

(I like that it is generic and that we could have in a library of helpers
shared across our startup script, but IIRC we had a similar discussion a
long time ago, and decide against it, because it is trivial enough to
call start-stop-daemon).

The rest of the changes are however interesting, so could you respin
with just fixing those, pelase?

Note: it is acceptable that some shellcheck triggers get forcefully
disabled. For example, SC2086 (Double quote to prevent globbing and word
splitting) must be disabled when expanding $DAEMON_ARGS; this can be
achieved with:

    # shellcheck source=/dev/null
    [ -f $CFGFILE ] && . $CFGFILE

    # shellcheck disable=SC2086
    start-stop-daemon -S -q -p $PIDFILE -x $DAEMON -- $DAEMON_ARGS

Basically, the canonical reference for a startup script is
package/busybox/S01syslogd.

Oh, btw, as much as I hate TABs, we do use TABs for indentation...

https://nightly.buildroot.org/#adding-packages-start-script

Regards,
Yann E. MORIN.

> >>  start() {
> >> -	printf 'Starting %s: ' "$NAME"
> >> -	start-stop-daemon -S -q -p $PIDFILE -x $DAEMON -- $DAEMON_ARGS
> >> -	[ $? = 0 ] && echo "OK" || echo "FAIL"
> >
> > This all looked matching our coding style. Why are you changing this?
> >>  case "$1" in
> >> -    start|stop|restart)
> >> -	"$1"
> >> -	;;
> >> -    reload)
> >> -	restart
> >> -	;;
> >> -    *)
> >> -	echo "Usage: $0 {start|stop|restart|reload}"
> >> -	exit 1
> >> +	start|stop|restart)
> >> +		"$1"
> >> +		;;
> >> +	reload)
> >> +		restart
> >> +		;;
> >> +	*)
> >> +		echo "Usage: $0 {start|stop|restart|reload}"
> >> +		exit 1
> >
> > I'm not sure what is the recommended indentation style in our init
> > scripts. Tabs? Spaces?
> 
> Shellcheck pointed out this section had four spaces indent instead of
> the eight (tab) used for the rest of the script.  Iirc from the "own
> code coding style" discussions C and shell script should follow the
> same style, but I may very well be wrong here.
> 
> 
> Best regards
>  /Joachim
> 
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot