diff mbox

qdev: add return value to init() callbacks.

Message ID 4A83F389.7080302@redhat.com
State Superseded
Headers show

Commit Message

Gerd Hoffmann Aug. 13, 2009, 11:05 a.m. UTC 
Hi,

> I find stashing error messages for later printing rather awkward.  Do
> you provide space for one fixed-sized message?  Or arbitrary length?
> Arbitrary number of messages?  Once you start to malloc(), you get to
> worry about free()...  Meh.

Arbitrary length.  See attached patch.

> I'd rather use the global or thread-local state to hold the sink for the
> messages, then send the messages there as we make them.  No memory
> management worries.

i.e. like config_error() in net.c?  What I don't like there is that the 
error handling policy (monitor -> continue, otherwise exit) is in the 
error printing function.  IMHO it is the job of the caller to decide how 
to handle an error (exit, ignore, pass up, whatever).

cheers,
   Gerd

Comments

Markus Armbruster Aug. 13, 2009, 11:42 a.m. UTC | #1 
Gerd Hoffmann <kraxel@redhat.com> writes:

>   Hi,
>
>> I find stashing error messages for later printing rather awkward.  Do
>> you provide space for one fixed-sized message?  Or arbitrary length?
>> Arbitrary number of messages?  Once you start to malloc(), you get to
>> worry about free()...  Meh.
>
> Arbitrary length.  See attached patch.

I just don't like filing diagnostics away in the hope that somebody up
the stack will remember to print them :)

At least, qemu_error_message needs to be thread-local.

>> I'd rather use the global or thread-local state to hold the sink for the
>> messages, then send the messages there as we make them.  No memory
>> management worries.
>
> i.e. like config_error() in net.c?  What I don't like there is that
> the error handling policy (monitor -> continue, otherwise exit) is in
> the error printing function.  IMHO it is the job of the caller to
> decide how to handle an error (exit, ignore, pass up, whatever).

config_error() uses a Monitor * to encode what to do on error, which is
admittedly rather limiting.  If you want to be more flexible, use (error
callback, argument).  If you need a richer interface than that, define
an output object with suitable methods, say error, warning, information,
...

The difference to your patch is what the error object does.

With your patch:

    call stuff (may accumulate messages in the output object)
    flush output object down the appropriate sink

Alternative I'd prefer:

    set up output object to point to the appropriate sink
    call stuff (may print messages via output object to sink)

Look ma, no buffer management!

Output diversions are easy, too:

    save output object
    set up output object to point to the appropriate sink
    call stuff (may print messages via output object to sink)
    restore output object
diff mbox

Patch

diff --git a/Makefile b/Makefile
index 5279504..e1e85f4 100644
--- a/Makefile
+++ b/Makefile
@@ -87,7 +87,7 @@  obj-y += sd.o ssi-sd.o
 obj-y += bt.o bt-host.o bt-vhci.o bt-l2cap.o bt-sdp.o bt-hci.o bt-hid.o usb-bt.o
 obj-y += bt-hci-csr.o
 obj-y += buffered_file.o migration.o migration-tcp.o net.o qemu-sockets.o
-obj-y += qemu-char.o aio.o net-checksum.o savevm.o
+obj-y += qemu-char.o qemu-log.o aio.o net-checksum.o savevm.o
 obj-y += msmouse.o ps2.o
 obj-y += qdev.o qdev-properties.o ssi.o
 
diff --git a/hw/qdev.c b/hw/qdev.c
index 1b7d963..53c0117 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -29,6 +29,7 @@ 
 #include "qdev.h"
 #include "sysemu.h"
 #include "monitor.h"
+#include "qemu-log.h"
 
 /* This is a nasty hack to allow passing a NULL bus to qdev_create.  */
 static BusState *main_system_bus;
@@ -92,7 +93,8 @@  DeviceState *qdev_create(BusState *bus, const char *name)
 
     info = qdev_find_info(bus->info, name);
     if (!info) {
-        hw_error("Unknown device '%s' for bus '%s'\n", name, bus->info->name);
+        error_append("Unknown device '%s' for bus '%s'\n", name, bus->info->name);
+        return NULL;
     }
 
     dev = qemu_mallocz(info->size);
@@ -138,8 +140,8 @@  static int set_property(const char *name, const char *value, void *opaque)
         return 0;
 
     if (-1 == qdev_prop_parse(dev, name, value)) {
-        fprintf(stderr, "can't set property \"%s\" to \"%s\" for \"%s\"\n",
-                name, value, dev->info->name);
+        error_append("can't set property \"%s\" to \"%s\" for \"%s\"\n",
+                     name, value, dev->info->name);
         return -1;
     }
     return 0;
@@ -154,14 +156,14 @@  DeviceState *qdev_device_add(QemuOpts *opts)
 
     driver = qemu_opt_get(opts, "driver");
     if (!driver) {
-        fprintf(stderr, "-device: no driver specified\n");
+        error_append("-device: no driver specified\n");
         return NULL;
     }
     if (strcmp(driver, "?") == 0) {
         char msg[256];
         for (info = device_info_list; info != NULL; info = info->next) {
             qdev_print_devinfo(info, msg, sizeof(msg));
-            fprintf(stderr, "%s\n", msg);
+            error_append("%s\n", msg);
         }
         return NULL;
     }
@@ -169,13 +171,13 @@  DeviceState *qdev_device_add(QemuOpts *opts)
     /* find driver */
     info = qdev_find_info(NULL, driver);
     if (!info) {
-        fprintf(stderr, "Device \"%s\" not found.  Try -device '?' for a list.\n",
-                driver);
+        error_append("Device \"%s\" not found.  Try -device '?' for a list.\n",
+                     driver);
         return NULL;
     }
     if (info->no_user) {
-        fprintf(stderr, "device \"%s\" can't be added via command line\n",
-                info->name);
+        error_append("device \"%s\" can't be added via command line\n",
+                     info->name);
         return NULL;
     }
 
@@ -191,6 +193,9 @@  DeviceState *qdev_device_add(QemuOpts *opts)
 
     /* create device, set properties */
     qdev = qdev_create(bus, driver);
+    if (qdev == NULL) {
+        return NULL;
+    }
     id = qemu_opts_id(opts);
     if (id) {
         qdev->id = id;
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 71c3868..f33c97a 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -20,6 +20,7 @@ 
 #include "sysemu.h"
 #include "msix.h"
 #include "net.h"
+#include "qemu-log.h"
 
 /* from Linux's linux/virtio_pci.h */
 
@@ -434,7 +435,7 @@  static int virtio_blk_init_pci(PCIDevice *pci_dev)
         proxy->class_code = PCI_CLASS_STORAGE_SCSI;
 
     if (!proxy->dinfo) {
-        fprintf(stderr, "drive property not set\n");
+        error_append("virtio-blk-pci: drive property not set\n");
         return -1;
     }
     vdev = virtio_blk_init(&pci_dev->qdev, proxy->dinfo);
diff --git a/qemu-log.c b/qemu-log.c
new file mode 100644
index 0000000..0a5d81f
--- /dev/null
+++ b/qemu-log.c
@@ -0,0 +1,60 @@ 
+#include "qemu-common.h"
+#include "monitor.h"
+#include "qemu-log.h"
+
+/* message buffer implementation */
+
+struct MsgBuf {
+    unsigned int bufsize;
+    unsigned int msgsize;
+    char         *message;
+};
+
+void msg_append(MsgBuf *buf, const char *fmt, ...)
+{
+    va_list args;
+    size_t len;
+
+again:
+    va_start(args, fmt);
+    len = vsnprintf(buf->message + buf->msgsize,
+                    buf->bufsize - buf->msgsize, fmt, args);
+    va_end(args);
+    if (len > buf->bufsize - buf->msgsize) {
+        buf->bufsize = buf->msgsize + len + 1;
+        buf->message = qemu_realloc(buf->message, buf->bufsize);
+        goto again;
+    }
+    buf->msgsize += len;
+}
+
+void msg_clear(MsgBuf *buf)
+{
+    buf->msgsize = 0;
+}
+
+void msg_free(MsgBuf *buf)
+{
+    free(buf->message);
+    buf->message = NULL;
+    buf->msgsize = 0;
+    buf->bufsize = 0;
+}
+
+void msg_print_file(MsgBuf *buf, FILE *fp, int clear)
+{
+    fprintf(fp, "%.*s", buf->msgsize, buf->message);
+    if (clear)
+        msg_clear(buf);
+}
+
+void msg_print_mon(MsgBuf *buf, Monitor *mon, int clear)
+{
+    monitor_printf(mon, "%.*s", buf->msgsize, buf->message);
+    if (clear)
+        msg_clear(buf);
+}
+
+/* use message buffer to store error messages */
+
+struct MsgBuf qemu_error_message;
diff --git a/qemu-log.h b/qemu-log.h
index fccfb11..0dea895 100644
--- a/qemu-log.h
+++ b/qemu-log.h
@@ -51,9 +51,9 @@  extern int loglevel;
 /* Special cases: */
 
 /* cpu_dump_state() logging functions: */
-#define log_cpu_state(env, f) cpu_dump_state((env), logfile, fprintf, (f));
-#define log_cpu_state_mask(b, env, f) do {           \
-      if (loglevel & (b)) log_cpu_state((env), (f)); \
+#define log_cpu_state(_env, f) cpu_dump_state((_env), logfile, fprintf, (f));
+#define log_cpu_state_mask(b, _env, f) do {           \
+      if (loglevel & (b)) log_cpu_state((_env), (f)); \
   } while (0)
 
 /* disas() and target_disas() to logfile: */
@@ -90,4 +90,24 @@  extern int loglevel;
     } while (0)
 
 
+/* message buffer implementation */
+struct Monitor;
+typedef struct MsgBuf MsgBuf;
+
+void msg_append(MsgBuf *buf, const char *fmt, ...)
+    __attribute__ ((format(printf, 2, 3)));
+void msg_clear(MsgBuf *buf);
+void msg_free(MsgBuf *buf);
+void msg_print_file(MsgBuf *buf, FILE *fp, int clear);
+void msg_print_mon(MsgBuf *buf, struct Monitor *mon, int clear);
+
+/* use message buffer to store error messages */
+extern struct MsgBuf qemu_error_message;
+#define error_append(...)                               \
+    msg_append(&qemu_error_message, ## __VA_ARGS__)
+#define error_print_stderr()                            \
+    msg_print_file(&qemu_error_message, stderr, 1)
+#define error_print_monitor(mon)                \
+    msg_print_mon(&qemu_error_message, mon, 1)
+
 #endif
diff --git a/vl.c b/vl.c
index 8b2b289..b88afbe 100644
--- a/vl.c
+++ b/vl.c
@@ -5936,8 +5936,10 @@  int main(int argc, char **argv, char **envp)
     }
 
     /* init generic devices */
-    if (qemu_opts_foreach(&qemu_device_opts, device_init_func, NULL, 1) != 0)
+    if (qemu_opts_foreach(&qemu_device_opts, device_init_func, NULL, 1) != 0) {
+        error_print_stderr();
         exit(1);
+    }
 
     if (!display_state)
         dumb_display_init();