On Fri, Jun 09, 2023 at 09:20:39AM -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 search
> the partial paths of '/machine/peripheral/' &
> '/machine/peripheral-anon/' in the QOM composition tree for virtio
> devices.
>
> A device is found to be a valid virtio device if (1) its canonical path
> is of 'TYPE_VIRTIO_DEVICE' and (2) the device has been realized.
>
> [Jonah: In the previous commit I had written that a device is found to
> be a valid virtio device if (1) it has a canonical path ending with
> 'virtio-backend'.
>
> The code now determines if it's a virtio device by appending
> 'virtio-backend' (if needed) to a given canonical path and then
> checking that path to see if the device is of type
> 'TYPE_VIRTIO_DEVICE'.
>
> The patch also instead now checks to make sure it's a virtio device
> before attempting to check whether the device is realized or not.]
>
> Signed-off-by: Jonah Palmer <jonah.pal...@oracle.com>
> ---
> hw/virtio/virtio-qmp.c | 128 ++++++++++++++++++++++++++---------------
> hw/virtio/virtio-qmp.h | 8 +--
> hw/virtio/virtio.c | 6 --
> 3 files changed, 82 insertions(+), 60 deletions(-)
>
> diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c
> index b5e1835299..e936cc8ce5 100644
> --- a/hw/virtio/virtio-qmp.c
> +++ b/hw/virtio/virtio-qmp.c
> @@ -668,67 +668,101 @@ VirtioDeviceFeatures *qmp_decode_features(uint16_t
> device_id, uint64_t bitmap)
> VirtioInfoList *qmp_x_query_virtio(Error **errp)
> {
> VirtioInfoList *list = NULL;
> - VirtioInfo *node;
> - VirtIODevice *vdev;
>
> - 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);
> + /* Query the QOM composition tree for virtio devices */
> + qmp_set_virtio_device_list("/machine/peripheral/", &list);
> + qmp_set_virtio_device_list("/machine/peripheral-anon/", &list);
> + if (list == NULL) {
> + error_setg(errp, "No virtio devices found");
> + return NULL;
> + }
> + return list;
> +}
> +
> +/* qmp_set_virtio_device_list:
> + * @ppath: An incomplete peripheral path to search from.
> + * @list: A list of realized virtio devices.
> + * Searches a given incomplete peripheral path (e.g. '/machine/peripheral/'
> + * or '/machine/peripheral-anon/') for realized virtio devices and adds them
> + * to a given list of virtio devices.
> + */
> +void qmp_set_virtio_device_list(const char *ppath, VirtioInfoList **list)
> +{
> + ObjectPropertyInfoList *plist;
> + VirtioInfoList *node;
> + Error *err = NULL;
> +
> + /* Search an incomplete path for virtio devices */
> + plist = qmp_qom_list(ppath, &err);
> + if (err == NULL) {
> + ObjectPropertyInfoList *start = plist;
> + while (plist != NULL) {
> + ObjectPropertyInfo *value = plist->value;
> + GString *path = g_string_new(ppath);
> + g_string_append(path, value->name);
> + g_string_append(path, "/virtio-backend");
> +
> + /* Determine if full path is a realized virtio device */
> + VirtIODevice *vdev = qmp_find_virtio_device(path->str);
> + if (vdev != NULL) {
> + node = g_new0(VirtioInfoList, 1);
> + node->value = g_new(VirtioInfo, 1);
> + node->value->path = g_strdup(path->str);
> + node->value->name = g_strdup(vdev->name);
> + QAPI_LIST_PREPEND(*list, node->value);
> }
> - g_string_free(is_realized, true);
> + g_string_free(path, true);
> + plist = plist->next;
> }
> - qobject_unref(obj);
> + qapi_free_ObjectPropertyInfoList(start);
> }
> -
> - return list;
> }
This is all way too complicated. AFAICT, it shouldn't require
much more than this:
static int one_child(Object *child, void *opaque)
{
VirtioInfoList **devs = opaque;
VirtIODevice *dev = object_dynamic_cast(child, TYPE_VIRTIO_DEVICE);
if (dev != NULL) {
VirtioInfo *info =g_new(VirtioInfo, 1);
info->path = g_strdup(path->str);
info->name = g_strdup(vdev->name);
QAPI_LIST_PREPEND(*devs, info);
}
return 0;
}
VirtioInfoList *qmp_x_query_virtio(Error **errp)
{
VirtioInfoList *devs = NULL;
object_child_foreach_recursive(object_get_root(),
one_child,
&devs);
if (devs == NULL) {
error_setg(errp, "No virtio devices found");
return NULL;
}
return devs;
}
>
> VirtIODevice *qmp_find_virtio_device(const char *path)
> {
> - VirtIODevice *vdev;
> -
> - QTAILQ_FOREACH(vdev, &virtio_list, next) {
> - DeviceState *dev = DEVICE(vdev);
> -
> - if (strcmp(dev->canonical_path, path) != 0) {
> - continue;
> + Error *err = NULL;
> + char *basename;
> +
> + /* Append 'virtio-backend' to path if needed */
> + basename = g_path_get_basename(path);
> + if (strcmp(basename, "virtio-backend")) {
> + GString *temp = g_string_new(path);
> + char *last = strrchr(path, '/');
> + if (g_strcmp0(last, "/")) {
> + g_string_append(temp, "/virtio-backend");
> + } else {
> + g_string_append(temp, "virtio-backend");
> }
> + path = g_strdup(temp->str);
> + g_string_free(temp, true);
> + }
>
> - 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)) {
> - g_string_free(is_realized, true);
> - qobject_unref(obj);
> - QTAILQ_REMOVE(&virtio_list, vdev, next);
> - return NULL;
> - }
> + /* Verify the canonical path is a virtio device */
> + Object *obj = object_resolve_path(path, NULL);
> + if (!obj || !object_dynamic_cast(obj, TYPE_VIRTIO_DEVICE)) {
> + object_unref(obj);
> + return NULL;
> + }
> +
> + /* Verify the virtio device is realized */
> + QObject *qobj = qmp_qom_get(path, "realized", &err);
> + if (err == NULL) {
> + GString *is_realized = qobject_to_json_pretty(qobj, true);
> + if (!strncmp(is_realized->str, "false", 4)) {
> g_string_free(is_realized, true);
> - } else {
> - /* virtio device doesn't exist in QOM tree */
> - QTAILQ_REMOVE(&virtio_list, vdev, next);
> - qobject_unref(obj);
> + qobject_unref(qobj);
> return NULL;
> }
> - /* device exists in QOM tree & is realized */
> - qobject_unref(obj);
> - return vdev;
> + g_string_free(is_realized, true);
> + } else {
> + qobject_unref(qobj);
> + return NULL;
> }
> - return NULL;
> + qobject_unref(qobj);
> +
> + /* Get VirtIODevice object */
> + VirtIODevice *vdev = VIRTIO_DEVICE(obj);
> + return vdev;
> }
>
> VirtioStatus *qmp_x_query_virtio_status(const char *path, Error **errp)
> diff --git a/hw/virtio/virtio-qmp.h b/hw/virtio/virtio-qmp.h
> index 8af5f5e65a..4b2b7875b4 100644
> --- a/hw/virtio/virtio-qmp.h
> +++ b/hw/virtio/virtio-qmp.h
> @@ -15,13 +15,7 @@
> #include "hw/virtio/virtio.h"
> #include "hw/virtio/vhost.h"
>
> -#include "qemu/queue.h"
> -
> -typedef QTAILQ_HEAD(QmpVirtIODeviceList, VirtIODevice) QmpVirtIODeviceList;
> -
> -/* QAPI list of realized VirtIODevices */
> -extern QmpVirtIODeviceList virtio_list;
> -
> +void qmp_set_virtio_device_list(const char *ppath, VirtioInfoList **list);
> VirtIODevice *qmp_find_virtio_device(const char *path);
> VirtioDeviceStatus *qmp_decode_status(uint8_t bitmap);
> VhostDeviceProtocols *qmp_decode_protocols(uint64_t bitmap);
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 295a603e58..83c5db3d26 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -45,8 +45,6 @@
> #include "standard-headers/linux/virtio_mem.h"
> #include "standard-headers/linux/virtio_vsock.h"
>
> -QmpVirtIODeviceList virtio_list;
> -
> /*
> * Maximum size of virtio device config space
> */
> @@ -3616,7 +3614,6 @@ static void virtio_device_realize(DeviceState *dev,
> Error **errp)
> vdev->listener.commit = virtio_memory_listener_commit;
> vdev->listener.name = "virtio";
> memory_listener_register(&vdev->listener, vdev->dma_as);
> - QTAILQ_INSERT_TAIL(&virtio_list, vdev, next);
> }
>
> static void virtio_device_unrealize(DeviceState *dev)
> @@ -3631,7 +3628,6 @@ static void virtio_device_unrealize(DeviceState *dev)
> vdc->unrealize(dev);
> }
>
> - QTAILQ_REMOVE(&virtio_list, vdev, next);
> g_free(vdev->bus_name);
> vdev->bus_name = NULL;
> }
> @@ -3805,8 +3801,6 @@ static void virtio_device_class_init(ObjectClass
> *klass, void *data)
> vdc->stop_ioeventfd = virtio_device_stop_ioeventfd_impl;
>
> vdc->legacy_features |= VIRTIO_LEGACY_FEATURES;
> -
> - QTAILQ_INIT(&virtio_list);
> }
>
> bool virtio_device_ioeventfd_enabled(VirtIODevice *vdev)
> --
> 2.39.3
>
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 :|
