From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MbYA3-0004LA-BT for qemu-devel@nongnu.org; Thu, 13 Aug 2009 07:07:59 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MbY9x-0004Hj-GQ for qemu-devel@nongnu.org; Thu, 13 Aug 2009 07:07:57 -0400 Received: from [199.232.76.173] (port=57317 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MbY9x-0004Hg-DD for qemu-devel@nongnu.org; Thu, 13 Aug 2009 07:07:53 -0400 Received: from mx2.redhat.com ([66.187.237.31]:45170) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MbY9w-0005mg-Li for qemu-devel@nongnu.org; Thu, 13 Aug 2009 07:07:53 -0400 Message-ID: <4A83F389.7080302@redhat.com> Date: Thu, 13 Aug 2009 13:05:45 +0200 From: Gerd Hoffmann MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] qdev: add return value to init() callbacks. References: <1250092766-23986-1-git-send-email-kraxel@redhat.com> <200908122028.41012.paul@codesourcery.com> <4A831C47.3070309@redhat.com> <873a7w2q6n.fsf@pike.pond.sub.org> <4A83C562.1090802@redhat.com> <87tz0cytem.fsf@pike.pond.sub.org> In-Reply-To: <87tz0cytem.fsf@pike.pond.sub.org> Content-Type: multipart/mixed; boundary="------------020101090307010808060701" List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Paul Brook , qemu-devel@nongnu.org This is a multi-part message in MIME format. --------------020101090307010808060701 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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 --------------020101090307010808060701 Content-Type: text/plain; name="fix" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="fix" 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(); --------------020101090307010808060701--