diff mbox

[FOR,0.12,v3,17/21] rework -monitor handling, switch to QemuOpts

Message ID 1260189773-20728-18-git-send-email-kraxel@redhat.com
State New
Headers show

Commit Message

Gerd Hoffmann Dec. 7, 2009, 12:42 p.m. UTC 
This patch reworks the -monitor handling:

 - It adds a new "mon" QemuOpts list for the monitor(s).
 - It adds a monitor_parse() function to parse the -monitor switch.
 - It adds a mon_init function to initialize the monitor(s) from the
   "mon" QemuOpts list.
 - It winds up everything and removes the old bits.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 qemu-config.c |   19 +++++++++
 qemu-config.h |    1 +
 vl.c          |  121 ++++++++++++++++++++++++++++++++++++--------------------
 3 files changed, 98 insertions(+), 43 deletions(-)

Comments

Luiz Capitulino Dec. 7, 2009, 2:59 p.m. UTC | #1 
On Mon,  7 Dec 2009 13:42:49 +0100
Gerd Hoffmann <kraxel@redhat.com> wrote:

> +static int mon_init_func(QemuOpts *opts, void *opaque)
> +{
> +    CharDriverState *chr;
> +    const char *chardev;
> +    const char *mode;
> +    int flags;
> +
> +    qemu_opts_print(opts, NULL);
> +
> +    mode = qemu_opt_get(opts, "mode");
> +    if (mode == NULL) {
> +        mode = "readline";
> +    }
> +    if (strcmp(mode, "readline") == 0) {
> +        flags = MONITOR_USE_READLINE;
> +    } else if (strcmp(mode, "control") == 0) {
> +        flags = MONITOR_USE_CONTROL;
> +    } else {
> +        fprintf(stderr, "unknown monitor mode \"%s\"\n", mode);
> +        exit(1);
> +    }
> +
> +    if (qemu_opt_get_bool(opts, "default", 0))
> +        flags |= MONITOR_IS_DEFAULT;
> +
> +    chardev = qemu_opt_get(opts, "chardev");
> +    chr = qemu_chr_find(chardev);
> +    if (chardev == NULL) {
> +        fprintf(stderr, "chardev \"%s\" not found\n", chardev);
> +        exit(1);
> +    }

 We should check for NULL before calling qemu_chr_find().

 Also, I'm getting a segfault when running QEMU w/o any monitor cmd-line,
like:

$ qemu -hda disks/fedora-11-kratos-i386.img -m 1G

 The reason for the segfault is that the chardev 'monitor' is not found,
so qemu_chr_find() returns NULL, passing it down to:

> +
> +    monitor_init(chr, flags);
> +    return 0;
> +}
> +

 Apart from that, monitor changes seem ok to me.
Gerd Hoffmann Dec. 7, 2009, 3:18 p.m. UTC | #2 
On 12/07/09 15:59, Luiz Capitulino wrote:
>> +    chardev = qemu_opt_get(opts, "chardev");
>> +    chr = qemu_chr_find(chardev);
>> +    if (chardev == NULL) {
>> +        fprintf(stderr, "chardev \"%s\" not found\n", chardev);
>> +        exit(1);
>> +    }
>
>   We should check for NULL before calling qemu_chr_find().

No.  qemu_chr_find() can deal with NULL just fine.  We should check the 
return value (chr) instead though.  When fixing that it prints the error 
message and exits as it should.  Well, almost, because ...

>   The reason for the segfault is that the chardev 'monitor' is not found,
> so qemu_chr_find() returns NULL, passing it down to:

... 'monitor' should be there, there is a default for the monitor 
device.  Hmm, checking why ...

cheers,
   Gerd
Anthony Liguori Dec. 7, 2009, 7:11 p.m. UTC | #3 
Gerd Hoffmann wrote:
> This patch reworks the -monitor handling:
>
>  - It adds a new "mon" QemuOpts list for the monitor(s).
>  - It adds a monitor_parse() function to parse the -monitor switch.
>  - It adds a mon_init function to initialize the monitor(s) from the
>    "mon" QemuOpts list.
>  - It winds up everything and removes the old bits.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>   

This SEGVs as Luiz has reported, but it also has some debugging noise in it:

> +static int mon_init_func(QemuOpts *opts, void *opaque)
> +{
> +    CharDriverState *chr;
> +    const char *chardev;
> +    const char *mode;
> +    int flags;
> +
> +    qemu_opts_print(opts, NULL);
>   

Right here.

> +    mode = qemu_opt_get(opts, "mode");
> +    if (mode == NULL) {
> +        mode = "readline";
> +    }
> +    if (strcmp(mode, "readline") == 0) {
> +        flags = MONITOR_USE_READLINE;
> +    } else if (strcmp(mode, "control") == 0) {
> +        flags = MONITOR_USE_CONTROL;
> +    } else {
> +        fprintf(stderr, "unknown monitor mode \"%s\"\n", mode);
> +        exit(1);
> +    }
> +
Regards,

Anthony Liguori
diff mbox

Patch

diff --git a/qemu-config.c b/qemu-config.c
index a23b125..c3203c8 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -224,6 +224,24 @@  QemuOptsList qemu_global_opts = {
     },
 };
 
+QemuOptsList qemu_mon_opts = {
+    .name = "mon",
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_mon_opts.head),
+    .desc = {
+        {
+            .name = "mode",
+            .type = QEMU_OPT_STRING,
+        },{
+            .name = "chardev",
+            .type = QEMU_OPT_STRING,
+        },{
+            .name = "default",
+            .type = QEMU_OPT_BOOL,
+        },
+        { /* end if list */ }
+    },
+};
+
 static QemuOptsList *lists[] = {
     &qemu_drive_opts,
     &qemu_chardev_opts,
@@ -232,6 +250,7 @@  static QemuOptsList *lists[] = {
     &qemu_net_opts,
     &qemu_rtc_opts,
     &qemu_global_opts,
+    &qemu_mon_opts,
     NULL,
 };
 
diff --git a/qemu-config.h b/qemu-config.h
index 6246e76..34dfadc 100644
--- a/qemu-config.h
+++ b/qemu-config.h
@@ -7,6 +7,7 @@  extern QemuOptsList qemu_device_opts;
 extern QemuOptsList qemu_netdev_opts;
 extern QemuOptsList qemu_net_opts;
 extern QemuOptsList qemu_rtc_opts;
+extern QemuOptsList qemu_mon_opts;
 
 int qemu_set_option(const char *str);
 int qemu_global_option(const char *str);
diff --git a/vl.c b/vl.c
index 701b687..11910ac 100644
--- a/vl.c
+++ b/vl.c
@@ -172,9 +172,6 @@  int main(int argc, char **argv)
 
 #define DEFAULT_RAM_SIZE 128
 
-/* Maximum number of monitor devices */
-#define MAX_MONITOR_DEVICES 10
-
 static const char *data_dir;
 const char *bios_name = NULL;
 /* Note: drives_table[MAX_DRIVES] is a dummy block driver if none available
@@ -211,7 +208,6 @@  int no_quit = 0;
 CharDriverState *serial_hds[MAX_SERIAL_PORTS];
 CharDriverState *parallel_hds[MAX_PARALLEL_PORTS];
 CharDriverState *virtcon_hds[MAX_VIRTIO_CONSOLES];
-CharDriverState *monitor_hds[MAX_MONITOR_DEVICES];
 #ifdef TARGET_I386
 int win2k_install_hack = 0;
 int rtc_td_hack = 0;
@@ -4630,13 +4626,85 @@  static int chardev_init_func(QemuOpts *opts, void *opaque)
     return 0;
 }
 
+static int mon_init_func(QemuOpts *opts, void *opaque)
+{
+    CharDriverState *chr;
+    const char *chardev;
+    const char *mode;
+    int flags;
+
+    qemu_opts_print(opts, NULL);
+
+    mode = qemu_opt_get(opts, "mode");
+    if (mode == NULL) {
+        mode = "readline";
+    }
+    if (strcmp(mode, "readline") == 0) {
+        flags = MONITOR_USE_READLINE;
+    } else if (strcmp(mode, "control") == 0) {
+        flags = MONITOR_USE_CONTROL;
+    } else {
+        fprintf(stderr, "unknown monitor mode \"%s\"\n", mode);
+        exit(1);
+    }
+
+    if (qemu_opt_get_bool(opts, "default", 0))
+        flags |= MONITOR_IS_DEFAULT;
+
+    chardev = qemu_opt_get(opts, "chardev");
+    chr = qemu_chr_find(chardev);
+    if (chardev == NULL) {
+        fprintf(stderr, "chardev \"%s\" not found\n", chardev);
+        exit(1);
+    }
+
+    monitor_init(chr, flags);
+    return 0;
+}
+
+static void monitor_parse(const char *optarg)
+{
+    static int monitor_device_index = 0;
+    QemuOpts *opts;
+    const char *p;
+    char label[32];
+    int def = 0;
+
+    if (strstart(optarg, "chardev:", &p)) {
+        snprintf(label, sizeof(label), "%s", p);
+    } else {
+        if (monitor_device_index) {
+            snprintf(label, sizeof(label), "monitor%d",
+                     monitor_device_index);
+        } else {
+            snprintf(label, sizeof(label), "monitor");
+            def = 1;
+        }
+        opts = qemu_chr_parse_compat(label, optarg);
+        if (!opts) {
+            fprintf(stderr, "fixme #1\n");
+            exit(1);
+        }
+    }
+
+    opts = qemu_opts_create(&qemu_mon_opts, label, 1);
+    if (!opts) {
+        fprintf(stderr, "fixme #2\n");
+        exit(1);
+    }
+    qemu_opt_set(opts, "mode", "readline");
+    qemu_opt_set(opts, "chardev", label);
+    if (def)
+        qemu_opt_set(opts, "default", "on");
+    monitor_device_index++;
+}
+
 struct device_config {
     enum {
         DEV_USB,       /* -usbdevice   */
         DEV_BT,        /* -bt          */
         DEV_SERIAL,    /* -serial      */
         DEV_PARALLEL,  /* -parallel    */
-        DEV_MONITOR,   /* -monitor     */
     } type;
     const char *cmdline;
     QTAILQ_ENTRY(device_config) next;
@@ -4712,32 +4780,6 @@  static int parallel_parse(const char *devname)
     return 0;
 }
 
-static int monitor_parse(const char *devname)
-{
-    static int index = 0;
-    char label[32];
-
-    if (strcmp(devname, "none") == 0)
-        return 0;
-    if (index == MAX_MONITOR_DEVICES) {
-        fprintf(stderr, "qemu: too many monitor devices\n");
-        exit(1);
-    }
-    if (index == 0) {
-        snprintf(label, sizeof(label), "monitor");
-    } else {
-        snprintf(label, sizeof(label), "monitor%d", index);
-    }
-    monitor_hds[index] = qemu_chr_open(label, devname, NULL);
-    if (!monitor_hds[index]) {
-        fprintf(stderr, "qemu: could not open monitor device '%s'\n",
-                devname);
-        return -1;
-    }
-    index++;
-    return 0;
-}
-
 int main(int argc, char **argv, char **envp)
 {
     const char *gdbstub_dev = NULL;
@@ -5241,7 +5283,7 @@  int main(int argc, char **argv, char **envp)
                     break;
                 }
             case QEMU_OPTION_monitor:
-                add_device_config(DEV_MONITOR, optarg);
+                monitor_parse(optarg);
                 default_monitor = 0;
                 break;
             case QEMU_OPTION_chardev:
@@ -5572,7 +5614,7 @@  int main(int argc, char **argv, char **envp)
             if (default_serial)
                 add_device_config(DEV_SERIAL, "stdio");
             if (default_monitor)
-                add_device_config(DEV_MONITOR, "stdio");
+                monitor_parse("stdio");
         }
     } else {
         if (default_serial)
@@ -5580,7 +5622,7 @@  int main(int argc, char **argv, char **envp)
         if (default_parallel)
             add_device_config(DEV_PARALLEL, "vc:80Cx24C");
         if (default_monitor)
-            add_device_config(DEV_MONITOR, "vc:80Cx24C");
+            monitor_parse("vc:80Cx24C");
     }
     if (default_vga)
         vga_interface_type = VGA_CIRRUS;
@@ -5774,8 +5816,6 @@  int main(int argc, char **argv, char **envp)
         }
     }
 
-    if (foreach_device_config(DEV_MONITOR, monitor_parse) < 0)
-        exit(1);
     if (foreach_device_config(DEV_SERIAL, serial_parse) < 0)
         exit(1);
     if (foreach_device_config(DEV_PARALLEL, parallel_parse) < 0)
@@ -5900,13 +5940,8 @@  int main(int argc, char **argv, char **envp)
 
     text_consoles_set_display(display_state);
 
-    for (i = 0; i < MAX_MONITOR_DEVICES; i++) {
-        if (monitor_hds[i]) {
-            monitor_init(monitor_hds[i],
-                         MONITOR_USE_READLINE |
-                         ((i == 0) ? MONITOR_IS_DEFAULT : 0));
-        }
-    }
+    if (qemu_opts_foreach(&qemu_mon_opts, mon_init_func, NULL, 1) != 0)
+        exit(1);
 
     if (gdbstub_dev && gdbserver_start(gdbstub_dev) < 0) {
         fprintf(stderr, "qemu: could not open gdbserver on device '%s'\n",