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

From: Joachim Wiberg <troglobit@gmail.com>
To: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>, buildroot@buildroot.org
Subject: Re: [Buildroot] [PATCH 3/3] package/ssdp-responder: fix warnings from check-package and shellcheck
Date: Sun, 06 Nov 2022 09:54:43 +0100	[thread overview]
Message-ID: <868rkojwdo.fsf@gmail.com> (raw)
In-Reply-To: <20221105184455.GL3918838@scaer>

On Sat, Nov 05, 2022 at 19:44, "Yann E. MORIN" <yann.morin.1998@free.fr> wrote:
> 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()
>> >> +{
>> >> [SNIP]
>> >> +}
>> > 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).

OK, I get it.  I was sort of aiming for a rehash of the start() and
stop() helper functions that many other start scripts use.  It's a small
generalization step up from that to reduce duplication and thus avoid
annoying bugs in behavior between stop and start actions.  Anyway ...

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

Sure thing, I'll send a v2! :-)

> Note: it is acceptable that some shellcheck triggers get forcefully
> disabled.

OK, was a bit unsure about the policy for that. Thanks!

> 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

Ah, yes that makes sense.  Thank you!

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

Got it! :)

> Oh, btw, as much as I hate TABs, we do use TABs for indentation...
> https://nightly.buildroot.org/#adding-packages-start-script

Yup, I'll be migrating my scripts to this.  Thanks for the
clarification and the thorough review!

Best regards
 /Joachim

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot