[Qemu-devel] [PATCH 00/16] PCI device assignment: improve error reporting over QMP

[Qemu-devel] [PATCH 00/16] PCI device assignment: improve error reporting over QMP

[Laszlo Ersek]

This is for <https://bugzilla.redhat.com/show_bug.cgi?id=616415>.

In general, we want to propagate non-fatal (ie. non-exit()ing,
non-abort()ing) errors to the QMP caller, rather than printing them
on-site. The series focuses on errors raised at PCI assignment time (ie.
reachable from assigned_initfn()), other errors are not converted.

Errors are not propagated through assigned_initfn(); let's wait for
someone else to convert "kvm-pci-assign" from qdev to QOM. The series is
nonetheless an improvement, because the forwarding of initialization
errors now stops just before device_realize(). We set the
stored/persistent monitor error there.

Informative and warning messages (that report about circumstances that
don't break the assignment operation) cannot terminate in
qerror_report_err(), because that would set the persistent monitor
error, breaking the high level (QMP) operation.

A call graph rooted in assigned_initfn() was generated with cflow.
Leaves that could never fail were removed from the graph (and this
property similarly propagated upwards as far as possible).

The patchset loosely follows a bottom-up algorithm on this calltree. Any
leaf that reports an error internally and returns a related failure is
converted to "throw" an Error structure instead. All direct callers of
the converted leaf are addressed at once, in the same patch, to consume
the error (and they become the new leaves gradually).

When the leaf to be converted is also called outside of
"hw/i386/kvm/pci-assign.c", the conversion keeps a compatibility
function under the original name, so that conversion of callers
unrelated to PCI assignment can be deferred.

Reviewers should copy the call graph to a text file, and mark, as the
series progresses, fully converted functions. (Ie. functions that now
report their terminating error messages with Error objects only.)

  assigned_initfn()
      error_report()
      get_real_device()
          monitor_handle_fd_param()
          error_report()
          get_real_vendor_id()
              get_real_id()
                  error_report()
          get_real_device_id()
              get_real_id()
                  error_report()
      assigned_device_pci_cap_init()
          check_irqchip_in_kernel()
              error_report()
          pci_add_capability()
          error_report()
      assigned_dev_register_msix_mmio()
          error_report()
      assigned_dev_register_regions()
          error_report()
      assign_device()
          error_report()
          assign_failed_examine()
              get_real_vendor_id()
                  get_real_id()
                      error_report()
              get_real_device_id()
                  get_real_id()
                      error_report()
              error_printf()
              error_report()
      assign_intx()
          check_irqchip_in_kernel()
              error_report()
          error_report()
          error_printf()

Laszlo Ersek (16):
  cutils: tighten qemu_parse_fd()
  monitor: add Error-propagating monitor_handle_fd_param2()
  pci-assign: accept Error from monitor_handle_fd_param2()
  pci-assign: make assign_failed_examine() just format the cause
  pci-assign: propagate errors from get_real_id()
  pci-assign: propagate Error from check_irqchip_in_kernel()
  pci: add Error-propagating pci_add_capability2()
  pci-assign: accept Error from pci_add_capability2()
  pci-assign: assignment should fail if we can't read config space
  pci-assign: propagate errors from get_real_device()
  pci-assign: propagate errors from assigned_device_pci_cap_init()
  pci-assign: propagate errors from assigned_dev_register_msix_mmio()
  pci-assign: propagate errors from assigned_dev_register_regions()
  pci-assign: propagate errors from assign_device()
  pci-assign: propagate errors from assign_intx()
  pci-assign: assigned_initfn(): set monitor error in common error
    handler

 include/hw/pci/pci.h      |   4 +
 include/monitor/monitor.h |   1 +
 hw/i386/kvm/pci-assign.c  | 273 ++++++++++++++++++++++++++++------------------
 hw/pci/pci.c              |  32 +++++-
 monitor.c                 |  29 ++++-
 util/cutils.c             |  13 ++-
 6 files changed, 232 insertions(+), 120 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 01/16] cutils: tighten qemu_parse_fd()

[Laszlo Ersek]

qemu_parse_fd() used to handle at least the following strings incorrectly:
o "-2":         simply let through
o "2147483648": returned as LONG_MAX==INT_MAX on ILP32 (with ERANGE
                ignored); implementation-defined behavior on LP64

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 util/cutils.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/util/cutils.c b/util/cutils.c
index b337293..dbe7412 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -22,10 +22,12 @@
  * THE SOFTWARE.
  */
 #include "qemu-common.h"
 #include "qemu/host-utils.h"
 #include <math.h>
+#include <limits.h>
+#include <errno.h>
 
 #include "qemu/sockets.h"
 #include "qemu/iov.h"
 #include "net/net.h"
 
@@ -455,15 +457,20 @@ int parse_uint_full(const char *s, unsigned long long *value, int base)
     return 0;
 }
 
 int qemu_parse_fd(const char *param)
 {
-    int fd;
-    char *endptr = NULL;
+    long fd;
+    char *endptr;
 
+    errno = 0;
     fd = strtol(param, &endptr, 10);
-    if (*endptr || (fd == 0 && param == endptr)) {
+    if (param == endptr /* no conversion performed */                    ||
+        errno != 0      /* not representable as long; possibly others */ ||
+        *endptr != '\0' /* final string not empty */                     ||
+        fd < 0          /* invalid as file descriptor */                 ||
+        fd > INT_MAX    /* not representable as int */) {
         return -1;
     }
     return fd;
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 02/16] monitor: add Error-propagating monitor_handle_fd_param2()

[Laszlo Ersek]

and rebase monitor_handle_fd_param() to it. (Note that this will slightly
change the behavior when the qemu_parse_fd() branch is selected and it
fails: we now report (and in case of QMP, set) the error immediately,
rather than allowing the caller to set its own error message (if any)).

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 include/monitor/monitor.h |  1 +
 monitor.c                 | 29 +++++++++++++++++++++++------
 2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index a49ea11..07e3d29 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -73,10 +73,11 @@ int monitor_read_block_device_key(Monitor *mon, const char *device,
                                   BlockDriverCompletionFunc *completion_cb,
                                   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);
 
 void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
     GCC_FMT_ATTR(2, 0);
 void monitor_printf(Monitor *mon, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
 void monitor_print_filename(Monitor *mon, const char *filename);
diff --git a/monitor.c b/monitor.c
index 342e83b..0ef9749 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2634,20 +2634,37 @@ int monitor_fdset_dup_fd_remove(int dup_fd)
 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 fd;
+    Error *local_err = NULL;
+
     if (!qemu_isdigit(fdname[0]) && mon) {
-
         fd = monitor_get_fd(mon, fdname, &local_err);
-        if (fd == -1) {
-            qerror_report_err(local_err);
-            error_free(local_err);
-            return -1;
-        }
     } else {
         fd = qemu_parse_fd(fdname);
+        if (fd == -1) {
+            error_setg(&local_err, "Invalid file descriptor number '%s'",
+                       fdname);
+        }
+    }
+    if (local_err) {
+        error_propagate(errp, local_err);
+        assert(fd == -1);
+    } else {
+        assert(fd != -1);
     }
 
     return fd;
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 03/16] pci-assign: accept Error from monitor_handle_fd_param2()

[Laszlo Ersek]

Propagate any errors in monitor fd handling up to get_real_device(), and
report them there. We'll continue the propagation upwards when
get_real_device() becomes a leaf itself (when none of its callees will
report errors internally any longer when detecting and returning an
error).

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/i386/kvm/pci-assign.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index a825871..bfce97f 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -539,22 +539,27 @@ static int get_real_device(AssignedDevice *pci_dev)
     FILE *f;
     uint64_t start, end, size, flags;
     uint16_t id;
     PCIRegion *rp;
     PCIDevRegions *dev = &pci_dev->real_device;
+    Error *local_err = NULL;
 
     dev->region_number = 0;
 
     snprintf(dir, sizeof(dir), "/sys/bus/pci/devices/%04x:%02x:%02x.%x/",
              pci_dev->host.domain, pci_dev->host.bus,
              pci_dev->host.slot, pci_dev->host.function);
 
     snprintf(name, sizeof(name), "%sconfig", dir);
 
     if (pci_dev->configfd_name && *pci_dev->configfd_name) {
-        dev->config_fd = monitor_handle_fd_param(cur_mon, pci_dev->configfd_name);
-        if (dev->config_fd < 0) {
+        dev->config_fd = monitor_handle_fd_param2(cur_mon,
+                                                  pci_dev->configfd_name,
+                                                  &local_err);
+        if (local_err) {
+            qerror_report_err(local_err);
+            error_free(local_err);
             return 1;
         }
     } else {
         dev->config_fd = open(name, O_RDWR);
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 04/16] pci-assign: make assign_failed_examine() just format the cause

[Laszlo Ersek]

This allows us to report the entire error with one error_report() call,
easing future error propagation.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/i386/kvm/pci-assign.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index bfce97f..6b8db25 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -729,11 +729,16 @@ static void free_assigned_device(AssignedDevice *dev)
     }
 
     free_msi_virqs(dev);
 }
 
-static void assign_failed_examine(AssignedDevice *dev)
+/* This function tries to determine the cause of the PCI assignment failure. It
+ * always returns the cause as a dynamically allocated, human readable string.
+ * If the function fails to determine the cause for any internal reason, then
+ * the returned string will state that fact.
+ */
+static char *assign_failed_examine(const AssignedDevice *dev)
 {
     char name[PATH_MAX], dir[PATH_MAX], driver[PATH_MAX] = {}, *ns;
     uint16_t vendor_id, device_id;
     int r;
 
@@ -759,12 +764,12 @@ static void assign_failed_examine(AssignedDevice *dev)
     if (get_real_vendor_id(dir, &vendor_id) ||
         get_real_device_id(dir, &device_id)) {
         goto fail;
     }
 
-    error_printf("*** The driver '%s' is occupying your device "
-        "%04x:%02x:%02x.%x.\n"
+    return g_strdup_printf(
+        "*** The driver '%s' is occupying your device %04x:%02x:%02x.%x.\n"
         "***\n"
         "*** You can try the following commands to free it:\n"
         "***\n"
         "*** $ echo \"%04x %04x\" > /sys/bus/pci/drivers/pci-stub/new_id\n"
         "*** $ echo \"%04x:%02x:%02x.%x\" > /sys/bus/pci/drivers/%s/unbind\n"
@@ -776,14 +781,12 @@ static void assign_failed_examine(AssignedDevice *dev)
         dev->host.function, vendor_id, device_id,
         dev->host.domain, dev->host.bus, dev->host.slot, dev->host.function,
         ns, dev->host.domain, dev->host.bus, dev->host.slot,
         dev->host.function, vendor_id, device_id);
 
-    return;
-
 fail:
-    error_report("Couldn't find out why.");
+    return g_strdup("Couldn't find out why.");
 }
 
 static int assign_device(AssignedDevice *dev)
 {
     uint32_t flags = KVM_DEV_ASSIGN_ENABLE_IOMMU;
@@ -808,18 +811,23 @@ static int assign_device(AssignedDevice *dev)
         flags |= KVM_DEV_ASSIGN_PCI_2_3;
     }
 
     r = kvm_device_pci_assign(kvm_state, &dev->host, flags, &dev->dev_id);
     if (r < 0) {
-        error_report("Failed to assign device \"%s\" : %s",
-                     dev->dev.qdev.id, strerror(-r));
-
         switch (r) {
-        case -EBUSY:
-            assign_failed_examine(dev);
+        case -EBUSY: {
+            char *cause;
+
+            cause = assign_failed_examine(dev);
+            error_report("Failed to assign device \"%s\" : %s\n%s",
+                         dev->dev.qdev.id, strerror(-r), cause);
+            g_free(cause);
             break;
+        }
         default:
+            error_report("Failed to assign device \"%s\" : %s",
+                         dev->dev.qdev.id, strerror(-r));
             break;
         }
     }
     return r;
 }
-- 
1.8.3.1

[Qemu-devel] [PATCH 05/16] pci-assign: propagate errors from get_real_id()

[Laszlo Ersek]

get_real_id() has two thin wrappers (and no other callers),
get_real_vendor_id() and get_real_device_id(); it's easiest to convert
them in one fell swoop.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/i386/kvm/pci-assign.c | 45 +++++++++++++++++++++++++++------------------
 1 file changed, 27 insertions(+), 18 deletions(-)

diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index 6b8db25..997ef09 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -497,47 +497,47 @@ static int assigned_dev_register_regions(PCIRegion *io_regions,
 
     /* success */
     return 0;
 }
 
-static int get_real_id(const char *devpath, const char *idname, uint16_t *val)
+static void get_real_id(const char *devpath, const char *idname, uint16_t *val,
+                        Error **errp)
 {
     FILE *f;
     char name[128];
     long id;
 
     snprintf(name, sizeof(name), "%s%s", devpath, idname);
     f = fopen(name, "r");
     if (f == NULL) {
-        error_report("%s: %s: %m", __func__, name);
-        return -1;
+        error_setg_file_open(errp, errno, name);
+        return;
     }
     if (fscanf(f, "%li\n", &id) == 1) {
         *val = id;
     } else {
-        fclose(f);
-        return -1;
+        error_setg(errp, "Failed to parse contents of '%s'", name);
     }
     fclose(f);
-
-    return 0;
 }
 
-static int get_real_vendor_id(const char *devpath, uint16_t *val)
+static void get_real_vendor_id(const char *devpath, uint16_t *val,
+                               Error **errp)
 {
-    return get_real_id(devpath, "vendor", val);
+    get_real_id(devpath, "vendor", val, errp);
 }
 
-static int get_real_device_id(const char *devpath, uint16_t *val)
+static void get_real_device_id(const char *devpath, uint16_t *val,
+                               Error **errp)
 {
-    return get_real_id(devpath, "device", val);
+    get_real_id(devpath, "device", val, errp);
 }
 
 static int get_real_device(AssignedDevice *pci_dev)
 {
     char dir[128], name[128];
-    int fd, r = 0, v;
+    int fd, r = 0;
     FILE *f;
     uint64_t start, end, size, flags;
     uint16_t id;
     PCIRegion *rp;
     PCIDevRegions *dev = &pci_dev->real_device;
@@ -637,20 +637,24 @@ again:
     }
 
     fclose(f);
 
     /* read and fill vendor ID */
-    v = get_real_vendor_id(dir, &id);
-    if (v) {
+    get_real_vendor_id(dir, &id, &local_err);
+    if (local_err) {
+        error_report("%s", error_get_pretty(local_err));
+        error_free(local_err);
         return 1;
     }
     pci_dev->dev.config[0] = id & 0xff;
     pci_dev->dev.config[1] = (id & 0xff00) >> 8;
 
     /* read and fill device ID */
-    v = get_real_device_id(dir, &id);
-    if (v) {
+    get_real_device_id(dir, &id, &local_err);
+    if (local_err) {
+        error_report("%s", error_get_pretty(local_err));
+        error_free(local_err);
         return 1;
     }
     pci_dev->dev.config[2] = id & 0xff;
     pci_dev->dev.config[3] = (id & 0xff00) >> 8;
 
@@ -739,10 +743,11 @@ static void free_assigned_device(AssignedDevice *dev)
 static char *assign_failed_examine(const AssignedDevice *dev)
 {
     char name[PATH_MAX], dir[PATH_MAX], driver[PATH_MAX] = {}, *ns;
     uint16_t vendor_id, device_id;
     int r;
+    Error *local_err = NULL;
 
     snprintf(dir, sizeof(dir), "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/",
             dev->host.domain, dev->host.bus, dev->host.slot,
             dev->host.function);
 
@@ -759,12 +764,16 @@ static char *assign_failed_examine(const AssignedDevice *dev)
         goto fail;
     }
 
     ns++;
 
-    if (get_real_vendor_id(dir, &vendor_id) ||
-        get_real_device_id(dir, &device_id)) {
+    if ((get_real_vendor_id(dir, &vendor_id, &local_err), local_err) ||
+        (get_real_device_id(dir, &device_id, &local_err), local_err)) {
+        /* We're already analyzing an assignment error, so we suppress this
+         * one just like the others above.
+         */
+        error_free(local_err);
         goto fail;
     }
 
     return g_strdup_printf(
         "*** The driver '%s' is occupying your device %04x:%02x:%02x.%x.\n"
-- 
1.8.3.1

[Qemu-devel] [PATCH 06/16] pci-assign: propagate Error from check_irqchip_in_kernel()

[Laszlo Ersek]

Rename check_irqchip_in_kernel() to verify_irqchip_in_kernel(), so that
the name reflects our expectation better. Rather than returning a bool,
make it do nothing or set an Error.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/i386/kvm/pci-assign.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index 997ef09..b4696aa 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -839,34 +839,36 @@ static int assign_device(AssignedDevice *dev)
         }
     }
     return r;
 }
 
-static bool check_irqchip_in_kernel(void)
+static void verify_irqchip_in_kernel(Error **errp)
 {
     if (kvm_irqchip_in_kernel()) {
-        return true;
+        return;
     }
-    error_report("pci-assign: error: requires KVM with in-kernel irqchip "
-                 "enabled");
-    return false;
+    error_setg(errp, "pci-assign requires KVM with in-kernel irqchip enabled");
 }
 
 static int assign_intx(AssignedDevice *dev)
 {
     AssignedIRQType new_type;
     PCIINTxRoute intx_route;
     bool intx_host_msi;
     int r;
+    Error *local_err = NULL;
 
     /* Interrupt PIN 0 means don't use INTx */
     if (assigned_dev_pci_read_byte(&dev->dev, PCI_INTERRUPT_PIN) == 0) {
         pci_device_set_intx_routing_notifier(&dev->dev, NULL);
         return 0;
     }
 
-    if (!check_irqchip_in_kernel()) {
+    verify_irqchip_in_kernel(&local_err);
+    if (local_err) {
+        error_report("%s", error_get_pretty(local_err));
+        error_free(local_err);
         return -ENOTSUP;
     }
 
     pci_device_set_intx_routing_notifier(&dev->dev,
                                          assigned_dev_update_irq_routing);
@@ -1239,10 +1241,11 @@ static void assigned_dev_setup_cap_read(AssignedDevice *dev, uint32_t offset,
 static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
 {
     AssignedDevice *dev = DO_UPCAST(AssignedDevice, dev, pci_dev);
     PCIRegion *pci_region = dev->real_device.regions;
     int ret, pos;
+    Error *local_err = NULL;
 
     /* Clear initial capabilities pointer and status copied from hw */
     pci_set_byte(pci_dev->config + PCI_CAPABILITY_LIST, 0);
     pci_set_word(pci_dev->config + PCI_STATUS,
                  pci_get_word(pci_dev->config + PCI_STATUS) &
@@ -1250,11 +1253,14 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
 
     /* Expose MSI capability
      * MSI capability is the 1st capability in capability config */
     pos = pci_find_cap_offset(pci_dev, PCI_CAP_ID_MSI, 0);
     if (pos != 0 && kvm_check_extension(kvm_state, KVM_CAP_ASSIGN_DEV_IRQ)) {
-        if (!check_irqchip_in_kernel()) {
+        verify_irqchip_in_kernel(&local_err);
+        if (local_err) {
+            error_report("%s", error_get_pretty(local_err));
+            error_free(local_err);
             return -ENOTSUP;
         }
         dev->cap.available |= ASSIGNED_DEVICE_CAP_MSI;
         /* Only 32-bit/no-mask currently supported */
         ret = pci_add_capability(pci_dev, PCI_CAP_ID_MSI, pos, 10);
@@ -1279,11 +1285,14 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
     pos = pci_find_cap_offset(pci_dev, PCI_CAP_ID_MSIX, 0);
     if (pos != 0 && kvm_device_msix_supported(kvm_state)) {
         int bar_nr;
         uint32_t msix_table_entry;
 
-        if (!check_irqchip_in_kernel()) {
+        verify_irqchip_in_kernel(&local_err);
+        if (local_err) {
+            error_report("%s", error_get_pretty(local_err));
+            error_free(local_err);
             return -ENOTSUP;
         }
         dev->cap.available |= ASSIGNED_DEVICE_CAP_MSIX;
         ret = pci_add_capability(pci_dev, PCI_CAP_ID_MSIX, pos, 12);
         if (ret < 0) {
-- 
1.8.3.1

[Qemu-devel] [PATCH 07/16] pci: add Error-propagating pci_add_capability2()

[Laszlo Ersek]

... and rebase pci_add_capability() to it.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 include/hw/pci/pci.h |  4 ++++
 hw/pci/pci.c         | 32 ++++++++++++++++++++++++++------
 2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 693dd6b..8c25ae5 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -4,10 +4,11 @@
 #include "qemu-common.h"
 
 #include "hw/qdev.h"
 #include "exec/memory.h"
 #include "sysemu/dma.h"
+#include "qapi/error.h"
 
 /* PCI includes legacy ISA access.  */
 #include "hw/isa/isa.h"
 
 #include "hw/pci/pcie.h"
@@ -306,10 +307,13 @@ void pci_register_vga(PCIDevice *pci_dev, MemoryRegion *mem,
 void pci_unregister_vga(PCIDevice *pci_dev);
 pcibus_t pci_get_bar_addr(PCIDevice *pci_dev, int region_num);
 
 int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
                        uint8_t offset, uint8_t size);
+int pci_add_capability2(PCIDevice *pdev, uint8_t cap_id,
+                       uint8_t offset, uint8_t size,
+                       Error **errp);
 
 void pci_del_capability(PCIDevice *pci_dev, uint8_t cap_id, uint8_t cap_size);
 
 uint8_t pci_find_capability(PCIDevice *pci_dev, uint8_t cap_id);
 
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 2a9f08e..64e6f23 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2011,32 +2011,52 @@ static void pci_del_option_rom(PCIDevice *pdev)
  * Find and reserve space and add capability to the linked list
  * in pci config space */
 int pci_add_capability(PCIDevice *pdev, uint8_t cap_id,
                        uint8_t offset, uint8_t size)
 {
+    int ret;
+    Error *local_err = NULL;
+
+    ret = pci_add_capability2(pdev, cap_id, offset, size, &local_err);
+    if (local_err) {
+        assert(ret < 0);
+        error_report("%s", error_get_pretty(local_err));
+        error_free(local_err);
+    } else {
+        /* success implies a positive offset in config space */
+        assert(ret > 0);
+    }
+    return ret;
+}
+
+int pci_add_capability2(PCIDevice *pdev, uint8_t cap_id,
+                       uint8_t offset, uint8_t size,
+                       Error **errp)
+{
     uint8_t *config;
     int i, overlapping_cap;
 
     if (!offset) {
         offset = pci_find_space(pdev, size);
         if (!offset) {
+            error_setg(errp, "out of PCI config space");
             return -ENOSPC;
         }
     } else {
         /* Verify that capabilities don't overlap.  Note: device assignment
          * depends on this check to verify that the device is not broken.
          * Should never trigger for emulated devices, but it's helpful
          * for debugging these. */
         for (i = offset; i < offset + size; i++) {
             overlapping_cap = pci_find_capability_at_offset(pdev, i);
             if (overlapping_cap) {
-                fprintf(stderr, "ERROR: %s:%02x:%02x.%x "
-                        "Attempt to add PCI capability %x at offset "
-                        "%x overlaps existing capability %x at offset %x\n",
-                        pci_root_bus_path(pdev), pci_bus_num(pdev->bus),
-                        PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn),
-                        cap_id, offset, overlapping_cap, i);
+                error_setg(errp, "%s:%02x:%02x.%x "
+                           "Attempt to add PCI capability %x at offset "
+                           "%x overlaps existing capability %x at offset %x",
+                           pci_root_bus_path(pdev), pci_bus_num(pdev->bus),
+                           PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn),
+                           cap_id, offset, overlapping_cap, i);
                 return -EINVAL;
             }
         }
     }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 08/16] pci-assign: accept Error from pci_add_capability2()

[Laszlo Ersek]

Propagate any errors while adding PCI capabilities to
assigned_device_pci_cap_init(). We'll continue the propagation upwards
when assigned_device_pci_cap_init() becomes a leaf itself (when none of
its callees will report errors internally any longer when detecting and
returning them).

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/i386/kvm/pci-assign.c | 35 ++++++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index b4696aa..f91d4fb 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -1261,12 +1261,15 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
             error_free(local_err);
             return -ENOTSUP;
         }
         dev->cap.available |= ASSIGNED_DEVICE_CAP_MSI;
         /* Only 32-bit/no-mask currently supported */
-        ret = pci_add_capability(pci_dev, PCI_CAP_ID_MSI, pos, 10);
+        ret = pci_add_capability2(pci_dev, PCI_CAP_ID_MSI, pos, 10,
+                                  &local_err);
         if (ret < 0) {
+            error_report("%s", error_get_pretty(local_err));
+            error_free(local_err);
             return ret;
         }
         pci_dev->msi_cap = pos;
 
         pci_set_word(pci_dev->config + pos + PCI_MSI_FLAGS,
@@ -1292,12 +1295,15 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
             error_report("%s", error_get_pretty(local_err));
             error_free(local_err);
             return -ENOTSUP;
         }
         dev->cap.available |= ASSIGNED_DEVICE_CAP_MSIX;
-        ret = pci_add_capability(pci_dev, PCI_CAP_ID_MSIX, pos, 12);
+        ret = pci_add_capability2(pci_dev, PCI_CAP_ID_MSIX, pos, 12,
+                                  &local_err);
         if (ret < 0) {
+            error_report("%s", error_get_pretty(local_err));
+            error_free(local_err);
             return ret;
         }
         pci_dev->msix_cap = pos;
 
         pci_set_word(pci_dev->config + pos + PCI_MSIX_FLAGS,
@@ -1320,12 +1326,15 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
     /* Minimal PM support, nothing writable, device appears to NAK changes */
     pos = pci_find_cap_offset(pci_dev, PCI_CAP_ID_PM, 0);
     if (pos) {
         uint16_t pmc;
 
-        ret = pci_add_capability(pci_dev, PCI_CAP_ID_PM, pos, PCI_PM_SIZEOF);
+        ret = pci_add_capability2(pci_dev, PCI_CAP_ID_PM, pos, PCI_PM_SIZEOF,
+                                  &local_err);
         if (ret < 0) {
+            error_report("%s", error_get_pretty(local_err));
+            error_free(local_err);
             return ret;
         }
 
         assigned_dev_setup_cap_read(dev, pos, PCI_PM_SIZEOF);
 
@@ -1386,12 +1395,15 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
             error_report("%s: Unsupported PCI express capability version %d",
                          __func__, version);
             return -EINVAL;
         }
 
-        ret = pci_add_capability(pci_dev, PCI_CAP_ID_EXP, pos, size);
+        ret = pci_add_capability2(pci_dev, PCI_CAP_ID_EXP, pos, size,
+                                  &local_err);
         if (ret < 0) {
+            error_report("%s", error_get_pretty(local_err));
+            error_free(local_err);
             return ret;
         }
 
         assigned_dev_setup_cap_read(dev, pos, size);
 
@@ -1460,12 +1472,15 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
     if (pos) {
         uint16_t cmd;
         uint32_t status;
 
         /* Only expose the minimum, 8 byte capability */
-        ret = pci_add_capability(pci_dev, PCI_CAP_ID_PCIX, pos, 8);
+        ret = pci_add_capability2(pci_dev, PCI_CAP_ID_PCIX, pos, 8,
+                                  &local_err);
         if (ret < 0) {
+            error_report("%s", error_get_pretty(local_err));
+            error_free(local_err);
             return ret;
         }
 
         assigned_dev_setup_cap_read(dev, pos, 8);
 
@@ -1486,12 +1501,15 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
     }
 
     pos = pci_find_cap_offset(pci_dev, PCI_CAP_ID_VPD, 0);
     if (pos) {
         /* Direct R/W passthrough */
-        ret = pci_add_capability(pci_dev, PCI_CAP_ID_VPD, pos, 8);
+        ret = pci_add_capability2(pci_dev, PCI_CAP_ID_VPD, pos, 8,
+                                  &local_err);
         if (ret < 0) {
+            error_report("%s", error_get_pretty(local_err));
+            error_free(local_err);
             return ret;
         }
 
         assigned_dev_setup_cap_read(dev, pos, 8);
 
@@ -1502,12 +1520,15 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
     /* Devices can have multiple vendor capabilities, get them all */
     for (pos = 0; (pos = pci_find_cap_offset(pci_dev, PCI_CAP_ID_VNDR, pos));
         pos += PCI_CAP_LIST_NEXT) {
         uint8_t len = pci_get_byte(pci_dev->config + pos + PCI_CAP_FLAGS);
         /* Direct R/W passthrough */
-        ret = pci_add_capability(pci_dev, PCI_CAP_ID_VNDR, pos, len);
+        ret = pci_add_capability2(pci_dev, PCI_CAP_ID_VNDR, pos, len,
+                                  &local_err);
         if (ret < 0) {
+            error_report("%s", error_get_pretty(local_err));
+            error_free(local_err);
             return ret;
         }
 
         assigned_dev_setup_cap_read(dev, pos, len);
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 09/16] pci-assign: assignment should fail if we can't read config space

[Laszlo Ersek]

assigned_initfn()
  get_real_device()
    read()

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/i386/kvm/pci-assign.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index f91d4fb..e89bb6a 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -574,10 +574,11 @@ again:
     if (r < 0) {
         if (errno == EINTR || errno == EAGAIN) {
             goto again;
         }
         error_report("%s: read failed, errno = %d", __func__, errno);
+        return 1;
     }
 
     /* Restore or clear multifunction, this is always controlled by qemu */
     if (pci_dev->dev.cap_present & QEMU_PCI_CAP_MULTIFUNCTION) {
         pci_dev->dev.config[PCI_HEADER_TYPE] |= PCI_HEADER_TYPE_MULTI_FUNCTION;
-- 
1.8.3.1

[Qemu-devel] [PATCH 10/16] pci-assign: propagate errors from get_real_device()

[Laszlo Ersek]

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/i386/kvm/pci-assign.c | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index e89bb6a..c6d1094 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -530,11 +530,11 @@ static void get_real_device_id(const char *devpath, uint16_t *val,
                                Error **errp)
 {
     get_real_id(devpath, "device", val, errp);
 }
 
-static int get_real_device(AssignedDevice *pci_dev)
+static void get_real_device(AssignedDevice *pci_dev, Error **errp)
 {
     char dir[128], name[128];
     int fd, r = 0;
     FILE *f;
     uint64_t start, end, size, flags;
@@ -554,31 +554,32 @@ static int get_real_device(AssignedDevice *pci_dev)
     if (pci_dev->configfd_name && *pci_dev->configfd_name) {
         dev->config_fd = monitor_handle_fd_param2(cur_mon,
                                                   pci_dev->configfd_name,
                                                   &local_err);
         if (local_err) {
-            qerror_report_err(local_err);
-            error_free(local_err);
-            return 1;
+            error_propagate(errp, local_err);
+            return;
         }
     } else {
         dev->config_fd = open(name, O_RDWR);
 
         if (dev->config_fd == -1) {
-            error_report("%s: %s: %m", __func__, name);
-            return 1;
+            error_setg_file_open(errp, errno, name);
+            return;
         }
     }
 again:
     r = read(dev->config_fd, pci_dev->dev.config,
              pci_config_size(&pci_dev->dev));
     if (r < 0) {
         if (errno == EINTR || errno == EAGAIN) {
             goto again;
         }
-        error_report("%s: read failed, errno = %d", __func__, errno);
-        return 1;
+        error_setg_errno(errp, errno, "read(\"%s\")",
+                         (pci_dev->configfd_name && *pci_dev->configfd_name) ?
+                         pci_dev->configfd_name : name);
+        return;
     }
 
     /* Restore or clear multifunction, this is always controlled by qemu */
     if (pci_dev->dev.cap_present & QEMU_PCI_CAP_MULTIFUNCTION) {
         pci_dev->dev.config[PCI_HEADER_TYPE] |= PCI_HEADER_TYPE_MULTI_FUNCTION;
@@ -594,12 +595,12 @@ again:
 
     snprintf(name, sizeof(name), "%sresource", dir);
 
     f = fopen(name, "r");
     if (f == NULL) {
-        error_report("%s: %s: %m", __func__, name);
-        return 1;
+        error_setg_file_open(errp, errno, name);
+        return;
     }
 
     for (r = 0; r < PCI_ROM_SLOT; r++) {
         if (fscanf(f, "%" SCNi64 " %" SCNi64 " %" SCNi64 "\n",
                    &start, &end, &flags) != 3) {
@@ -640,32 +641,29 @@ again:
     fclose(f);
 
     /* read and fill vendor ID */
     get_real_vendor_id(dir, &id, &local_err);
     if (local_err) {
-        error_report("%s", error_get_pretty(local_err));
-        error_free(local_err);
-        return 1;
+        error_propagate(errp, local_err);
+        return;
     }
     pci_dev->dev.config[0] = id & 0xff;
     pci_dev->dev.config[1] = (id & 0xff00) >> 8;
 
     /* read and fill device ID */
     get_real_device_id(dir, &id, &local_err);
     if (local_err) {
-        error_report("%s", error_get_pretty(local_err));
-        error_free(local_err);
-        return 1;
+        error_propagate(errp, local_err);
+        return;
     }
     pci_dev->dev.config[2] = id & 0xff;
     pci_dev->dev.config[3] = (id & 0xff00) >> 8;
 
     pci_word_test_and_clear_mask(pci_dev->emulate_config_write + PCI_COMMAND,
                                  PCI_COMMAND_MASTER | PCI_COMMAND_INTX_DISABLE);
 
     dev->region_number = r;
-    return 0;
 }
 
 static void free_msi_virqs(AssignedDevice *dev)
 {
     int i;
@@ -1749,10 +1747,11 @@ static void reset_assigned_device(DeviceState *dev)
 static int assigned_initfn(struct PCIDevice *pci_dev)
 {
     AssignedDevice *dev = DO_UPCAST(AssignedDevice, dev, pci_dev);
     uint8_t e_intx;
     int r;
+    Error *local_err = NULL;
 
     if (!kvm_enabled()) {
         error_report("pci-assign: error: requires KVM support");
         return -1;
     }
@@ -1781,13 +1780,14 @@ static int assigned_initfn(struct PCIDevice *pci_dev)
     assigned_dev_direct_config_read(dev, PCI_MIN_GNT, 1);
     assigned_dev_direct_config_read(dev, PCI_MAX_LAT, 1);
     memcpy(dev->emulate_config_write, dev->emulate_config_read,
            sizeof(dev->emulate_config_read));
 
-    if (get_real_device(dev)) {
-        error_report("pci-assign: Error: Couldn't get real device (%s)!",
-                     dev->dev.qdev.id);
+    get_real_device(dev, &local_err);
+    if (local_err) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         goto out;
     }
 
     if (assigned_device_pci_cap_init(pci_dev) < 0) {
         goto out;
-- 
1.8.3.1

[Qemu-devel] [PATCH 11/16] pci-assign: propagate errors from assigned_device_pci_cap_init()

[Laszlo Ersek]

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/i386/kvm/pci-assign.c | 45 +++++++++++++++++++--------------------------
 1 file changed, 19 insertions(+), 26 deletions(-)

diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index c6d1094..2de6559 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -1235,11 +1235,11 @@ static void assigned_dev_setup_cap_read(AssignedDevice *dev, uint32_t offset,
 {
     assigned_dev_direct_config_read(dev, offset, len);
     assigned_dev_emulate_config_read(dev, offset + PCI_CAP_LIST_NEXT, 1);
 }
 
-static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
+static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp)
 {
     AssignedDevice *dev = DO_UPCAST(AssignedDevice, dev, pci_dev);
     PCIRegion *pci_region = dev->real_device.regions;
     int ret, pos;
     Error *local_err = NULL;
@@ -1254,21 +1254,19 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
      * MSI capability is the 1st capability in capability config */
     pos = pci_find_cap_offset(pci_dev, PCI_CAP_ID_MSI, 0);
     if (pos != 0 && kvm_check_extension(kvm_state, KVM_CAP_ASSIGN_DEV_IRQ)) {
         verify_irqchip_in_kernel(&local_err);
         if (local_err) {
-            error_report("%s", error_get_pretty(local_err));
-            error_free(local_err);
+            error_propagate(errp, local_err);
             return -ENOTSUP;
         }
         dev->cap.available |= ASSIGNED_DEVICE_CAP_MSI;
         /* Only 32-bit/no-mask currently supported */
         ret = pci_add_capability2(pci_dev, PCI_CAP_ID_MSI, pos, 10,
                                   &local_err);
         if (ret < 0) {
-            error_report("%s", error_get_pretty(local_err));
-            error_free(local_err);
+            error_propagate(errp, local_err);
             return ret;
         }
         pci_dev->msi_cap = pos;
 
         pci_set_word(pci_dev->config + pos + PCI_MSI_FLAGS,
@@ -1289,20 +1287,18 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
         int bar_nr;
         uint32_t msix_table_entry;
 
         verify_irqchip_in_kernel(&local_err);
         if (local_err) {
-            error_report("%s", error_get_pretty(local_err));
-            error_free(local_err);
+            error_propagate(errp, local_err);
             return -ENOTSUP;
         }
         dev->cap.available |= ASSIGNED_DEVICE_CAP_MSIX;
         ret = pci_add_capability2(pci_dev, PCI_CAP_ID_MSIX, pos, 12,
                                   &local_err);
         if (ret < 0) {
-            error_report("%s", error_get_pretty(local_err));
-            error_free(local_err);
+            error_propagate(errp, local_err);
             return ret;
         }
         pci_dev->msix_cap = pos;
 
         pci_set_word(pci_dev->config + pos + PCI_MSIX_FLAGS,
@@ -1328,12 +1324,11 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
         uint16_t pmc;
 
         ret = pci_add_capability2(pci_dev, PCI_CAP_ID_PM, pos, PCI_PM_SIZEOF,
                                   &local_err);
         if (ret < 0) {
-            error_report("%s", error_get_pretty(local_err));
-            error_free(local_err);
+            error_propagate(errp, local_err);
             return ret;
         }
 
         assigned_dev_setup_cap_read(dev, pos, PCI_PM_SIZEOF);
 
@@ -1367,12 +1362,12 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
              * PCIe v3.0 spec that regs should exist and be read as 0,
              * not optionally provided and shorten the struct size.
              */
             size = MIN(0x3c, PCI_CONFIG_SPACE_SIZE - pos);
             if (size < 0x34) {
-                error_report("%s: Invalid size PCIe cap-id 0x%x",
-                             __func__, PCI_CAP_ID_EXP);
+                error_setg(errp, "Invalid size PCIe cap-id 0x%x",
+                           PCI_CAP_ID_EXP);
                 return -EINVAL;
             } else if (size != 0x3c) {
                 error_report("WARNING, %s: PCIe cap-id 0x%x has "
                              "non-standard size 0x%x; std size should be 0x3c",
                              __func__, PCI_CAP_ID_EXP, size);
@@ -1389,31 +1384,30 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
                 size = 0x3c;
             }
         }
 
         if (size == 0) {
-            error_report("%s: Unsupported PCI express capability version %d",
-                         __func__, version);
+            error_setg(errp, "Unsupported PCI express capability version %d",
+                       version);
             return -EINVAL;
         }
 
         ret = pci_add_capability2(pci_dev, PCI_CAP_ID_EXP, pos, size,
                                   &local_err);
         if (ret < 0) {
-            error_report("%s", error_get_pretty(local_err));
-            error_free(local_err);
+            error_propagate(errp, local_err);
             return ret;
         }
 
         assigned_dev_setup_cap_read(dev, pos, size);
 
         type = pci_get_word(pci_dev->config + pos + PCI_EXP_FLAGS);
         type = (type & PCI_EXP_FLAGS_TYPE) >> 4;
         if (type != PCI_EXP_TYPE_ENDPOINT &&
             type != PCI_EXP_TYPE_LEG_END && type != PCI_EXP_TYPE_RC_END) {
-            error_report("Device assignment only supports endpoint assignment,"
-                         " device type %d", type);
+            error_setg(errp, "Device assignment only supports endpoint "
+                       "assignment, device type %d", type);
             return -EINVAL;
         }
 
         /* capabilities, pass existing read-only copy
          * PCI_EXP_FLAGS_IRQ: updated by hardware, should be direct read */
@@ -1474,12 +1468,11 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
 
         /* Only expose the minimum, 8 byte capability */
         ret = pci_add_capability2(pci_dev, PCI_CAP_ID_PCIX, pos, 8,
                                   &local_err);
         if (ret < 0) {
-            error_report("%s", error_get_pretty(local_err));
-            error_free(local_err);
+            error_propagate(errp, local_err);
             return ret;
         }
 
         assigned_dev_setup_cap_read(dev, pos, 8);
 
@@ -1503,12 +1496,11 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
     if (pos) {
         /* Direct R/W passthrough */
         ret = pci_add_capability2(pci_dev, PCI_CAP_ID_VPD, pos, 8,
                                   &local_err);
         if (ret < 0) {
-            error_report("%s", error_get_pretty(local_err));
-            error_free(local_err);
+            error_propagate(errp, local_err);
             return ret;
         }
 
         assigned_dev_setup_cap_read(dev, pos, 8);
 
@@ -1522,12 +1514,11 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
         uint8_t len = pci_get_byte(pci_dev->config + pos + PCI_CAP_FLAGS);
         /* Direct R/W passthrough */
         ret = pci_add_capability2(pci_dev, PCI_CAP_ID_VNDR, pos, len,
                                   &local_err);
         if (ret < 0) {
-            error_report("%s", error_get_pretty(local_err));
-            error_free(local_err);
+            error_propagate(errp, local_err);
             return ret;
         }
 
         assigned_dev_setup_cap_read(dev, pos, len);
 
@@ -1787,11 +1778,13 @@ static int assigned_initfn(struct PCIDevice *pci_dev)
         qerror_report_err(local_err);
         error_free(local_err);
         goto out;
     }
 
-    if (assigned_device_pci_cap_init(pci_dev) < 0) {
+    if (assigned_device_pci_cap_init(pci_dev, &local_err) < 0) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         goto out;
     }
 
     /* intercept MSI-X entry page in the MMIO */
     if (dev->cap.available & ASSIGNED_DEVICE_CAP_MSIX) {
-- 
1.8.3.1

[Qemu-devel] [PATCH 12/16] pci-assign: propagate errors from assigned_dev_register_msix_mmio()

[Laszlo Ersek]

The return type is also changed from "int" to "void", because it was used
in a success vs. failure sense only (the caller didn't distinguish error
codes from each other, and even assigned_dev_register_msix_mmio() masked
mmap()'s errno values with a common -EFAULT).

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/i386/kvm/pci-assign.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index 2de6559..3a904e8 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -1642,24 +1642,23 @@ static void assigned_dev_msix_reset(AssignedDevice *dev)
     for (i = 0, entry = dev->msix_table; i < dev->msix_max; i++, entry++) {
         entry->ctrl = cpu_to_le32(0x1); /* Masked */
     }
 }
 
-static int assigned_dev_register_msix_mmio(AssignedDevice *dev)
+static void assigned_dev_register_msix_mmio(AssignedDevice *dev, Error **errp)
 {
     dev->msix_table = mmap(NULL, MSIX_PAGE_SIZE, PROT_READ|PROT_WRITE,
                            MAP_ANONYMOUS|MAP_PRIVATE, 0, 0);
     if (dev->msix_table == MAP_FAILED) {
-        error_report("fail allocate msix_table! %s", strerror(errno));
-        return -EFAULT;
+        error_setg_errno(errp, errno, "failed to allocate msix_table");
+        return;
     }
 
     assigned_dev_msix_reset(dev);
 
     memory_region_init_io(&dev->mmio, OBJECT(dev), &assigned_dev_msix_mmio_ops,
                           dev, "assigned-dev-msix", MSIX_PAGE_SIZE);
-    return 0;
 }
 
 static void assigned_dev_unregister_msix_mmio(AssignedDevice *dev)
 {
     if (!dev->msix_table) {
@@ -1786,11 +1785,14 @@ static int assigned_initfn(struct PCIDevice *pci_dev)
         goto out;
     }
 
     /* intercept MSI-X entry page in the MMIO */
     if (dev->cap.available & ASSIGNED_DEVICE_CAP_MSIX) {
-        if (assigned_dev_register_msix_mmio(dev)) {
+        assigned_dev_register_msix_mmio(dev, &local_err);
+        if (local_err) {
+            qerror_report_err(local_err);
+            error_free(local_err);
             goto out;
         }
     }
 
     /* handle real device's MMIO/PIO BARs */
-- 
1.8.3.1

[Qemu-devel] [PATCH 13/16] pci-assign: propagate errors from assigned_dev_register_regions()

[Laszlo Ersek]

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/i386/kvm/pci-assign.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index 3a904e8..9aa92a1 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -392,13 +392,14 @@ static uint8_t pci_find_cap_offset(PCIDevice *d, uint8_t cap, uint8_t start)
         pos += PCI_CAP_LIST_NEXT;
     }
     return 0;
 }
 
-static int assigned_dev_register_regions(PCIRegion *io_regions,
-                                         unsigned long regions_num,
-                                         AssignedDevice *pci_dev)
+static void assigned_dev_register_regions(PCIRegion *io_regions,
+                                          unsigned long regions_num,
+                                          AssignedDevice *pci_dev,
+                                          Error **errp)
 {
     uint32_t i;
     PCIRegion *cur_region = io_regions;
 
     for (i = 0; i < regions_num; i++, cur_region++) {
@@ -423,13 +424,13 @@ static int assigned_dev_register_regions(PCIRegion *io_regions,
                                                     cur_region->resource_fd,
                                                     (off_t)0);
 
             if (pci_dev->v_addrs[i].u.r_virtbase == MAP_FAILED) {
                 pci_dev->v_addrs[i].u.r_virtbase = NULL;
-                error_report("%s: Error: Couldn't mmap 0x%" PRIx64 "!",
-                             __func__, cur_region->base_addr);
-                return -1;
+                error_setg_errno(errp, errno, "Couldn't mmap 0x%" PRIx64 "!",
+                                 cur_region->base_addr);
+                return;
             }
 
             pci_dev->v_addrs[i].r_size = cur_region->size;
             pci_dev->v_addrs[i].e_size = 0;
 
@@ -494,11 +495,10 @@ static int assigned_dev_register_regions(PCIRegion *io_regions,
                              &pci_dev->v_addrs[i].container);
         }
     }
 
     /* success */
-    return 0;
 }
 
 static void get_real_id(const char *devpath, const char *idname, uint16_t *val,
                         Error **errp)
 {
@@ -1794,13 +1794,16 @@ static int assigned_initfn(struct PCIDevice *pci_dev)
             goto out;
         }
     }
 
     /* handle real device's MMIO/PIO BARs */
-    if (assigned_dev_register_regions(dev->real_device.regions,
-                                      dev->real_device.region_number,
-                                      dev)) {
+    assigned_dev_register_regions(dev->real_device.regions,
+                                  dev->real_device.region_number, dev,
+                                  &local_err);
+    if (local_err) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         goto out;
     }
 
     /* handle interrupt routing */
     e_intx = dev->dev.config[PCI_INTERRUPT_PIN] - 1;
-- 
1.8.3.1

[Qemu-devel] [PATCH 14/16] pci-assign: propagate errors from assign_device()

[Laszlo Ersek]

Also, change the return type to "void"; the function is static (with a
sole caller) and the negative errno values are not distinguished from each
other.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/i386/kvm/pci-assign.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index 9aa92a1..0fedca8 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -793,27 +793,27 @@ static char *assign_failed_examine(const AssignedDevice *dev)
 
 fail:
     return g_strdup("Couldn't find out why.");
 }
 
-static int assign_device(AssignedDevice *dev)
+static void assign_device(AssignedDevice *dev, Error **errp)
 {
     uint32_t flags = KVM_DEV_ASSIGN_ENABLE_IOMMU;
     int r;
 
     /* Only pass non-zero PCI segment to capable module */
     if (!kvm_check_extension(kvm_state, KVM_CAP_PCI_SEGMENT) &&
         dev->host.domain) {
-        error_report("Can't assign device inside non-zero PCI segment "
-                     "as this KVM module doesn't support it.");
-        return -ENODEV;
+        error_setg(errp, "Can't assign device inside non-zero PCI segment "
+                   "as this KVM module doesn't support it.");
+        return;
     }
 
     if (!kvm_check_extension(kvm_state, KVM_CAP_IOMMU)) {
-        error_report("No IOMMU found.  Unable to assign device \"%s\"",
-                     dev->dev.qdev.id);
-        return -ENODEV;
+        error_setg(errp, "No IOMMU found.  Unable to assign device \"%s\"",
+                   dev->dev.qdev.id);
+        return;
     }
 
     if (dev->features & ASSIGNED_DEVICE_SHARE_INTX_MASK &&
         kvm_has_intx_set_mask()) {
         flags |= KVM_DEV_ASSIGN_PCI_2_3;
@@ -824,22 +824,21 @@ static int assign_device(AssignedDevice *dev)
         switch (r) {
         case -EBUSY: {
             char *cause;
 
             cause = assign_failed_examine(dev);
-            error_report("Failed to assign device \"%s\" : %s\n%s",
-                         dev->dev.qdev.id, strerror(-r), cause);
+            error_setg_errno(errp, -r, "Failed to assign device \"%s\"\n%s",
+                             dev->dev.qdev.id, cause);
             g_free(cause);
             break;
         }
         default:
-            error_report("Failed to assign device \"%s\" : %s",
-                         dev->dev.qdev.id, strerror(-r));
+            error_setg_errno(errp, -r, "Failed to assign device \"%s\"",
+                             dev->dev.qdev.id);
             break;
         }
     }
-    return r;
 }
 
 static void verify_irqchip_in_kernel(Error **errp)
 {
     if (kvm_irqchip_in_kernel()) {
@@ -1810,12 +1809,14 @@ static int assigned_initfn(struct PCIDevice *pci_dev)
     dev->intpin = e_intx;
     dev->intx_route.mode = PCI_INTX_DISABLED;
     dev->intx_route.irq = -1;
 
     /* assign device to guest */
-    r = assign_device(dev);
-    if (r < 0) {
+    assign_device(dev, &local_err);
+    if (local_err) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         goto out;
     }
 
     /* assign legacy INTx to the device */
     r = assign_intx(dev);
-- 
1.8.3.1

[Qemu-devel] [PATCH 15/16] pci-assign: propagate errors from assign_intx()

[Laszlo Ersek]

Among the callers, only assigned_initfn() should set the  monitor's stored
error. Other callers may run in contexts where the monitor's stored error
makes no sense. For example:

assigned_dev_pci_write_config()
  assigned_dev_update_msix()
    assign_intx()

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/i386/kvm/pci-assign.c | 39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index 0fedca8..6891729 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -845,11 +845,11 @@ static void verify_irqchip_in_kernel(Error **errp)
         return;
     }
     error_setg(errp, "pci-assign requires KVM with in-kernel irqchip enabled");
 }
 
-static int assign_intx(AssignedDevice *dev)
+static int assign_intx(AssignedDevice *dev, Error **errp)
 {
     AssignedIRQType new_type;
     PCIINTxRoute intx_route;
     bool intx_host_msi;
     int r;
@@ -861,12 +861,11 @@ static int assign_intx(AssignedDevice *dev)
         return 0;
     }
 
     verify_irqchip_in_kernel(&local_err);
     if (local_err) {
-        error_report("%s", error_get_pretty(local_err));
-        error_free(local_err);
+        error_propagate(errp, local_err);
         return -ENOTSUP;
     }
 
     pci_device_set_intx_routing_notifier(&dev->dev,
                                          assigned_dev_update_irq_routing);
@@ -925,14 +924,15 @@ retry:
                          "using MSI instead");
             error_printf("Some devices do not work properly in this mode.\n");
             dev->features |= ASSIGNED_DEVICE_PREFER_MSI_MASK;
             goto retry;
         }
-        error_report("Failed to assign irq for \"%s\": %s",
-                     dev->dev.qdev.id, strerror(-r));
-        error_report("Perhaps you are assigning a device "
-                     "that shares an IRQ with another device?");
+        error_setg_errno(errp, -r,
+                         "Failed to assign irq for \"%s\"\n"
+                         "Perhaps you are assigning a device "
+                         "that shares an IRQ with another device?",
+                         dev->dev.qdev.id);
         return r;
     }
 
     dev->intx_route = intx_route;
     dev->assigned_irq_type = new_type;
@@ -954,12 +954,15 @@ static void assigned_dev_update_irq_routing(PCIDevice *dev)
 {
     AssignedDevice *assigned_dev = DO_UPCAST(AssignedDevice, dev, dev);
     Error *err = NULL;
     int r;
 
-    r = assign_intx(assigned_dev);
+    r = assign_intx(assigned_dev, &err);
     if (r < 0) {
+        error_report("%s", error_get_pretty(err));
+        error_free(err);
+        err = NULL;
         qdev_unplug(&dev->qdev, &err);
         assert(!err);
     }
 }
 
@@ -1006,11 +1009,17 @@ static void assigned_dev_update_msi(PCIDevice *pci_dev)
 
         assigned_dev->intx_route.mode = PCI_INTX_DISABLED;
         assigned_dev->intx_route.irq = -1;
         assigned_dev->assigned_irq_type = ASSIGNED_IRQ_MSI;
     } else {
-        assign_intx(assigned_dev);
+        Error *local_err = NULL;
+
+        assign_intx(assigned_dev, &local_err);
+        if (local_err) {
+            error_report("%s", error_get_pretty(local_err));
+            error_free(local_err);
+        }
     }
 }
 
 static void assigned_dev_update_msi_msg(PCIDevice *pci_dev)
 {
@@ -1148,11 +1157,17 @@ static void assigned_dev_update_msix(PCIDevice *pci_dev)
         }
         assigned_dev->intx_route.mode = PCI_INTX_DISABLED;
         assigned_dev->intx_route.irq = -1;
         assigned_dev->assigned_irq_type = ASSIGNED_IRQ_MSIX;
     } else {
-        assign_intx(assigned_dev);
+        Error *local_err = NULL;
+
+        assign_intx(assigned_dev, &local_err);
+        if (local_err) {
+            error_report("%s", error_get_pretty(local_err));
+            error_free(local_err);
+        }
     }
 }
 
 static uint32_t assigned_dev_pci_read_config(PCIDevice *pci_dev,
                                              uint32_t address, int len)
@@ -1817,12 +1832,14 @@ static int assigned_initfn(struct PCIDevice *pci_dev)
         error_free(local_err);
         goto out;
     }
 
     /* assign legacy INTx to the device */
-    r = assign_intx(dev);
+    r = assign_intx(dev, &local_err);
     if (r < 0) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         goto assigned_out;
     }
 
     assigned_dev_load_option_rom(dev);
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 16/16] pci-assign: assigned_initfn(): set monitor error in common error handler

[Laszlo Ersek]

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/i386/kvm/pci-assign.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index 6891729..e55421a 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -1754,18 +1754,18 @@ static int assigned_initfn(struct PCIDevice *pci_dev)
     uint8_t e_intx;
     int r;
     Error *local_err = NULL;
 
     if (!kvm_enabled()) {
-        error_report("pci-assign: error: requires KVM support");
-        return -1;
+        error_setg(&local_err, "pci-assign requires KVM support");
+        goto exit_with_error;
     }
 
     if (!dev->host.domain && !dev->host.bus && !dev->host.slot &&
         !dev->host.function) {
-        error_report("pci-assign: error: no host device specified");
-        return -1;
+        error_setg(&local_err, "no host device specified");
+        goto exit_with_error;
     }
 
     /*
      * Set up basic config space access control. Will be further refined during
      * device initialization.
@@ -1786,38 +1786,30 @@ static int assigned_initfn(struct PCIDevice *pci_dev)
     memcpy(dev->emulate_config_write, dev->emulate_config_read,
            sizeof(dev->emulate_config_read));
 
     get_real_device(dev, &local_err);
     if (local_err) {
-        qerror_report_err(local_err);
-        error_free(local_err);
         goto out;
     }
 
     if (assigned_device_pci_cap_init(pci_dev, &local_err) < 0) {
-        qerror_report_err(local_err);
-        error_free(local_err);
         goto out;
     }
 
     /* intercept MSI-X entry page in the MMIO */
     if (dev->cap.available & ASSIGNED_DEVICE_CAP_MSIX) {
         assigned_dev_register_msix_mmio(dev, &local_err);
         if (local_err) {
-            qerror_report_err(local_err);
-            error_free(local_err);
             goto out;
         }
     }
 
     /* handle real device's MMIO/PIO BARs */
     assigned_dev_register_regions(dev->real_device.regions,
                                   dev->real_device.region_number, dev,
                                   &local_err);
     if (local_err) {
-        qerror_report_err(local_err);
-        error_free(local_err);
         goto out;
     }
 
     /* handle interrupt routing */
     e_intx = dev->dev.config[PCI_INTERRUPT_PIN] - 1;
@@ -1826,20 +1818,16 @@ static int assigned_initfn(struct PCIDevice *pci_dev)
     dev->intx_route.irq = -1;
 
     /* assign device to guest */
     assign_device(dev, &local_err);
     if (local_err) {
-        qerror_report_err(local_err);
-        error_free(local_err);
         goto out;
     }
 
     /* assign legacy INTx to the device */
     r = assign_intx(dev, &local_err);
     if (r < 0) {
-        qerror_report_err(local_err);
-        error_free(local_err);
         goto assigned_out;
     }
 
     assigned_dev_load_option_rom(dev);
 
@@ -1847,12 +1835,18 @@ static int assigned_initfn(struct PCIDevice *pci_dev)
 
     return 0;
 
 assigned_out:
     deassign_device(dev);
+
 out:
     free_assigned_device(dev);
+
+exit_with_error:
+    assert(local_err);
+    qerror_report_err(local_err);
+    error_free(local_err);
     return -1;
 }
 
 static void assigned_exitfn(struct PCIDevice *pci_dev)
 {
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 01/16] cutils: tighten qemu_parse_fd()

[Eric Blake]

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

On 04/10/2014 02:24 AM, Laszlo Ersek wrote:
> qemu_parse_fd() used to handle at least the following strings incorrectly:
> o "-2":         simply let through
> o "2147483648": returned as LONG_MAX==INT_MAX on ILP32 (with ERANGE
>                 ignored); implementation-defined behavior on LP64
> 
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  util/cutils.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)

I still think qemu should follow libvirt's lead of wrapping ALL uses of
strto*l behind sane wrappers, since this is not the only place in the
code base affected by misuse of the function - but that's a story for
another day.

>  
> +    errno = 0;
>      fd = strtol(param, &endptr, 10);
> -    if (*endptr || (fd == 0 && param == endptr)) {
> +    if (param == endptr /* no conversion performed */                    ||
> +        errno != 0      /* not representable as long; possibly others */ ||
> +        *endptr != '\0' /* final string not empty */                     ||
> +        fd < 0          /* invalid as file descriptor */                 ||
> +        fd > INT_MAX    /* not representable as int */) {
>          return -1;

Your comments make it particularly obvious that YOU know how to properly
use this function, and hopefully teach future readers. :)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 00/16] PCI device assignment: improve error reporting over QMP

[Luiz Capitulino]

On Wed, 30 Apr 2014 15:48:13 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> Hi Luiz,
> 
> On 04/10/14 10:24, Laszlo Ersek wrote:
> > This is for <https://bugzilla.redhat.com/show_bug.cgi?id=616415>.
> > 
> > In general, we want to propagate non-fatal (ie. non-exit()ing,
> > non-abort()ing) errors to the QMP caller, rather than printing them
> > on-site. The series focuses on errors raised at PCI assignment time (ie.
> > reachable from assigned_initfn()), other errors are not converted.
> > 
> > Errors are not propagated through assigned_initfn(); let's wait for
> > someone else to convert "kvm-pci-assign" from qdev to QOM. The series is
> > nonetheless an improvement, because the forwarding of initialization
> > errors now stops just before device_realize(). We set the
> > stored/persistent monitor error there.
> > 
> > Informative and warning messages (that report about circumstances that
> > don't break the assignment operation) cannot terminate in
> > qerror_report_err(), because that would set the persistent monitor
> > error, breaking the high level (QMP) operation.
> > 
> > A call graph rooted in assigned_initfn() was generated with cflow.
> > Leaves that could never fail were removed from the graph (and this
> > property similarly propagated upwards as far as possible).
> > 
> > The patchset loosely follows a bottom-up algorithm on this calltree. Any
> > leaf that reports an error internally and returns a related failure is
> > converted to "throw" an Error structure instead. All direct callers of
> > the converted leaf are addressed at once, in the same patch, to consume
> > the error (and they become the new leaves gradually).
> > 
> > When the leaf to be converted is also called outside of
> > "hw/i386/kvm/pci-assign.c", the conversion keeps a compatibility
> > function under the original name, so that conversion of callers
> > unrelated to PCI assignment can be deferred.
> > 
> > Reviewers should copy the call graph to a text file, and mark, as the
> > series progresses, fully converted functions. (Ie. functions that now
> > report their terminating error messages with Error objects only.)
> > 
> >   assigned_initfn()
> >       error_report()
> >       get_real_device()
> >           monitor_handle_fd_param()
> >           error_report()
> >           get_real_vendor_id()
> >               get_real_id()
> >                   error_report()
> >           get_real_device_id()
> >               get_real_id()
> >                   error_report()
> >       assigned_device_pci_cap_init()
> >           check_irqchip_in_kernel()
> >               error_report()
> >           pci_add_capability()
> >           error_report()
> >       assigned_dev_register_msix_mmio()
> >           error_report()
> >       assigned_dev_register_regions()
> >           error_report()
> >       assign_device()
> >           error_report()
> >           assign_failed_examine()
> >               get_real_vendor_id()
> >                   get_real_id()
> >                       error_report()
> >               get_real_device_id()
> >                   get_real_id()
> >                       error_report()
> >               error_printf()
> >               error_report()
> >       assign_intx()
> >           check_irqchip_in_kernel()
> >               error_report()
> >           error_report()
> >           error_printf()
> > 
> > Laszlo Ersek (16):
> >   cutils: tighten qemu_parse_fd()
> >   monitor: add Error-propagating monitor_handle_fd_param2()
> >   pci-assign: accept Error from monitor_handle_fd_param2()
> >   pci-assign: make assign_failed_examine() just format the cause
> >   pci-assign: propagate errors from get_real_id()
> >   pci-assign: propagate Error from check_irqchip_in_kernel()
> >   pci: add Error-propagating pci_add_capability2()
> >   pci-assign: accept Error from pci_add_capability2()
> >   pci-assign: assignment should fail if we can't read config space
> >   pci-assign: propagate errors from get_real_device()
> >   pci-assign: propagate errors from assigned_device_pci_cap_init()
> >   pci-assign: propagate errors from assigned_dev_register_msix_mmio()
> >   pci-assign: propagate errors from assigned_dev_register_regions()
> >   pci-assign: propagate errors from assign_device()
> >   pci-assign: propagate errors from assign_intx()
> >   pci-assign: assigned_initfn(): set monitor error in common error
> >     handler
> > 
> >  include/hw/pci/pci.h      |   4 +
> >  include/monitor/monitor.h |   1 +
> >  hw/i386/kvm/pci-assign.c  | 273 ++++++++++++++++++++++++++++------------------
> >  hw/pci/pci.c              |  32 +++++-
> >  monitor.c                 |  29 ++++-
> >  util/cutils.c             |  13 ++-
> >  6 files changed, 232 insertions(+), 120 deletions(-)
> > 
> 
> I forgot to CC you on this.

Applied to the qmp branch, thanks.

> 
> Thanks
> Laszlo
> 

Re: [Qemu-devel] [PATCH 16/16] pci-assign: assigned_initfn(): set monitor error in common error handler

[Luiz Capitulino]

On Wed, 30 Apr 2014 02:35:28 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 04/30/14 01:25, Eric Blake wrote:
> > On 04/10/2014 02:24 AM, Laszlo Ersek wrote:
> >> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> >> ---
> >>  hw/i386/kvm/pci-assign.c | 26 ++++++++++----------------
> >>  1 file changed, 10 insertions(+), 16 deletions(-)
> >>
> > 
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > 
> 
> Your (surprisingly lenient! :)) review is greatly appreciated! Let's see
> what maintainers say.

Eric is a maintainer too.

Re: [Qemu-devel] [PATCH 16/16] pci-assign: assigned_initfn(): set monitor error in common error handler

[Laszlo Ersek]

On 05/07/14 15:52, Luiz Capitulino wrote:
> On Wed, 30 Apr 2014 02:35:28 +0200
> Laszlo Ersek <lersek@redhat.com> wrote:
> 
>> On 04/30/14 01:25, Eric Blake wrote:
>>> On 04/10/2014 02:24 AM, Laszlo Ersek wrote:
>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>>> ---
>>>>  hw/i386/kvm/pci-assign.c | 26 ++++++++++----------------
>>>>  1 file changed, 10 insertions(+), 16 deletions(-)
>>>>
>>>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>
>>
>> Your (surprisingly lenient! :)) review is greatly appreciated! Let's see
>> what maintainers say.
> 
> Eric is a maintainer too.

(I'm aware; it was him referring to maintainers other than himself.)

Laszlo

Re: [Qemu-devel] [PATCH 00/16] PCI device assignment: improve error reporting over QMP

[Laszlo Ersek]

On 05/07/14 15:52, Luiz Capitulino wrote:
> On Wed, 30 Apr 2014 15:48:13 +0200
> Laszlo Ersek <lersek@redhat.com> wrote:
> 
>> Hi Luiz,
>>
>> On 04/10/14 10:24, Laszlo Ersek wrote:
>>> This is for <https://bugzilla.redhat.com/show_bug.cgi?id=616415>.
>>>
>>> In general, we want to propagate non-fatal (ie. non-exit()ing,
>>> non-abort()ing) errors to the QMP caller, rather than printing them
>>> on-site. The series focuses on errors raised at PCI assignment time (ie.
>>> reachable from assigned_initfn()), other errors are not converted.
>>>
>>> Errors are not propagated through assigned_initfn(); let's wait for
>>> someone else to convert "kvm-pci-assign" from qdev to QOM. The series is
>>> nonetheless an improvement, because the forwarding of initialization
>>> errors now stops just before device_realize(). We set the
>>> stored/persistent monitor error there.
>>>
>>> Informative and warning messages (that report about circumstances that
>>> don't break the assignment operation) cannot terminate in
>>> qerror_report_err(), because that would set the persistent monitor
>>> error, breaking the high level (QMP) operation.
>>>
>>> A call graph rooted in assigned_initfn() was generated with cflow.
>>> Leaves that could never fail were removed from the graph (and this
>>> property similarly propagated upwards as far as possible).
>>>
>>> The patchset loosely follows a bottom-up algorithm on this calltree. Any
>>> leaf that reports an error internally and returns a related failure is
>>> converted to "throw" an Error structure instead. All direct callers of
>>> the converted leaf are addressed at once, in the same patch, to consume
>>> the error (and they become the new leaves gradually).
>>>
>>> When the leaf to be converted is also called outside of
>>> "hw/i386/kvm/pci-assign.c", the conversion keeps a compatibility
>>> function under the original name, so that conversion of callers
>>> unrelated to PCI assignment can be deferred.
>>>
>>> Reviewers should copy the call graph to a text file, and mark, as the
>>> series progresses, fully converted functions. (Ie. functions that now
>>> report their terminating error messages with Error objects only.)
>>>
>>>   assigned_initfn()
>>>       error_report()
>>>       get_real_device()
>>>           monitor_handle_fd_param()
>>>           error_report()
>>>           get_real_vendor_id()
>>>               get_real_id()
>>>                   error_report()
>>>           get_real_device_id()
>>>               get_real_id()
>>>                   error_report()
>>>       assigned_device_pci_cap_init()
>>>           check_irqchip_in_kernel()
>>>               error_report()
>>>           pci_add_capability()
>>>           error_report()
>>>       assigned_dev_register_msix_mmio()
>>>           error_report()
>>>       assigned_dev_register_regions()
>>>           error_report()
>>>       assign_device()
>>>           error_report()
>>>           assign_failed_examine()
>>>               get_real_vendor_id()
>>>                   get_real_id()
>>>                       error_report()
>>>               get_real_device_id()
>>>                   get_real_id()
>>>                       error_report()
>>>               error_printf()
>>>               error_report()
>>>       assign_intx()
>>>           check_irqchip_in_kernel()
>>>               error_report()
>>>           error_report()
>>>           error_printf()
>>>
>>> Laszlo Ersek (16):
>>>   cutils: tighten qemu_parse_fd()
>>>   monitor: add Error-propagating monitor_handle_fd_param2()
>>>   pci-assign: accept Error from monitor_handle_fd_param2()
>>>   pci-assign: make assign_failed_examine() just format the cause
>>>   pci-assign: propagate errors from get_real_id()
>>>   pci-assign: propagate Error from check_irqchip_in_kernel()
>>>   pci: add Error-propagating pci_add_capability2()
>>>   pci-assign: accept Error from pci_add_capability2()
>>>   pci-assign: assignment should fail if we can't read config space
>>>   pci-assign: propagate errors from get_real_device()
>>>   pci-assign: propagate errors from assigned_device_pci_cap_init()
>>>   pci-assign: propagate errors from assigned_dev_register_msix_mmio()
>>>   pci-assign: propagate errors from assigned_dev_register_regions()
>>>   pci-assign: propagate errors from assign_device()
>>>   pci-assign: propagate errors from assign_intx()
>>>   pci-assign: assigned_initfn(): set monitor error in common error
>>>     handler
>>>
>>>  include/hw/pci/pci.h      |   4 +
>>>  include/monitor/monitor.h |   1 +
>>>  hw/i386/kvm/pci-assign.c  | 273 ++++++++++++++++++++++++++++------------------
>>>  hw/pci/pci.c              |  32 +++++-
>>>  monitor.c                 |  29 ++++-
>>>  util/cutils.c             |  13 ++-
>>>  6 files changed, 232 insertions(+), 120 deletions(-)
>>>
>>
>> I forgot to CC you on this.
> 
> Applied to the qmp branch, thanks.

Awesome, thank you.
Laszlo