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

[Laurent Vivier]

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