[v2,2/2] semihosting: add --semihosting-config arg sub-argument

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.

Signed-off-by: Leon Alrae <leon.alrae@imgtec.com>
---
 include/exec/semihost.h | 12 ++++++++++++
 qemu-options.hx         |  8 +++++---
 vl.c                    | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 50 insertions(+), 3 deletions(-)

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

On 07/05/2015 14:02, Peter Maydell wrote:
>> +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().

glib pointer array certainly simplifies managing an array of pointers,
but I think in this case we wouldn't benefit much from it as the array
is set here only and never modified later. Also people not familiar with
glib pointer arrays may find raw int argc and const char **argv fields
clearer than GPtrArray type.
I'll leave it as it is, but if anyone has strong preference on using
g_ptr_array I don't mind updating the patch as changes are trivial.

Thanks,
Leon