Re: [PATCH v4 1/3] qmp: remove virtio_list, search QOM tree instead

["Daniel P. Berrangé" <berrange@redhat.com>]

On Fri, Aug 18, 2023 at 01:19:24PM -0400, Jonah Palmer wrote:
> The virtio_list duplicates information about virtio devices that already
> exist in the QOM composition tree. Instead of creating this list of
> realized virtio devices, search the QOM composition tree instead.
> 
> This patch modifies the QMP command qmp_x_query_virtio to instead
> recursively search the QOM composition tree for devices of type
> 'TYPE_VIRTIO_DEVICE'. The device is also checked to ensure it's
> realized.
> 
> Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com>
> ---
> 
>  Jonah: In the v2 patches, the qmp_x_query_virtio function was
>  iterating through devices found via. qmp_qom_list and appending
>  "/virtio-backend" to devices' paths to check if they were a virtio
>  device.
> 
>  This method was messy and involved unneeded string manipulation.
> 
>  Instead, we can use recursion with object_get_root to iterate through
>  all parent and child device paths to find virtio devices.
> 
>  The qmp_find_virtio_device function was also updated to simplify the
>  method of determining if a path is to a valid and realized virtio
>  device.
> 
>  hw/virtio/virtio-qmp.c | 88 +++++++++++++++---------------------------
>  hw/virtio/virtio-qmp.h |  7 ----
>  hw/virtio/virtio.c     |  6 ---
>  3 files changed, 32 insertions(+), 69 deletions(-)
> 
> diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c
> index 7515b0947b..ac5f0ee0ee 100644
> --- a/hw/virtio/virtio-qmp.c
> +++ b/hw/virtio/virtio-qmp.c
> @@ -667,70 +667,46 @@ VirtioDeviceFeatures *qmp_decode_features(uint16_t device_id, uint64_t bitmap)
>      return features;
>  }
>  
> -VirtioInfoList *qmp_x_query_virtio(Error **errp)
> +static int query_dev_child(Object *child, void *opaque)
>  {
> -    VirtioInfoList *list = NULL;
> -    VirtioInfo *node;
> -    VirtIODevice *vdev;
> +    VirtioInfoList **vdevs = opaque;
> +    Object *dev = object_dynamic_cast(child, TYPE_VIRTIO_DEVICE);
> +    if (dev != NULL && DEVICE(dev)->realized) {
> +        VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> +
> +        VirtioInfo *info = g_new(VirtioInfo, 1);
> +
> +        /* Get canonical path of device */
> +        g_autofree char *path = object_get_canonical_path(dev);
>  
> -    QTAILQ_FOREACH(vdev, &virtio_list, next) {
> -        DeviceState *dev = DEVICE(vdev);
> -        Error *err = NULL;
> -        QObject *obj = qmp_qom_get(dev->canonical_path, "realized", &err);
> -
> -        if (err == NULL) {
> -            GString *is_realized = qobject_to_json_pretty(obj, true);
> -            /* virtio device is NOT realized, remove it from list */
> -            if (!strncmp(is_realized->str, "false", 4)) {
> -                QTAILQ_REMOVE(&virtio_list, vdev, next);
> -            } else {
> -                node = g_new(VirtioInfo, 1);
> -                node->path = g_strdup(dev->canonical_path);
> -                node->name = g_strdup(vdev->name);
> -                QAPI_LIST_PREPEND(list, node);
> -            }
> -           g_string_free(is_realized, true);
> -        }
> -        qobject_unref(obj);
> +        info->path = g_strdup(path);

Just call object_get_canonical_path(dev) directly and avoid
duplicating & freeing the intermediate 'path' variable

> +        info->name = g_strdup(vdev->name);
> +        QAPI_LIST_PREPEND(*vdevs, info);
>      }
> +    return 0;
> +}
>  


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|