[PATCH v2 0/2] hw/misc/ivshmem-pci: Improve error handling

[PATCH v2 0/2] hw/misc/ivshmem-pci: Improve error handling

[David Hamilton]

This series addresses two error handling issues in the ivshmem
PCI device:

1. Handle the return value of kvm_irqchip_add_irqfd_notifier_gsi()
   which was being silently ignored.

2. Check for errors from setup_interrupt() in process_msg_connect()
   before continuing to ioeventfd setup.

Changes from v1:
- Made setup_interrupt() return bool per QEMU conventions (Markus)
- Simplified errp check using bool return, dropped ERRP_GUARD()
- Dropped vector number from error message

David Hamilton (2):
  hw/misc/ivshmem-pci: Handle error from
    kvm_irqchip_add_irqfd_notifier_gsi()
  hw/misc/ivshmem-pci: Check error after setup_interrupt() in
    process_msg_connect()

 hw/misc/ivshmem-pci.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

-- 
2.50.1 (Apple Git-155)

[PATCH v2 1/2] hw/misc/ivshmem-pci: Handle error from kvm_irqchip_add_irqfd_notifier_gsi()

[David Hamilton]

The return value of kvm_irqchip_add_irqfd_notifier_gsi() was being
ignored. Propagate the error to the caller via errp.

Also change setup_interrupt() to return bool to follow QEMU error
handling conventions, making error checks at call sites simpler.

Resolves the TODO comment at the call site.

Signed-off-by: David Hamilton <dahamilt0@gmail.com>
---
 hw/misc/ivshmem-pci.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/hw/misc/ivshmem-pci.c b/hw/misc/ivshmem-pci.c
index a3a43f53bd..c987eebb98 100644
--- a/hw/misc/ivshmem-pci.c
+++ b/hw/misc/ivshmem-pci.c
@@ -442,13 +442,14 @@ static void ivshmem_add_kvm_msi_virq(IVShmemState *s, int vector,
     s->msi_vectors[vector].pdev = pdev;
 }
 
-static void setup_interrupt(IVShmemState *s, int vector, Error **errp)
+static bool setup_interrupt(IVShmemState *s, int vector, Error **errp)
 {
     EventNotifier *n = &s->peers[s->vm_id].eventfds[vector];
     bool with_irqfd = kvm_msi_via_irqfd_enabled() &&
         ivshmem_has_feature(s, IVSHMEM_MSI);
     PCIDevice *pdev = PCI_DEVICE(s);
     Error *err = NULL;
+    int ret;
 
     IVSHMEM_DPRINTF("setting up interrupt for vector: %d\n", vector);
 
@@ -460,18 +461,22 @@ static void setup_interrupt(IVShmemState *s, int vector, Error **errp)
         ivshmem_add_kvm_msi_virq(s, vector, &err);
         if (err) {
             error_propagate(errp, err);
-            return;
+            return false;
         }
 
         if (!msix_is_masked(pdev, vector)) {
-            kvm_irqchip_add_irqfd_notifier_gsi(kvm_state, n, NULL,
+            ret = kvm_irqchip_add_irqfd_notifier_gsi(kvm_state, n, NULL,
                                                s->msi_vectors[vector].virq);
-            /* TODO handle error */
+            if (ret < 0) {
+                error_setg(errp, "Failed to configure irqfd notifier");
+                return false;
+            }
         }
     } else {
         /* it will be delayed until msix is enabled, in write_config */
         IVSHMEM_DPRINTF("with irqfd, delayed until msix enabled\n");
     }
+    return true;
 }
 
 static void process_msg_shmem(IVShmemState *s, int fd, Error **errp)
-- 
2.50.1 (Apple Git-155)

Re: [PATCH v2 1/2] hw/misc/ivshmem-pci: Handle error from kvm_irqchip_add_irqfd_notifier_gsi()

[Markus Armbruster]

David Hamilton <dahamilt0@gmail.com> writes:

> The return value of kvm_irqchip_add_irqfd_notifier_gsi() was being
> ignored. Propagate the error to the caller via errp.
>
> Also change setup_interrupt() to return bool to follow QEMU error
> handling conventions, making error checks at call sites simpler.
>
> Resolves the TODO comment at the call site.
>
> Signed-off-by: David Hamilton <dahamilt0@gmail.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[PATCH v2 2/2] hw/misc/ivshmem-pci: Check error after setup_interrupt() in process_msg_connect()

[David Hamilton]

setup_interrupt() can fail and report via errp, but
process_msg_connect() was ignoring the error and continuing
to ivshmem_add_eventfd(). Check the bool return to avoid
operating on a vector whose interrupt setup failed.

Resolves the TODO comment at the call site.

Signed-off-by: David Hamilton <dahamilt0@gmail.com>
---
 hw/misc/ivshmem-pci.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/misc/ivshmem-pci.c b/hw/misc/ivshmem-pci.c
index c987eebb98..f1262fa5f7 100644
--- a/hw/misc/ivshmem-pci.c
+++ b/hw/misc/ivshmem-pci.c
@@ -553,8 +553,9 @@ static void process_msg_connect(IVShmemState *s, uint16_t posn, int fd,
     }
 
     if (posn == s->vm_id) {
-        setup_interrupt(s, vector, errp);
-        /* TODO do we need to handle the error? */
+        if (!setup_interrupt(s, vector, errp)) {
+            return;
+        }
     }
 
     if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD)) {
-- 
2.50.1 (Apple Git-155)

Re: [PATCH v2 2/2] hw/misc/ivshmem-pci: Check error after setup_interrupt() in process_msg_connect()

[Markus Armbruster]

David Hamilton <dahamilt0@gmail.com> writes:

> setup_interrupt() can fail and report via errp, but
> process_msg_connect() was ignoring the error and continuing
> to ivshmem_add_eventfd(). Check the bool return to avoid
> operating on a vector whose interrupt setup failed.
>
> Resolves the TODO comment at the call site.
>
> Signed-off-by: David Hamilton <dahamilt0@gmail.com>
> ---
>  hw/misc/ivshmem-pci.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/misc/ivshmem-pci.c b/hw/misc/ivshmem-pci.c
> index c987eebb98..f1262fa5f7 100644
> --- a/hw/misc/ivshmem-pci.c
> +++ b/hw/misc/ivshmem-pci.c
> @@ -553,8 +553,9 @@ static void process_msg_connect(IVShmemState *s, uint16_t posn, int fd,
>      }
>  
>      if (posn == s->vm_id) {
> -        setup_interrupt(s, vector, errp);
> -        /* TODO do we need to handle the error? */
> +        if (!setup_interrupt(s, vector, errp)) {

Any cleanup needed here?  Error returns above close(fd).

> +            return;
> +        }
>      }
>  
>      if (ivshmem_has_feature(s, IVSHMEM_IOEVENTFD)) {