[PATCH 0/2] failover: fix a regression introduced by JSON'ification of -device

[PATCH 0/2] failover: fix a regression introduced by JSON'ification of -device

[Laurent Vivier]

Kevin's series "qdev: Add JSON -device" has introduced a regression
in failover by removing the QemuOpts parameter.

This series fixes that (see PATCH 1) and also makes some cleanup
in the hide_device function caller to remove the failover specific
code from qdev_device_add_from_qdict() and clarify the fact that
a primary device must have an id.

Laurent Vivier (2):
  failover: fix a regression introduced by JSON'ification of -device
  qdev/qbus: remove failover specific code

 hw/net/virtio-net.c    | 36 +++++++++++++++++++++++++++++-------
 softmmu/qdev-monitor.c | 18 ++++++------------
 2 files changed, 35 insertions(+), 19 deletions(-)

-- 
2.31.1

[PATCH 1/2] failover: fix a regression introduced by JSON'ification of -device

[Laurent Vivier]

The hide_device helper can be called several times for the same
devices as it shouldn't change any state and should only return an
information.

But not to rely anymore on QemuOpts we have introduced a new field
to store the parameters of the device and don't allow to update it
once it is done.

And as the function is called several times, we ends with:

  warning: Cannot attach more than one primary device to 'virtio0'

That is not only a warning as it prevents to hide the device and breaks
failover.

Fix that by checking the device id.

Now, we fail only if the virtio-net device is really used by two different
devices, for instance:

   -device virtio-net-pci,id=virtio0,failover=on,... \
   -device vfio-pci,id=hostdev0,failover_pair_id=virtio0,... \
   -device e1000e,id=e1000e0,failover_pair_id=virtio0,... \

will exit with:

  Cannot attach more than one primary device to 'virtio0': 'hostdev0' and 'e1000e0'

Fixes: 259a10dbcb4f ("virtio-net: Store failover primary opts pointer locally")
Cc: kwolf@redhat.com
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 hw/net/virtio-net.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 09e173a55854..83642c85b2e5 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3304,15 +3304,27 @@ static bool failover_hide_primary_device(DeviceListener *listener,
         return false;
     }
 
+    /*
+     * The hide helper can be called several times for a given device.
+     * Check there is only one primary for a virtio-net device but
+     * don't duplicate the qdict several times if it's called for the same
+     * device.
+     */
     if (n->primary_opts) {
-        error_setg(errp, "Cannot attach more than one primary device to '%s'",
-                   n->netclient_name);
-        return false;
+        const char *old, *new;
+        /* devices with failover_pair_id always have an id */
+        old = qdict_get_str(n->primary_opts, "id");
+        new = qdict_get_str(device_opts, "id");
+        if (strcmp(old, new) != 0) {
+            error_setg(errp, "Cannot attach more than one primary device to "
+                       "'%s': '%s' and '%s'", n->netclient_name, old, new);
+            return false;
+        }
+    } else {
+        n->primary_opts = qdict_clone_shallow(device_opts);
+        n->primary_opts_from_json = from_json;
     }
 
-    n->primary_opts = qdict_clone_shallow(device_opts);
-    n->primary_opts_from_json = from_json;
-
     /* failover_primary_hidden is set during feature negotiation */
     return qatomic_read(&n->failover_primary_hidden);
 }
-- 
2.31.1

[PATCH 2/2] qdev/qbus: remove failover specific code

[Laurent Vivier]

Commit f3a850565693 ("qdev/qbus: add hidden device support") has
introduced a generic way to hide a device but it has modified
qdev_device_add() to check a specific option of the failover device,
"failover_pair_id", before calling the generic mechanism.

It's not needed (and not generic) to do that in qdev_device_add() because
this is also checked by the failover_hide_primary_device() function that
uses the generic mechanism to hide the device.

Cc: Jens Freimann <jfreimann@redhat.com>
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 hw/net/virtio-net.c    | 12 +++++++++++-
 softmmu/qdev-monitor.c | 18 ++++++------------
 2 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 83642c85b2e5..3dd2896ff95c 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3299,7 +3299,17 @@ static bool failover_hide_primary_device(DeviceListener *listener,
     if (!device_opts) {
         return false;
     }
-    standby_id = qdict_get_try_str(device_opts, "failover_pair_id");
+
+    if (!qdict_haskey(device_opts, "failover_pair_id")) {
+        return false;
+    }
+
+    if (!qdict_haskey(device_opts, "id")) {
+        error_setg(errp, "Device with failover_pair_id needs to have id");
+        return false;
+    }
+
+    standby_id = qdict_get_str(device_opts, "failover_pair_id");
     if (g_strcmp0(standby_id, n->netclient_name) != 0) {
         return false;
     }
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 89c473cb22a2..4851de51a5cb 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -639,19 +639,13 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
         }
     }
 
-    if (qdict_haskey(opts, "failover_pair_id")) {
-        if (!qdict_haskey(opts, "id")) {
-            error_setg(errp, "Device with failover_pair_id don't have id");
-            return NULL;
-        }
-        if (qdev_should_hide_device(opts, from_json, errp)) {
-            if (bus && !qbus_is_hotpluggable(bus)) {
-                error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name);
-            }
-            return NULL;
-        } else if (*errp) {
-            return NULL;
+    if (qdev_should_hide_device(opts, from_json, errp)) {
+        if (bus && !qbus_is_hotpluggable(bus)) {
+            error_setg(errp, QERR_BUS_NO_HOTPLUG, bus->name);
         }
+        return NULL;
+    } else if (*errp) {
+        return NULL;
     }
 
     if (phase_check(PHASE_MACHINE_READY) && bus && !qbus_is_hotpluggable(bus)) {
-- 
2.31.1

Re: [PATCH 0/2] failover: fix a regression introduced by JSON'ification of -device

[Kevin Wolf]

Am 19.10.2021 um 09:15 hat Laurent Vivier geschrieben:
> Kevin's series "qdev: Add JSON -device" has introduced a regression
> in failover by removing the QemuOpts parameter.
> 
> This series fixes that (see PATCH 1) and also makes some cleanup
> in the hide_device function caller to remove the failover specific
> code from qdev_device_add_from_qdict() and clarify the fact that
> a primary device must have an id.

Reviewed-by: Kevin Wolf <kwolf@redhat.com>