Re: [Buildroot] [PATCH v2 2/3] board/qemu/start-qemu.sh.in: rework argument handling

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

James, All,

On 2023-04-07 01:21 -0400, James Knight spake thusly:
> Provides the ability to forward command line options directly to QEMU.
> When invoking `start-qemu.sh`, users can forward arguments by adding a
> double dash (`--`) into the argument set, and any trailing arguments
> will be forwarded into QEMU. For example, `start-qemu.sh -- --help`.
> 
> The original implementation supported a "serial-only" command line
> argument to help run in a non-graphical mode for some use cases. These
> changes try to promote a newly added `--serial-only` argument to drive
> this mode; that being said, a `serial-only` argument will still be
> accepted for backwards compatibility.
> 
> Since this script is primarily for CI-related testing, a warning is
> generated for users if any unsupported options are provided.
> 
> Signed-off-by: James Knight <james.d.knight@live.com>
> ---
> Changes v1 -> v2:
>   - From the original patch, this second patch now only contains changes
>     related to argument processing.
>   - Add unsupported warning with custom options (suggested by Romain)
>   - Ensure sh compliant in the argument parsing (while condition).
>   - Properly handle legacy `serial-only` check in argument processing by
>     not cycling just on dash-prefixed arguments.
> ---
>  board/qemu/start-qemu.sh.in | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/board/qemu/start-qemu.sh.in b/board/qemu/start-qemu.sh.in
> index c2d77734c7a6b318a5f7adedfd9b0b5875e84f59..dd387507f8867778ec73f49d769679dcd0956fd4 100644
> --- a/board/qemu/start-qemu.sh.in
> +++ b/board/qemu/start-qemu.sh.in
> @@ -3,12 +3,25 @@
>  BINARIES_DIR="${0%/*}/"
>  cd ${BINARIES_DIR}
>  
> -if [ "${1}" = "serial-only" ]; then
> +mode_serial=0
> +while [ "$1" ]; do
> +    case "$1" in
> +    --serial-only|serial-only) mode_serial=1; shift;;

I like to use false/true for booleans, first because this is exactly
what mode_serial is, and second because we can directly call them to
test truthness...

> +    --) shift; break;;
> +    *) echo "unknown option: $1" 1>&2; exit 1;;
> +    esac
> +done
> +
> +if [ $mode_serial -eq 1 ]; then

... here, as:

    if ${mode_serial}; then
        ...

>      EXTRA_ARGS='VAR_SERIAL_ARGS'
>  else
>      EXTRA_ARGS='VAR_DEFAULT_ARGS'
>  fi
>  
> +if [ "$*" ]; then
> +    echo "(warning) unsupported options: $*"
> +fi

This warning is not going to be usefull, is it? If the user does pass
extra variables for qemu, $* will not be empty, and this is expected.

OTOH, if there were options the script did not recognise for itself, it
would have already errored out becasue of 'exit 1' earlier.

So, I dropped the warning.

Applied to master, thanks.

Regards,
Yann E. MORIN.

>  export PATH="${HOST_DIR}/bin:${PATH}"
> -exec VAR_QEMU_CMD_LINE ${EXTRA_ARGS}
> +exec VAR_QEMU_CMD_LINE ${EXTRA_ARGS} "$@"
>  )
> -- 
> 2.39.1.windows.1
> 
> _______________________________________________
> 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