[PATCH v2 0/3] failover: trivial cleanup and fix

[PATCH v2 0/3] failover: trivial cleanup and fix

[Laurent Vivier]

The first patch removes a duplicate assignment to
allow_unplug_during_migration, and simplify the code.

The second patch fixes a dangling object in failover_add_primary()
that prevents to cleanup the internal structure after the object has
been unplugged.

v2: add a third patch

The third patch reports a warning if the failover device is not found

Laurent Vivier (3):
  pci: cleanup failover sanity check
  virtio-net: add missing object_unref()
  failover: really display a warning when the primary device is not
    found

 hw/net/virtio-net.c | 18 +++++++++---------
 hw/pci/pci.c        |  6 ++----
 2 files changed, 11 insertions(+), 13 deletions(-)

-- 
2.29.2

[PATCH v2 1/3] pci: cleanup failover sanity check

[Laurent Vivier]

Commit a1190ab628 has added a "allow_unplug_during_migration = true" at
the end of the main "if" block, so it is not needed to set it anymore
in the previous checking.

Remove it, to have only sub-ifs that check for needed conditions and exit
if one fails.

Fixes: 4f5b6a05a4e7 ("pci: add option for net failover")
Fixes: a1190ab628c0 ("migration: allow unplug during migration for failover devices")
Cc: jfreimann@redhat.com
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
Reviewed-by: Jens Freimann <jfreimann@redhat.com>
---
 hw/pci/pci.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 512e9042ffae..ecb7aa31fabd 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2120,10 +2120,8 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
             pci_qdev_unrealize(DEVICE(pci_dev));
             return;
         }
-        if (!(pci_dev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION)
-            && (PCI_FUNC(pci_dev->devfn) == 0)) {
-            qdev->allow_unplug_during_migration = true;
-        } else {
+        if ((pci_dev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION)
+            || (PCI_FUNC(pci_dev->devfn) != 0)) {
             error_setg(errp, "failover: primary device must be in its own "
                               "PCI slot");
             pci_qdev_unrealize(DEVICE(pci_dev));
-- 
2.29.2

Re: [PATCH v2 1/3] pci: cleanup failover sanity check

[Jason Wang]

On 2021/2/11 上午1:45, Laurent Vivier wrote:
> Commit a1190ab628 has added a "allow_unplug_during_migration = true" at
> the end of the main "if" block, so it is not needed to set it anymore
> in the previous checking.
>
> Remove it, to have only sub-ifs that check for needed conditions and exit
> if one fails.
>
> Fixes: 4f5b6a05a4e7 ("pci: add option for net failover")
> Fixes: a1190ab628c0 ("migration: allow unplug during migration for failover devices")
> Cc: jfreimann@redhat.com
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> Reviewed-by: Jens Freimann <jfreimann@redhat.com>
> ---
>   hw/pci/pci.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)


Acked-by: Jason Wang <jasowang@redhat.com>


>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 512e9042ffae..ecb7aa31fabd 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2120,10 +2120,8 @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp)
>               pci_qdev_unrealize(DEVICE(pci_dev));
>               return;
>           }
> -        if (!(pci_dev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION)
> -            && (PCI_FUNC(pci_dev->devfn) == 0)) {
> -            qdev->allow_unplug_during_migration = true;
> -        } else {
> +        if ((pci_dev->cap_present & QEMU_PCI_CAP_MULTIFUNCTION)
> +            || (PCI_FUNC(pci_dev->devfn) != 0)) {
>               error_setg(errp, "failover: primary device must be in its own "
>                                 "PCI slot");
>               pci_qdev_unrealize(DEVICE(pci_dev));

[PATCH v2 2/3] virtio-net: add missing object_unref()

[Laurent Vivier]

failover_add_primary() calls qdev_device_add() and doesn't unref
the device. Because of that, when the device is unplugged a reference
is remaining and prevents the cleanup of the object.

This prevents to be able to plugin back the failover primary device,
with errors like:

  (qemu) device_add vfio-pci,host=0000:41:00.0,id=hostdev0,bus=root.3,failover_pair_id=net0
  (qemu) device_del hostdev0

We can check with "info qtree" and "info pci" that the device has been removed, and then:

  (qemu) device_add vfio-pci,host=0000:41:00.0,id=hostdev1,bus=root.3,failover_pair_id=net0
  Error: vfio 0000:41:00.0: device is already attached
  (qemu) device_add vfio-pci,host=0000:41:00.0,id=hostdev0,bus=root.3,failover_pair_id=net0
  qemu-kvm: Duplicate ID 'hostdev0' for device

Fixes: 21e8709b29cd ("failover: Remove primary_dev member")
Cc: quintela@redhat.com
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
Reviewed-by: Jens Freimann <jfreimann@redhat.com>
---
 hw/net/virtio-net.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 5150f295e8c5..1c5af08dc556 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -862,6 +862,8 @@ static void failover_add_primary(VirtIONet *n, Error **errp)
         dev = qdev_device_add(opts, &err);
         if (err) {
             qemu_opts_del(opts);
+        } else {
+            object_unref(OBJECT(dev));
         }
     } else {
         error_setg(errp, "Primary device not found");
-- 
2.29.2

Re: [PATCH v2 2/3] virtio-net: add missing object_unref()

[Jason Wang]

On 2021/2/11 上午1:45, Laurent Vivier wrote:
> failover_add_primary() calls qdev_device_add() and doesn't unref
> the device. Because of that, when the device is unplugged a reference
> is remaining and prevents the cleanup of the object.
>
> This prevents to be able to plugin back the failover primary device,
> with errors like:
>
>    (qemu) device_add vfio-pci,host=0000:41:00.0,id=hostdev0,bus=root.3,failover_pair_id=net0
>    (qemu) device_del hostdev0
>
> We can check with "info qtree" and "info pci" that the device has been removed, and then:
>
>    (qemu) device_add vfio-pci,host=0000:41:00.0,id=hostdev1,bus=root.3,failover_pair_id=net0
>    Error: vfio 0000:41:00.0: device is already attached
>    (qemu) device_add vfio-pci,host=0000:41:00.0,id=hostdev0,bus=root.3,failover_pair_id=net0
>    qemu-kvm: Duplicate ID 'hostdev0' for device
>
> Fixes: 21e8709b29cd ("failover: Remove primary_dev member")
> Cc: quintela@redhat.com
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> Reviewed-by: Jens Freimann <jfreimann@redhat.com>


Acked-by: Jason Wang <jasowang@redhat.com>


> ---
>   hw/net/virtio-net.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 5150f295e8c5..1c5af08dc556 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -862,6 +862,8 @@ static void failover_add_primary(VirtIONet *n, Error **errp)
>           dev = qdev_device_add(opts, &err);
>           if (err) {
>               qemu_opts_del(opts);
> +        } else {
> +            object_unref(OBJECT(dev));
>           }
>       } else {
>           error_setg(errp, "Primary device not found");

[PATCH v2 3/3] failover: really display a warning when the primary device is not found

[Laurent Vivier]

In failover_add_primary(), we search the id of the failover device by
scanning the list of the devices in the opts list to find a device with
a failover_pair_id equals to the id of the virtio-net device.

If the failover_pair_id is not found, QEMU ignores the primary
device silently (which also means it will not be hidden and
it will be enabled directly at boot).

After that, we search the id in the opts list to do a qdev_device_add()
with it. The device will be always found as otherwise we had exited
before, and thus the warning is never displayed.

Fix that by moving the error report to the first exit condition.
Also add a g_assert() to be sure the compiler will not complain
about a possibly NULL pointer.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 hw/net/virtio-net.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 1c5af08dc556..439f823b190c 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -855,21 +855,19 @@ static void failover_add_primary(VirtIONet *n, Error **errp)
 
     id = failover_find_primary_device_id(n);
     if (!id) {
+        error_setg(errp, "Primary device not found");
+        error_append_hint(errp, "Virtio-net failover will not work. Make "
+                          "sure primary device has parameter"
+                          " failover_pair_id=%s\n", n->netclient_name);
         return;
     }
     opts = qemu_opts_find(qemu_find_opts("device"), id);
-    if (opts) {
-        dev = qdev_device_add(opts, &err);
-        if (err) {
-            qemu_opts_del(opts);
-        } else {
-            object_unref(OBJECT(dev));
-        }
+    g_assert(opts); /* cannot be NULL because id was found using opts list */
+    dev = qdev_device_add(opts, &err);
+    if (err) {
+        qemu_opts_del(opts);
     } else {
-        error_setg(errp, "Primary device not found");
-        error_append_hint(errp, "Virtio-net failover will not work. Make "
-                          "sure primary device has parameter"
-                          " failover_pair_id=<virtio-net-id>\n");
+        object_unref(OBJECT(dev));
     }
     error_propagate(errp, err);
 }
-- 
2.29.2

Re: [PATCH v2 3/3] failover: really display a warning when the primary device is not found

[Jason Wang]

On 2021/2/11 上午1:45, Laurent Vivier wrote:
> In failover_add_primary(), we search the id of the failover device by
> scanning the list of the devices in the opts list to find a device with
> a failover_pair_id equals to the id of the virtio-net device.
>
> If the failover_pair_id is not found, QEMU ignores the primary
> device silently (which also means it will not be hidden and
> it will be enabled directly at boot).
>
> After that, we search the id in the opts list to do a qdev_device_add()
> with it. The device will be always found as otherwise we had exited
> before, and thus the warning is never displayed.
>
> Fix that by moving the error report to the first exit condition.
> Also add a g_assert() to be sure the compiler will not complain
> about a possibly NULL pointer.
>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>


Acked-by: Jason Wang <jasowang@redhat.com>


> ---
>   hw/net/virtio-net.c | 20 +++++++++-----------
>   1 file changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 1c5af08dc556..439f823b190c 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -855,21 +855,19 @@ static void failover_add_primary(VirtIONet *n, Error **errp)
>   
>       id = failover_find_primary_device_id(n);
>       if (!id) {
> +        error_setg(errp, "Primary device not found");
> +        error_append_hint(errp, "Virtio-net failover will not work. Make "
> +                          "sure primary device has parameter"
> +                          " failover_pair_id=%s\n", n->netclient_name);
>           return;
>       }
>       opts = qemu_opts_find(qemu_find_opts("device"), id);
> -    if (opts) {
> -        dev = qdev_device_add(opts, &err);
> -        if (err) {
> -            qemu_opts_del(opts);
> -        } else {
> -            object_unref(OBJECT(dev));
> -        }
> +    g_assert(opts); /* cannot be NULL because id was found using opts list */
> +    dev = qdev_device_add(opts, &err);
> +    if (err) {
> +        qemu_opts_del(opts);
>       } else {
> -        error_setg(errp, "Primary device not found");
> -        error_append_hint(errp, "Virtio-net failover will not work. Make "
> -                          "sure primary device has parameter"
> -                          " failover_pair_id=<virtio-net-id>\n");
> +        object_unref(OBJECT(dev));
>       }
>       error_propagate(errp, err);
>   }