[Qemu-devel] [PATCH v2 0/2] semihosting: clean up and add --semihosting-config arg

[Qemu-devel] [PATCH v2 0/2] semihosting: clean up and add --semihosting-config arg

[Leon Alrae]

Hi,

This patch series adds "arg=" sub-option to --semihosting-config group. It
allows building up a list of input arguments as it can appear multiple
times in the command line. This is a flexible solution for creating
argc/argv for the guest program (needed by UHI semihosting for example).
RFC patch and related discussion was here:
https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg00115.html

It also contains some generic code clean up -- all semihosting related
things were moved to vl.c (where they are actually set) and grouped in
the SemihostingConfig structure. They can be accessed via
include/exec/semihost.h introduced in this patch series.

Since this touches generic semihosting code I'm sending it as a separate
patchset from MIPS-specific UHI semihosting patches.

Regards,
Leon

v2:
* squash clean-up related patches so renaming is not required (these
  modifications are relatively simple anyway).

Leon Alrae (2):
  semihosting: create SemihostingConfig structure and semihost.h
  semihosting: add --semihosting-config arg sub-argument

 gdbstub.c                 |  8 +++---
 include/exec/gdbstub.h    |  6 ----
 include/exec/semihost.h   | 56 +++++++++++++++++++++++++++++++++++++
 include/sysemu/sysemu.h   |  1 -
 qemu-options.hx           |  8 ++++--
 target-arm/helper.c       |  7 +++--
 target-lm32/helper.c      |  3 +-
 target-m68k/op_helper.c   |  5 ++--
 target-xtensa/translate.c |  3 +-
 vl.c                      | 71 +++++++++++++++++++++++++++++++++++++++++------
 10 files changed, 137 insertions(+), 31 deletions(-)
 create mode 100644 include/exec/semihost.h

[Qemu-devel] [PATCH v2 1/2] semihosting: create SemihostingConfig structure and semihost.h

[Leon Alrae]

Remove semihosting_enabled and semihosting_target and replace them with
SemihostingConfig structure containing equivalent fields. The structure
is defined in vl.c where it is actually set.

Also introduce separate header file include/exec/semihost.h allowing to
access semihosting config related stuff from target specific semihosting
code.

Signed-off-by: Leon Alrae <leon.alrae@imgtec.com>
---
 gdbstub.c                 |  8 ++++----
 include/exec/gdbstub.h    |  6 ------
 include/exec/semihost.h   | 44 ++++++++++++++++++++++++++++++++++++++++++++
 include/sysemu/sysemu.h   |  1 -
 target-arm/helper.c       |  7 ++++---
 target-lm32/helper.c      |  3 ++-
 target-m68k/op_helper.c   |  5 ++---
 target-xtensa/translate.c |  3 ++-
 vl.c                      | 38 +++++++++++++++++++++++++++++---------
 9 files changed, 87 insertions(+), 28 deletions(-)
 create mode 100644 include/exec/semihost.h

diff --git a/gdbstub.c b/gdbstub.c
index 8abcb8a..9931d81 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -40,6 +40,7 @@
 #include "cpu.h"
 #include "qemu/sockets.h"
 #include "sysemu/kvm.h"
+#include "exec/semihost.h"
 
 static inline int target_memory_rw_debug(CPUState *cpu, target_ulong addr,
                                          uint8_t *buf, int len, bool is_write)
@@ -317,8 +318,6 @@ static GDBState *gdbserver_state;
 
 bool gdb_has_xml;
 
-int semihosting_target = SEMIHOSTING_TARGET_AUTO;
-
 #ifdef CONFIG_USER_ONLY
 /* XXX: This is not thread safe.  Do we care?  */
 static int gdbserver_fd = -1;
@@ -356,10 +355,11 @@ static enum {
 /* Decide if either remote gdb syscalls or native file IO should be used. */
 int use_gdb_syscalls(void)
 {
-    if (semihosting_target == SEMIHOSTING_TARGET_NATIVE) {
+    SemihostingTarget target = semihosting_get_target();
+    if (target == SEMIHOSTING_TARGET_NATIVE) {
         /* -semihosting-config target=native */
         return false;
-    } else if (semihosting_target == SEMIHOSTING_TARGET_GDB) {
+    } else if (target == SEMIHOSTING_TARGET_GDB) {
         /* -semihosting-config target=gdb */
         return true;
     }
diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index c633248..a608a26 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -95,10 +95,4 @@ extern bool gdb_has_xml;
 /* in gdbstub-xml.c, generated by scripts/feature_to_c.sh */
 extern const char *const xml_builtin[][2];
 
-/* Command line option defining whether semihosting should go via gdb or not */
-extern int semihosting_target;
-#define SEMIHOSTING_TARGET_AUTO     0
-#define SEMIHOSTING_TARGET_NATIVE   1
-#define SEMIHOSTING_TARGET_GDB      2
-
 #endif
diff --git a/include/exec/semihost.h b/include/exec/semihost.h
new file mode 100644
index 0000000..c2f0bcb
--- /dev/null
+++ b/include/exec/semihost.h
@@ -0,0 +1,44 @@
+/*
+ * Semihosting support
+ *
+ * Copyright (c) 2015 Imagination Technologies
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef SEMIHOST_H
+#define SEMIHOST_H
+
+typedef enum SemihostingTarget {
+    SEMIHOSTING_TARGET_AUTO = 0,
+    SEMIHOSTING_TARGET_NATIVE,
+    SEMIHOSTING_TARGET_GDB
+} SemihostingTarget;
+
+#ifdef CONFIG_USER_ONLY
+static inline bool semihosting_enabled(void)
+{
+    return true;
+}
+
+static inline SemihostingTarget semihosting_get_target(void)
+{
+    return SEMIHOSTING_TARGET_AUTO;
+}
+#else
+bool semihosting_enabled(void);
+SemihostingTarget semihosting_get_target(void);
+#endif
+
+#endif
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 8a52934..bf552a7 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -125,7 +125,6 @@ extern int cursor_hide;
 extern int graphic_rotate;
 extern int no_quit;
 extern int no_shutdown;
-extern int semihosting_enabled;
 extern int old_param;
 extern int boot_menu;
 extern bool boot_strict;
diff --git a/target-arm/helper.c b/target-arm/helper.c
index f8f8d76..f5a9897 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -10,6 +10,7 @@
 #include "exec/cpu_ldst.h"
 #include "arm_ldst.h"
 #include <zlib.h> /* For crc32 */
+#include "exec/semihost.h"
 
 #ifndef CONFIG_USER_ONLY
 static inline int get_phys_addr(CPUARMState *env, target_ulong address,
@@ -4401,7 +4402,7 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
         armv7m_nvic_set_pending(env->nvic, ARMV7M_EXCP_MEM);
         return;
     case EXCP_BKPT:
-        if (semihosting_enabled) {
+        if (semihosting_enabled()) {
             int nr;
             nr = arm_lduw_code(env, env->regs[15], env->bswap_code) & 0xff;
             if (nr == 0xab) {
@@ -4713,7 +4714,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
             offset = 4;
         break;
     case EXCP_SWI:
-        if (semihosting_enabled) {
+        if (semihosting_enabled()) {
             /* Check for semihosting interrupt.  */
             if (env->thumb) {
                 mask = arm_lduw_code(env, env->regs[15] - 2, env->bswap_code)
@@ -4740,7 +4741,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
         break;
     case EXCP_BKPT:
         /* See if this is a semihosting syscall.  */
-        if (env->thumb && semihosting_enabled) {
+        if (env->thumb && semihosting_enabled()) {
             mask = arm_lduw_code(env, env->regs[15], env->bswap_code) & 0xff;
             if (mask == 0xab
                   && (env->uncached_cpsr & CPSR_M) != ARM_CPU_MODE_USR) {
diff --git a/target-lm32/helper.c b/target-lm32/helper.c
index 7a41f29..a88aa5a 100644
--- a/target-lm32/helper.c
+++ b/target-lm32/helper.c
@@ -20,6 +20,7 @@
 #include "cpu.h"
 #include "qemu/host-utils.h"
 #include "sysemu/sysemu.h"
+#include "exec/semihost.h"
 
 int lm32_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int rw,
                               int mmu_idx)
@@ -162,7 +163,7 @@ void lm32_cpu_do_interrupt(CPUState *cs)
 
     switch (cs->exception_index) {
     case EXCP_SYSTEMCALL:
-        if (unlikely(semihosting_enabled)) {
+        if (unlikely(semihosting_enabled())) {
             /* do_semicall() returns true if call was handled. Otherwise
              * do the normal exception handling. */
             if (lm32_cpu_do_semihosting(cs)) {
diff --git a/target-m68k/op_helper.c b/target-m68k/op_helper.c
index 06661f5..4f8fabb 100644
--- a/target-m68k/op_helper.c
+++ b/target-m68k/op_helper.c
@@ -19,6 +19,7 @@
 #include "cpu.h"
 #include "exec/helper-proto.h"
 #include "exec/cpu_ldst.h"
+#include "exec/semihost.h"
 
 #if defined(CONFIG_USER_ONLY)
 
@@ -33,8 +34,6 @@ static inline void do_interrupt_m68k_hardirq(CPUM68KState *env)
 
 #else
 
-extern int semihosting_enabled;
-
 /* Try to fill the TLB and return an exception if error. If retaddr is
    NULL, it means that the function was called in C code (i.e. not
    from generated code or from helper.c) */
@@ -85,7 +84,7 @@ static void do_interrupt_all(CPUM68KState *env, int is_hw)
             do_rte(env);
             return;
         case EXCP_HALT_INSN:
-            if (semihosting_enabled
+            if (semihosting_enabled()
                     && (env->sr & SR_S) != 0
                     && (env->pc & 3) == 0
                     && cpu_lduw_code(env, env->pc - 4) == 0x4e71
diff --git a/target-xtensa/translate.c b/target-xtensa/translate.c
index 6e5096c..3d52079 100644
--- a/target-xtensa/translate.c
+++ b/target-xtensa/translate.c
@@ -37,6 +37,7 @@
 #include "qemu/log.h"
 #include "sysemu/sysemu.h"
 #include "exec/cpu_ldst.h"
+#include "exec/semihost.h"
 
 #include "exec/helper-proto.h"
 #include "exec/helper-gen.h"
@@ -1216,7 +1217,7 @@ static void disas_xtensa_insn(CPUXtensaState *env, DisasContext *dc)
                         break;
 
                     case 1: /*SIMCALL*/
-                        if (semihosting_enabled) {
+                        if (semihosting_enabled()) {
                             if (gen_check_privilege(dc)) {
                                 gen_helper_simcall(cpu_env);
                             }
diff --git a/vl.c b/vl.c
index 15bccc4..f3319a9 100644
--- a/vl.c
+++ b/vl.c
@@ -119,6 +119,7 @@ int main(int argc, char **argv)
 #include "qapi/opts-visitor.h"
 #include "qom/object_interfaces.h"
 #include "qapi-event.h"
+#include "exec/semihost.h"
 
 #define DEFAULT_RAM_SIZE 128
 
@@ -171,7 +172,6 @@ int graphic_rotate = 0;
 const char *watchdog;
 QEMUOptionRom option_rom[MAX_OPTION_ROMS];
 int nb_option_roms;
-int semihosting_enabled = 0;
 int old_param = 0;
 const char *qemu_name;
 int alt_grab = 0;
@@ -1225,6 +1225,26 @@ static void configure_msg(QemuOpts *opts)
 }
 
 /***********************************************************/
+/* Semihosting */
+
+typedef struct SemihostingConfig {
+    bool enabled;
+    SemihostingTarget target;
+} SemihostingConfig;
+
+static SemihostingConfig semihosting;
+
+bool semihosting_enabled(void)
+{
+    return semihosting.enabled;
+}
+
+SemihostingTarget semihosting_get_target(void)
+{
+    return semihosting.target;
+}
+
+/***********************************************************/
 /* USB devices */
 
 static int usb_device_add(const char *devname)
@@ -3545,24 +3565,24 @@ int main(int argc, char **argv, char **envp)
                 nb_option_roms++;
                 break;
             case QEMU_OPTION_semihosting:
-                semihosting_enabled = 1;
-                semihosting_target = SEMIHOSTING_TARGET_AUTO;
+                semihosting.enabled = true;
+                semihosting.target = SEMIHOSTING_TARGET_AUTO;
                 break;
             case QEMU_OPTION_semihosting_config:
-                semihosting_enabled = 1;
+                semihosting.enabled = true;
                 opts = qemu_opts_parse(qemu_find_opts("semihosting-config"),
                                            optarg, 0);
                 if (opts != NULL) {
-                    semihosting_enabled = qemu_opt_get_bool(opts, "enable",
+                    semihosting.enabled = qemu_opt_get_bool(opts, "enable",
                                                             true);
                     const char *target = qemu_opt_get(opts, "target");
                     if (target != NULL) {
                         if (strcmp("native", target) == 0) {
-                            semihosting_target = SEMIHOSTING_TARGET_NATIVE;
+                            semihosting.target = SEMIHOSTING_TARGET_NATIVE;
                         } else if (strcmp("gdb", target) == 0) {
-                            semihosting_target = SEMIHOSTING_TARGET_GDB;
+                            semihosting.target = SEMIHOSTING_TARGET_GDB;
                         } else  if (strcmp("auto", target) == 0) {
-                            semihosting_target = SEMIHOSTING_TARGET_AUTO;
+                            semihosting.target = SEMIHOSTING_TARGET_AUTO;
                         } else {
                             fprintf(stderr, "Unsupported semihosting-config"
                                     " %s\n",
@@ -3570,7 +3590,7 @@ int main(int argc, char **argv, char **envp)
                             exit(1);
                         }
                     } else {
-                        semihosting_target = SEMIHOSTING_TARGET_AUTO;
+                        semihosting.target = SEMIHOSTING_TARGET_AUTO;
                     }
                 } else {
                     fprintf(stderr, "Unsupported semihosting-config %s\n",

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

[Leon Alrae]

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(-)

diff --git a/include/exec/semihost.h b/include/exec/semihost.h
index c2f0bcb..6e4e8c0 100644
--- a/include/exec/semihost.h
+++ b/include/exec/semihost.h
@@ -36,9 +36,21 @@ static inline SemihostingTarget semihosting_get_target(void)
 {
     return SEMIHOSTING_TARGET_AUTO;
 }
+
+static inline const char *semihosting_get_arg(int i)
+{
+    return NULL;
+}
+
+static inline int semihosting_get_argc(void)
+{
+    return 0;
+}
 #else
 bool semihosting_enabled(void);
 SemihostingTarget semihosting_get_target(void);
+const char *semihosting_get_arg(int i);
+int semihosting_get_argc(void);
 #endif
 
 #endif
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)
 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;
+    }
+    return 0;
+}
+
 /***********************************************************/
 /* USB devices */
 
@@ -3592,6 +3622,9 @@ int main(int argc, char **argv, char **envp)
                     } else {
                         semihosting.target = SEMIHOSTING_TARGET_AUTO;
                     }
+                    /* Set semihosting argument count and vector */
+                    qemu_opt_foreach(opts, add_semihosting_arg,
+                                     &semihosting, 0);
                 } else {
                     fprintf(stderr, "Unsupported semihosting-config %s\n",
                             optarg);

Re: [Qemu-devel] [PATCH v2 1/2] semihosting: create SemihostingConfig structure and semihost.h

[Peter Maydell]

On 7 May 2015 at 12:49, Leon Alrae <leon.alrae@imgtec.com> wrote:
> Remove semihosting_enabled and semihosting_target and replace them with
> SemihostingConfig structure containing equivalent fields. The structure
> is defined in vl.c where it is actually set.
>
> Also introduce separate header file include/exec/semihost.h allowing to
> access semihosting config related stuff from target specific semihosting
> code.
>
> Signed-off-by: Leon Alrae <leon.alrae@imgtec.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

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

[Peter Maydell]

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

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

[Leon Alrae]

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