[PATCH v2 12/15] virtio-net: Store failover primary opts pointer locally

Kevin Wolf posted 15 patches 1 month, 2 weeks ago

[PATCH v2 12/15] virtio-net: Store failover primary opts pointer locally

Posted by Kevin Wolf 1 month, 2 weeks ago
Instead of accessing the global QemuOptsList, which really belong to the
command line parser and shouldn't be accessed from devices, store a
pointer to the QemuOpts in a new VirtIONet field.

This is not the final state, but just an intermediate step to get rid of
QemuOpts in devices. It will later be replaced with an options QDict.

Before this patch, two "primary" devices could be hidden for the same
standby device, but only one of them would actually be enabled and the
other one would be kept hidden forever, so this doesn't make sense.
After this patch, configuring a second primary device is an error.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/hw/virtio/virtio-net.h |  1 +
 hw/net/virtio-net.c            | 26 ++++++++++++++++++--------
 2 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
index 824a69c23f..d118c95f69 100644
--- a/include/hw/virtio/virtio-net.h
+++ b/include/hw/virtio/virtio-net.h
@@ -209,6 +209,7 @@ struct VirtIONet {
     bool failover_primary_hidden;
     bool failover;
     DeviceListener primary_listener;
+    QemuOpts *primary_opts;
     Notifier migration_state;
     VirtioNetRssData rss_data;
     struct NetRxPkt *rx_pkt;
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index a17d5739fc..ed9a9012e9 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -858,27 +858,24 @@ static DeviceState *failover_find_primary_device(VirtIONet *n)
 static void failover_add_primary(VirtIONet *n, Error **errp)
 {
     Error *err = NULL;
-    QemuOpts *opts;
-    char *id;
     DeviceState *dev = failover_find_primary_device(n);
 
     if (dev) {
         return;
     }
 
-    id = failover_find_primary_device_id(n);
-    if (!id) {
+    if (!n->primary_opts) {
         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);
-    g_assert(opts); /* cannot be NULL because id was found using opts list */
-    dev = qdev_device_add(opts, &err);
+
+    dev = qdev_device_add(n->primary_opts, &err);
     if (err) {
-        qemu_opts_del(opts);
+        qemu_opts_del(n->primary_opts);
+        n->primary_opts = NULL;
     } else {
         object_unref(OBJECT(dev));
     }
@@ -3317,6 +3314,19 @@ static bool failover_hide_primary_device(DeviceListener *listener,
         return false;
     }
 
+    if (n->primary_opts) {
+        error_setg(errp, "Cannot attach more than one primary device to '%s'",
+                   n->netclient_name);
+        return false;
+    }
+
+    /*
+     * Having a weak reference here should be okay because a device can't be
+     * deleted while it's hidden. This will be replaced soon with a QDict that
+     * has a clearer ownership model.
+     */
+    n->primary_opts = device_opts;
+
     /* failover_primary_hidden is set during feature negotiation */
     return qatomic_read(&n->failover_primary_hidden);
 }
-- 
2.31.1

Re: [PATCH v2 12/15] virtio-net: Store failover primary opts pointer locally

Posted by Laurent Vivier 1 month, 1 week ago
On 08/10/2021 15:34, Kevin Wolf wrote:
> Instead of accessing the global QemuOptsList, which really belong to the
> command line parser and shouldn't be accessed from devices, store a
> pointer to the QemuOpts in a new VirtIONet field.
> 
> This is not the final state, but just an intermediate step to get rid of
> QemuOpts in devices. It will later be replaced with an options QDict.
> 
> Before this patch, two "primary" devices could be hidden for the same
> standby device, but only one of them would actually be enabled and the
> other one would be kept hidden forever, so this doesn't make sense.
> After this patch, configuring a second primary device is an error.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   include/hw/virtio/virtio-net.h |  1 +
>   hw/net/virtio-net.c            | 26 ++++++++++++++++++--------
>   2 files changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
> index 824a69c23f..d118c95f69 100644
> --- a/include/hw/virtio/virtio-net.h
> +++ b/include/hw/virtio/virtio-net.h
> @@ -209,6 +209,7 @@ struct VirtIONet {
>       bool failover_primary_hidden;
>       bool failover;
>       DeviceListener primary_listener;
> +    QemuOpts *primary_opts;
>       Notifier migration_state;
>       VirtioNetRssData rss_data;
>       struct NetRxPkt *rx_pkt;
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index a17d5739fc..ed9a9012e9 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -858,27 +858,24 @@ static DeviceState *failover_find_primary_device(VirtIONet *n)
>   static void failover_add_primary(VirtIONet *n, Error **errp)
>   {
>       Error *err = NULL;
> -    QemuOpts *opts;
> -    char *id;
>       DeviceState *dev = failover_find_primary_device(n);
>   
>       if (dev) {
>           return;
>       }
>   
> -    id = failover_find_primary_device_id(n);
> -    if (!id) {
> +    if (!n->primary_opts) {
>           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);
> -    g_assert(opts); /* cannot be NULL because id was found using opts list */
> -    dev = qdev_device_add(opts, &err);
> +
> +    dev = qdev_device_add(n->primary_opts, &err);
>       if (err) {
> -        qemu_opts_del(opts);
> +        qemu_opts_del(n->primary_opts);
> +        n->primary_opts = NULL;
>       } else {
>           object_unref(OBJECT(dev));
>       }
> @@ -3317,6 +3314,19 @@ static bool failover_hide_primary_device(DeviceListener *listener,
>           return false;
>       }
>   
> +    if (n->primary_opts) {
> +        error_setg(errp, "Cannot attach more than one primary device to '%s'",
> +                   n->netclient_name);
> +        return false;
> +    }
> +

This part has introduced a regression, I've sent a patch to fix that.

https://patchew.org/QEMU/20211019071532.682717-1-lvivier@redhat.com/

Thanks,
Laurent

Re: [PATCH v2 12/15] virtio-net: Store failover primary opts pointer locally

Posted by Kevin Wolf 1 month, 1 week ago
Am 19.10.2021 um 10:06 hat Laurent Vivier geschrieben:
> On 08/10/2021 15:34, Kevin Wolf wrote:
> > Instead of accessing the global QemuOptsList, which really belong to the
> > command line parser and shouldn't be accessed from devices, store a
> > pointer to the QemuOpts in a new VirtIONet field.
> > 
> > This is not the final state, but just an intermediate step to get rid of
> > QemuOpts in devices. It will later be replaced with an options QDict.
> > 
> > Before this patch, two "primary" devices could be hidden for the same
> > standby device, but only one of them would actually be enabled and the
> > other one would be kept hidden forever, so this doesn't make sense.
> > After this patch, configuring a second primary device is an error.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >   include/hw/virtio/virtio-net.h |  1 +
> >   hw/net/virtio-net.c            | 26 ++++++++++++++++++--------
> >   2 files changed, 19 insertions(+), 8 deletions(-)
> > 
> > diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
> > index 824a69c23f..d118c95f69 100644
> > --- a/include/hw/virtio/virtio-net.h
> > +++ b/include/hw/virtio/virtio-net.h
> > @@ -209,6 +209,7 @@ struct VirtIONet {
> >       bool failover_primary_hidden;
> >       bool failover;
> >       DeviceListener primary_listener;
> > +    QemuOpts *primary_opts;
> >       Notifier migration_state;
> >       VirtioNetRssData rss_data;
> >       struct NetRxPkt *rx_pkt;
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index a17d5739fc..ed9a9012e9 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -858,27 +858,24 @@ static DeviceState *failover_find_primary_device(VirtIONet *n)
> >   static void failover_add_primary(VirtIONet *n, Error **errp)
> >   {
> >       Error *err = NULL;
> > -    QemuOpts *opts;
> > -    char *id;
> >       DeviceState *dev = failover_find_primary_device(n);
> >       if (dev) {
> >           return;
> >       }
> > -    id = failover_find_primary_device_id(n);
> > -    if (!id) {
> > +    if (!n->primary_opts) {
> >           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);
> > -    g_assert(opts); /* cannot be NULL because id was found using opts list */
> > -    dev = qdev_device_add(opts, &err);
> > +
> > +    dev = qdev_device_add(n->primary_opts, &err);
> >       if (err) {
> > -        qemu_opts_del(opts);
> > +        qemu_opts_del(n->primary_opts);
> > +        n->primary_opts = NULL;
> >       } else {
> >           object_unref(OBJECT(dev));
> >       }
> > @@ -3317,6 +3314,19 @@ static bool failover_hide_primary_device(DeviceListener *listener,
> >           return false;
> >       }
> > +    if (n->primary_opts) {
> > +        error_setg(errp, "Cannot attach more than one primary device to '%s'",
> > +                   n->netclient_name);
> > +        return false;
> > +    }
> > +
> 
> This part has introduced a regression, I've sent a patch to fix that.
> 
> https://patchew.org/QEMU/20211019071532.682717-1-lvivier@redhat.com/

Thanks for catching this! The fix looks good to me.

Kevin