Re: [PATCH] sim: common: modernize gennltvals.sh

Simon Marchi via Gdb-patches
2021-01-18T18:09:07Z

On 2021-01-18 12:52 p.m., Mike Frysinger wrote:
> On 18 Jan 2021 12:19, Simon Marchi via Gdb-patches wrote:
>> On 2021-01-18 12:13 p.m., Mike Frysinger wrote:
>>> i had run shellcheck and the only warnings (about unquoted expansion)
>>> i didn't think we worth fixing because of the limited scope of the
>>> script. the variable in question isn't accepting user input, it's
>>> operating on fixed inputs, and if we restrict ourselves to POSIX
>>> shell (which i think we do), then our options are limited, and imo
>>> the alternatives make it harder to read/understand.
>>
>> Even considering this, I think it's worth just quoting the variables
>> and getting rid the warnings. It's trivial, and it would make any
>> future (more important) warning more apparent.
>
> to be clear, it isn't a style issue, it's a correctness issue.
> adding the quotes will break the script.
>
> here's the warning:
> printf '#include <%s>\n' ${files}
> ^------^ SC2086: Double quote to prevent globbing and word splitting.
>
> this is because the code passes in multiple files to process as an arg:
> gentvals "" errno ... "errno.h sys/errno.h" ...
>
> then we use it like:
> files=$4
> ...
> for f in ${files}; do
> ...
> printf '#include <%s>\n' ${files}
>
> unquoted, we get:
> #include <errno.h>
> #include <sys/errno.h>
>
> quoted, we get:
> #include <errno.h sys/errno.h>

Wait, what. This printf call gives you multiple lines, and does not
tell you "you have too many arguments for the format string"? This
is... surprising.

To produce that output, I would have expected a for loop:

for f in ${files}; do
printf '#include <%s>\n' "$f"
done

>
> our options are limited with POSIX shell:
> * use arrays ... POSIX shell only has one builtin array: the args.
> so we'd rework the func API to pass in multiple files, and we'd
> operate on $@ by shifting it and iterating.

Ok, I'm used to using bash arrays, I forgot they weren't standard.

> * change the shell runtime env by disabling path expansion. that
> would mitigate the path expansion (which doesn't happen here as
> we know the inputs are all alphanumeric/periods), but still would
> have word splitting because we want that.
>
> shell is just a bad programming language. but i think the tree
> specifically constrains itself to it for portability. i don't know
> what, if any, policies we have about using any other language.
> -mike

I think it would be perfectly fine to require bash or python.

Simon