On 7 May 2015 at 12:49, Leon Alrae <leon.al...@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
