From 248337e178fbcf1c20132d4f3d1033cb0dde7638 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 9 Feb 2015 11:49:08 +0100 Subject: vhost-scsi: Improve error reporting for invalid vhostfd We get two error messages: one from monitor_handle_fd_param2(), and another one from vhost_scsi_realize(). The second one gets suppressed in QMP context. That's because monitor_handle_fd_param() calls qerror_report_err(). Calling qerror_report_err() is always inappropriate in realize methods, because it doesn't return the Error object. It either reports the error to stderr or the human monitor, or it stores it in the QMP monitor, where it makes the QMP command fail even when the realize method ignores the error and succeeds. Fortunately, vhost_scsi_realize() doesn't do that. Fix by switching to monitor_handle_fd_param2(). Signed-off-by: Markus Armbruster Acked-by: Paolo Bonzini --- hw/scsi/vhost-scsi.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'hw/scsi/vhost-scsi.c') diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c index dcb2bc5a6e..567f350c43 100644 --- a/hw/scsi/vhost-scsi.c +++ b/hw/scsi/vhost-scsi.c @@ -214,9 +214,11 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp) } if (vs->conf.vhostfd) { - vhostfd = monitor_handle_fd_param(cur_mon, vs->conf.vhostfd); + vhostfd = monitor_handle_fd_param2(cur_mon, vs->conf.vhostfd, &err); if (vhostfd == -1) { - error_setg(errp, "vhost-scsi: unable to parse vhostfd"); + error_setg(errp, "vhost-scsi: unable to parse vhostfd: %s", + error_get_pretty(err)); + error_free(err); return; } } else { -- cgit v1.2.3 From 1677f4c66cf2228eb14f1b0571d0e3b38d0d6606 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Mon, 9 Feb 2015 14:03:19 +0100 Subject: monitor: Clean up around monitor_handle_fd_param() monitor_handle_fd_param() is a wrapper around monitor_handle_fd_param2() that feeds errors to qerror_report_err() instead of returning them. qerror_report_err() is inappropriate in many contexts. monitor_handle_fd_param() looks simpler than monitor_handle_fd_param2(), which tempts use. Remove the temptation: drop the wrapper and open-code the (trivial) error handling instead. Replace the open-coded qerror_report_err() by error_report_err() in places that already use error_report(). Turns out that's everywhere. While there, rename monitor_handle_fd_param2() to monitor_fd_param(). Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake --- hw/i386/kvm/pci-assign.c | 5 ++--- hw/scsi/vhost-scsi.c | 2 +- include/monitor/monitor.h | 3 +-- monitor.c | 15 +-------------- net/socket.c | 4 +++- net/tap.c | 11 ++++++++--- 6 files changed, 16 insertions(+), 24 deletions(-) (limited to 'hw/scsi/vhost-scsi.c') diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c index 29ce2c4eb8..bd92c69916 100644 --- a/hw/i386/kvm/pci-assign.c +++ b/hw/i386/kvm/pci-assign.c @@ -552,9 +552,8 @@ static void get_real_device(AssignedDevice *pci_dev, Error **errp) snprintf(name, sizeof(name), "%sconfig", dir); if (pci_dev->configfd_name && *pci_dev->configfd_name) { - dev->config_fd = monitor_handle_fd_param2(cur_mon, - pci_dev->configfd_name, - &local_err); + dev->config_fd = monitor_fd_param(cur_mon, pci_dev->configfd_name, + &local_err); if (local_err) { error_propagate(errp, local_err); return; diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c index 567f350c43..484f4a8e52 100644 --- a/hw/scsi/vhost-scsi.c +++ b/hw/scsi/vhost-scsi.c @@ -214,7 +214,7 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp) } if (vs->conf.vhostfd) { - vhostfd = monitor_handle_fd_param2(cur_mon, vs->conf.vhostfd, &err); + vhostfd = monitor_fd_param(cur_mon, vs->conf.vhostfd, &err); if (vhostfd == -1) { error_setg(errp, "vhost-scsi: unable to parse vhostfd: %s", error_get_pretty(err)); diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h index 47606d04ad..1c06bed39d 100644 --- a/include/monitor/monitor.h +++ b/include/monitor/monitor.h @@ -34,8 +34,7 @@ int monitor_read_block_device_key(Monitor *mon, const char *device, void *opaque); int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp); -int monitor_handle_fd_param(Monitor *mon, const char *fdname); -int monitor_handle_fd_param2(Monitor *mon, const char *fdname, Error **errp); +int monitor_fd_param(Monitor *mon, const char *fdname, Error **errp); void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap) GCC_FMT_ATTR(2, 0); diff --git a/monitor.c b/monitor.c index c3cc060b45..ac2a4ab6e6 100644 --- a/monitor.c +++ b/monitor.c @@ -2570,20 +2570,7 @@ void monitor_fdset_dup_fd_remove(int dup_fd) monitor_fdset_dup_fd_find_remove(dup_fd, true); } -int monitor_handle_fd_param(Monitor *mon, const char *fdname) -{ - int fd; - Error *local_err = NULL; - - fd = monitor_handle_fd_param2(mon, fdname, &local_err); - if (local_err) { - qerror_report_err(local_err); - error_free(local_err); - } - return fd; -} - -int monitor_handle_fd_param2(Monitor *mon, const char *fdname, Error **errp) +int monitor_fd_param(Monitor *mon, const char *fdname, Error **errp) { int fd; Error *local_err = NULL; diff --git a/net/socket.c b/net/socket.c index 68a93cd7e3..c30e03f5ae 100644 --- a/net/socket.c +++ b/net/socket.c @@ -695,6 +695,7 @@ static int net_socket_udp_init(NetClientState *peer, int net_init_socket(const NetClientOptions *opts, const char *name, NetClientState *peer) { + Error *err = NULL; const NetdevSocketOptions *sock; assert(opts->kind == NET_CLIENT_OPTIONS_KIND_SOCKET); @@ -715,8 +716,9 @@ int net_init_socket(const NetClientOptions *opts, const char *name, if (sock->has_fd) { int fd; - fd = monitor_handle_fd_param(cur_mon, sock->fd); + fd = monitor_fd_param(cur_mon, sock->fd, &err); if (fd == -1) { + error_report_err(err); return -1; } qemu_set_nonblock(fd); diff --git a/net/tap.c b/net/tap.c index 1fe0edfdf7..968df46c8c 100644 --- a/net/tap.c +++ b/net/tap.c @@ -605,6 +605,7 @@ static int net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer, const char *downscript, const char *vhostfdname, int vnet_hdr, int fd) { + Error *err = NULL; TAPState *s; int vhostfd; @@ -643,8 +644,9 @@ static int net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer, options.force = tap->has_vhostforce && tap->vhostforce; if (tap->has_vhostfd || tap->has_vhostfds) { - vhostfd = monitor_handle_fd_param(cur_mon, vhostfdname); + vhostfd = monitor_fd_param(cur_mon, vhostfdname, &err); if (vhostfd == -1) { + error_report_err(err); return -1; } } else { @@ -704,6 +706,7 @@ int net_init_tap(const NetClientOptions *opts, const char *name, /* for the no-fd, no-helper case */ const char *script = NULL; /* suppress wrong "uninit'd use" gcc warning */ const char *downscript = NULL; + Error *err = NULL; const char *vhostfdname; char ifname[128]; @@ -729,8 +732,9 @@ int net_init_tap(const NetClientOptions *opts, const char *name, return -1; } - fd = monitor_handle_fd_param(cur_mon, tap->fd); + fd = monitor_fd_param(cur_mon, tap->fd, &err); if (fd == -1) { + error_report_err(err); return -1; } @@ -768,8 +772,9 @@ int net_init_tap(const NetClientOptions *opts, const char *name, } for (i = 0; i < nfds; i++) { - fd = monitor_handle_fd_param(cur_mon, fds[i]); + fd = monitor_fd_param(cur_mon, fds[i], &err); if (fd == -1) { + error_report_err(err); return -1; } -- cgit v1.2.3