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

[Kevin Wolf <kwolf@redhat.com>]

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