From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45437) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YqLRv-0005OK-Ag for qemu-devel@nongnu.org; Thu, 07 May 2015 09:02:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YqLRr-0008PY-6i for qemu-devel@nongnu.org; Thu, 07 May 2015 09:02:47 -0400 Received: from mail-ig0-f178.google.com ([209.85.213.178]:33978) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YqLRr-0008P5-3S for qemu-devel@nongnu.org; Thu, 07 May 2015 09:02:43 -0400 Received: by iget9 with SMTP id t9so10977523ige.1 for ; Thu, 07 May 2015 06:02:42 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1430999376-16601-3-git-send-email-leon.alrae@imgtec.com> References: <1430999376-16601-1-git-send-email-leon.alrae@imgtec.com> <1430999376-16601-3-git-send-email-leon.alrae@imgtec.com> From: Peter Maydell Date: Thu, 7 May 2015 14:02:22 +0100 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH v2 2/2] semihosting: add --semihosting-config arg sub-argument List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Leon Alrae Cc: Liviu Ionescu , Christopher Covington , QEMU Developers , Matthew Fortune On 7 May 2015 at 12:49, Leon Alrae 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