Re: [Qemu-devel] [PATCH v2 2/2] semihosting: add --semihosting-config arg sub-argument

[Peter Maydell <peter.maydell@linaro.org>]

On 7 May 2015 at 12:49, Leon Alrae <leon.alrae@imgtec.com> wrote:
> Add new "arg" sub-argument to the --semihosting-config allowing to pass
> multiple input argument separately. It is required for example by UHI
> semihosting to construct argc and argv.

> diff --git a/qemu-options.hx b/qemu-options.hx
> index ec356f6..ed2a7e8 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3296,14 +3296,16 @@ STEXI
>  Enable semihosting mode (ARM, M68K, Xtensa only).
>  ETEXI
>  DEF("semihosting-config", HAS_ARG, QEMU_OPTION_semihosting_config,
> -    "-semihosting-config [enable=on|off,]target=native|gdb|auto   semihosting configuration\n",
> +    "-semihosting-config [enable=on|off][,target=native|gdb|auto][,arg=str[,...]]\n" \
> +    "                semihosting configuration\n",
>  QEMU_ARCH_ARM | QEMU_ARCH_M68K | QEMU_ARCH_XTENSA | QEMU_ARCH_LM32)
>  STEXI
> -@item -semihosting-config [enable=on|off,]target=native|gdb|auto
> +@item -semihosting-config [enable=on|off][,target=native|gdb|auto][,arg=str[,...]]
>  @findex -semihosting-config
>  Enable semihosting and define where the semihosting calls will be addressed,
>  to QEMU (@code{native}) or to GDB (@code{gdb}). The default is @code{auto}, which means
> -@code{gdb} during debug sessions and @code{native} otherwise (ARM, M68K, Xtensa only).
> +@code{gdb} during debug sessions and @code{native} otherwise. The @var{arg} allows to
> +pass input arguments, can be used multiple times to build up a list (ARM, M68K, Xtensa only)

This makes it sound like the var{arg} bit is arm/m68k/xtensa only, whereas it's
the whole semihosting-config that that applies to. I'd suggest that now we
have more than one subargument to this we should rephrase it to:

 Enable and configure semihosting (ARM, M68K, Xtensa only).
 @var{target} defines where the semihosting calls will be addressed [etc]
 @var{arg} allows [etc]

PS: avoid "allows to", which isn't idiomatic English.

You also need to document how {arg} interacts with the old-style
-kernel/-append method of passing a command line to semihosting.

>  ETEXI
>  DEF("old-param", 0, QEMU_OPTION_old_param,
>      "-old-param      old param mode\n", QEMU_ARCH_ARM)
> diff --git a/vl.c b/vl.c
> index f3319a9..c8ac1e6 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -486,6 +486,9 @@ static QemuOptsList qemu_semihosting_config_opts = {
>          }, {
>              .name = "target",
>              .type = QEMU_OPT_STRING,
> +        }, {
> +            .name = "arg",
> +            .type = QEMU_OPT_STRING,
>          },
>          { /* end of list */ }
>      },
> @@ -1230,6 +1233,8 @@ static void configure_msg(QemuOpts *opts)
>  typedef struct SemihostingConfig {
>      bool enabled;
>      SemihostingTarget target;
> +    const char **argv;
> +    int argc;
>  } SemihostingConfig;
>
>  static SemihostingConfig semihosting;
> @@ -1244,6 +1249,31 @@ SemihostingTarget semihosting_get_target(void)
>      return semihosting.target;
>  }
>
> +const char *semihosting_get_arg(int i)
> +{
> +    if (i >= semihosting.argc) {
> +        return NULL;
> +    }
> +
> +    return semihosting.argv[i];
> +}
> +
> +int semihosting_get_argc(void)
> +{
> +    return semihosting.argc;
> +}
> +
> +static int add_semihosting_arg(const char *name, const char *val, void *opaque)
> +{
> +    SemihostingConfig *s = opaque;
> +    if (strcmp(name, "arg") == 0) {
> +        s->argc++;
> +        s->argv = g_realloc(s->argv, s->argc * sizeof(void *));
> +        s->argv[s->argc - 1] = val;
> +    }

Consider using a glib pointer array? Then this is just
a call to g_pointer_array_add().

(If not, then I agree that this is entirely fine and it's
more self-contained and maintainable to just realloc here
than to add code to the option-parsing first-pass loop.)

thanks
-- PMM