Re: [PATCH v2 14/15] qdev: Base object creation on QDict rather than QemuOpts

From: Laurent Vivier <lvivier@redhat.com>
To: Kevin Wolf <kwolf@redhat.com>, qemu-devel@nongnu.org
Cc: damien.hedde@greensocs.com, pkrempa@redhat.com,
	berrange@redhat.com, ehabkost@redhat.com, qemu-block@nongnu.org,
	mst@redhat.com, libvir-list@redhat.com, jasowang@redhat.com,
	quintela@redhat.com, armbru@redhat.com, vsementsov@virtuozzo.com,
	its@irrelevant.dk, pbonzini@redhat.com, eblake@redhat.com
Subject: Re: [PATCH v2 14/15] qdev: Base object creation on QDict rather than QemuOpts
Date: Fri, 8 Oct 2021 17:17:29 +0200	[thread overview]
Message-ID: <924b4711-574f-0629-782f-09199a799d78@redhat.com> (raw)
In-Reply-To: <20211008133442.141332-15-kwolf@redhat.com>

On 08/10/2021 15:34, Kevin Wolf wrote:
> QDicts are both what QMP natively uses and what the keyval parser
> produces. Going through QemuOpts isn't useful for either one, so switch
> the main device creation function to QDicts. By sharing more code with
> the -object/object-add code path, we can even reduce the code size a
> bit.
> 
> This commit doesn't remove the detour through QemuOpts from any code
> path yet, but it allows the following commits to do so.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   include/hw/qdev-core.h         | 11 +++---
>   include/hw/virtio/virtio-net.h |  3 +-
>   include/monitor/qdev.h         |  2 +
>   hw/core/qdev.c                 |  7 ++--
>   hw/net/virtio-net.c            | 23 +++++++-----
>   hw/vfio/pci.c                  |  4 +-
>   softmmu/qdev-monitor.c         | 69 +++++++++++++++-------------------
>   7 files changed, 60 insertions(+), 59 deletions(-)
> 
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 74d8b614a7..910042c650 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -180,7 +180,7 @@ struct DeviceState {
>       char *canonical_path;
>       bool realized;
>       bool pending_deleted_event;
> -    QemuOpts *opts;
> +    QDict *opts;
>       int hotplugged;
>       bool allow_unplug_during_migration;
>       BusState *parent_bus;
> @@ -205,8 +205,8 @@ struct DeviceListener {
>        * On errors, it returns false and errp is set. Device creation
>        * should fail in this case.
>        */
> -    bool (*hide_device)(DeviceListener *listener, QemuOpts *device_opts,
> -                        Error **errp);
> +    bool (*hide_device)(DeviceListener *listener, const QDict *device_opts,
> +                        bool from_json, Error **errp);
>       QTAILQ_ENTRY(DeviceListener) link;
>   };
>   
> @@ -835,13 +835,14 @@ void device_listener_unregister(DeviceListener *listener);
>   
>   /**
>    * @qdev_should_hide_device:
> - * @opts: QemuOpts as passed on cmdline.
> + * @opts: options QDict > + * @from_json: true if @opts entries are typed, false for all strings

Add the errp here too:

     * @errp: pointer to error object

>    *
>    * Check if a device should be added.
>    * When a device is added via qdev_device_add() this will be called,
>    * and return if the device should be added now or not.
>    */
> -bool qdev_should_hide_device(QemuOpts *opts, Error **errp);
> +bool qdev_should_hide_device(const QDict *opts, bool from_json, Error **errp);
>   
>   typedef enum MachineInitPhase {
>       /* current_machine is NULL.  */

Thank you to have added the errp pointer in the hide_device helpers, it helps in my series.

Laurent