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

[Jonah Palmer]

On 8/3/23 12:40, Manos Pitsidianakis wrote:

On Thu, 03 Aug 2023 17:54, Jonah Palmer <jonah.pal...@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