Re: [Qemu-devel] [PATCH] qdev: add return value to init() callbacks.

[Gerd Hoffmann <kraxel@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 750 bytes --]

   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

[-- Attachment #2: fix --]
[-- Type: text/plain, Size: 7193 bytes --]

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