| Message ID | 20230803145500.2108691-2-jonah.palmer@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | qmp, vhost-user: Remove virtio_list & update virtio introspection | expand |
On Thu, Aug 03, 2023 at 10:54:58AM -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. > > [Jonah: In the previous commit 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.] FWIW, this "history" would typically go after the '---' but before the diffstat, as it is relevant to reviewers of this new v3, but doesn't need to get into the permanent git log once merged. > Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com> > --- > hw/virtio/virtio-qmp.c | 96 ++++++++++++++++++------------------------ > hw/virtio/virtio-qmp.h | 7 --- > hw/virtio/virtio.c | 6 --- > 3 files changed, 40 insertions(+), 69 deletions(-) Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> With regards, Daniel
On Thu, 03 Aug 2023 17:54, Jonah Palmer <jonah.palmer@oracle.com> wrote: >-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 */ >+ gchar *path = object_get_canonical_path(dev); (You can use g_autofree char * here) >+ >+ info->path = g_strdup(path); >+ info->name = g_strdup(vdev->name); >+ QAPI_LIST_PREPEND(*vdevs, info); > >- 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); >+ g_free(path); >+ } else { >+ object_unref(dev); > } The object_unref should not happen only in the else branch, no? Though it's not clear to me where the ref count was previously incremented. >+ object_child_foreach_recursive(object_get_root(), query_dev_child, >&vdevs); >+ if (vdevs == NULL) { >+ error_setg(errp, "No virtio devices found"); >+ return NULL; (No need for early return here) > } >- return NULL; >+ return vdevs; >+} >+ >+VirtIODevice *qmp_find_virtio_device(const char *path) >+{ >+ /* Verify the canonical path is a realized virtio device */ >+ Object *dev = object_dynamic_cast(object_resolve_path(path, NULL), >+ TYPE_VIRTIO_DEVICE); >+ if (!dev || !DEVICE(dev)->realized) { >+ object_unref(dev); Same as before with object refs
On 8/3/23 11:05, Daniel P. Berrangé wrote: > On Thu, Aug 03, 2023 at 10:54:58AM -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. >> >> [Jonah: In the previous commit 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.] > FWIW, this "history" would typically go after the '---' but before > the diffstat, as it is relevant to reviewers of this new v3, but > doesn't need to get into the permanent git log once merged. Yes, you're right, my apologies. I'm still familiarizing myself with these kinds of things. I will move this comment to the correct location in v4 so it won't show up in the git log. Thank you for the clarification! >> Signed-off-by: Jonah Palmer<jonah.palmer@oracle.com> >> --- >> hw/virtio/virtio-qmp.c | 96 ++++++++++++++++++------------------------ >> hw/virtio/virtio-qmp.h | 7 --- >> hw/virtio/virtio.c | 6 --- >> 3 files changed, 40 insertions(+), 69 deletions(-) > Reviewed-by: Daniel P. Berrangé<berrange@redhat.com> > > > With regards, > Daniel
On 8/3/23 12:40, Manos Pitsidianakis wrote: > On Thu, 03 Aug 2023 17:54, Jonah Palmer <jonah.palmer@oracle.com> wrote: >> -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 */ >> + gchar *path = object_get_canonical_path(dev); > > (You can use g_autofree char * here) > Got it, thanks. I'll use this and remove the g_free() call as well. >> + >> + info->path = g_strdup(path); >> + info->name = g_strdup(vdev->name); >> + QAPI_LIST_PREPEND(*vdevs, info); >> >> - 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); >> + g_free(path); >> + } else { >> + object_unref(dev); >> } > > The object_unref should not happen only in the else branch, no? Though > it's not clear to me where the ref count was previously incremented. > There is no reference count being incremented, my apologies. Therefore, there's no need to have this object_unref being called. I'll remove this and the one in the qmp_find_virtio_device function below. Thanks for pointing this out. >> + object_child_foreach_recursive(object_get_root(), query_dev_child, >> &vdevs); >> + if (vdevs == NULL) { >> + error_setg(errp, "No virtio devices found"); >> + return NULL; > > (No need for early return here) > Good catch. Will remove. Thanks! >> } >> - return NULL; >> + return vdevs; >> +} >> + >> +VirtIODevice *qmp_find_virtio_device(const char *path) >> +{ >> + /* Verify the canonical path is a realized virtio device */ >> + Object *dev = object_dynamic_cast(object_resolve_path(path, NULL), >> + TYPE_VIRTIO_DEVICE); >> + if (!dev || !DEVICE(dev)->realized) { >> + object_unref(dev); > > Same as before with object refs
diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c index 3d32dbec8d..baec351c4f 100644 --- a/hw/virtio/virtio-qmp.c +++ b/hw/virtio/virtio-qmp.c @@ -665,70 +665,54 @@ 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 */ + gchar *path = object_get_canonical_path(dev); + + info->path = g_strdup(path); + info->name = g_strdup(vdev->name); + QAPI_LIST_PREPEND(*vdevs, info); - 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); + g_free(path); + } else { + object_unref(dev); } - return list; + return 0; } -VirtIODevice *qmp_find_virtio_device(const char *path) +VirtioInfoList *qmp_x_query_virtio(Error **errp) { - VirtIODevice *vdev; + VirtioInfoList *vdevs = NULL; - QTAILQ_FOREACH(vdev, &virtio_list, next) { - DeviceState *dev = DEVICE(vdev); - - if (strcmp(dev->canonical_path, path) != 0) { - continue; - } - - 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; - } - g_string_free(is_realized, true); - } else { - /* virtio device doesn't exist in QOM tree */ - QTAILQ_REMOVE(&virtio_list, vdev, next); - qobject_unref(obj); - return NULL; - } - /* device exists in QOM tree & is realized */ - qobject_unref(obj); - return vdev; + /* Query the QOM composition tree recursively for virtio devices */ + object_child_foreach_recursive(object_get_root(), query_dev_child, &vdevs); + if (vdevs == NULL) { + error_setg(errp, "No virtio devices found"); + return NULL; } - return NULL; + return vdevs; +} + +VirtIODevice *qmp_find_virtio_device(const char *path) +{ + /* Verify the canonical path is a realized virtio device */ + Object *dev = object_dynamic_cast(object_resolve_path(path, NULL), + TYPE_VIRTIO_DEVICE); + if (!dev || !DEVICE(dev)->realized) { + object_unref(dev); + return NULL; + } + + return VIRTIO_DEVICE(dev); } VirtioStatus *qmp_x_query_virtio_status(const char *path, Error **errp) @@ -738,7 +722,7 @@ VirtioStatus *qmp_x_query_virtio_status(const char *path, Error **errp) vdev = qmp_find_virtio_device(path); if (vdev == NULL) { - error_setg(errp, "Path %s is not a VirtIODevice", path); + error_setg(errp, "Path %s is not a realized VirtIODevice", path); return NULL; } diff --git a/hw/virtio/virtio-qmp.h b/hw/virtio/virtio-qmp.h index 8af5f5e65a..245a446a56 100644 --- a/hw/virtio/virtio-qmp.h +++ b/hw/virtio/virtio-qmp.h @@ -15,13 +15,6 @@ #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; - 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)
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. [Jonah: In the previous commit 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.] Signed-off-by: Jonah Palmer <jonah.palmer@oracle.com> --- hw/virtio/virtio-qmp.c | 96 ++++++++++++++++++------------------------ hw/virtio/virtio-qmp.h | 7 --- hw/virtio/virtio.c | 6 --- 3 files changed, 40 insertions(+), 69 deletions(-)