Re: [PATCH v4 2/5] hw/virtio: document SharedObject structures

[Manos Pitsidianakis <manos.pitsidianakis@linaro.org>]

On Mon, 19 Feb 2024 16:34, Albert Esteve <aesteve@redhat.com> wrote:
>Change VirtioSharedObject value type from
>a generic pointer to a union storing the different
>supported underlying types, which makes naming
>less confusing.
>
>With the update, use the chance to add kdoc
>to both the SharedObjectType enum and
>VirtioSharedObject struct.
>
>Signed-off-by: Albert Esteve <aesteve@redhat.com>
>---
> hw/display/virtio-dmabuf.c        |  8 ++++----
> include/hw/virtio/virtio-dmabuf.h | 25 ++++++++++++++++++++++++-
> 2 files changed, 28 insertions(+), 5 deletions(-)
>
>diff --git a/hw/display/virtio-dmabuf.c b/hw/display/virtio-dmabuf.c
>index 3dba4577ca..497cb6fa7c 100644
>--- a/hw/display/virtio-dmabuf.c
>+++ b/hw/display/virtio-dmabuf.c
>@@ -57,7 +57,7 @@ bool virtio_add_dmabuf(QemuUUID *uuid, int udmabuf_fd)
>     }
>     vso = g_new(VirtioSharedObject, 1);
>     vso->type = TYPE_DMABUF;
>-    vso->value = GINT_TO_POINTER(udmabuf_fd);
>+    vso->value.udma_buf = udmabuf_fd;
>     result = virtio_add_resource(uuid, vso);
>     if (!result) {
>         g_free(vso);
>@@ -75,7 +75,7 @@ bool virtio_add_vhost_device(QemuUUID *uuid, struct vhost_dev *dev)
>     }
>     vso = g_new(VirtioSharedObject, 1);
>     vso->type = TYPE_VHOST_DEV;
>-    vso->value = dev;
>+    vso->value.dev = dev;
>     result = virtio_add_resource(uuid, vso);
>     if (!result) {
>         g_free(vso);
>@@ -114,7 +114,7 @@ int virtio_lookup_dmabuf(const QemuUUID *uuid)
>         return -1;
>     }
>     assert(vso->type == TYPE_DMABUF);
>-    return GPOINTER_TO_INT(vso->value);
>+    return vso->value.udma_buf;
> }
> 
> struct vhost_dev *virtio_lookup_vhost_device(const QemuUUID *uuid)
>@@ -124,7 +124,7 @@ struct vhost_dev *virtio_lookup_vhost_device(const QemuUUID *uuid)
>         return NULL;
>     }
>     assert(vso->type == TYPE_VHOST_DEV);
>-    return (struct vhost_dev *) vso->value;
>+    return (struct vhost_dev *) vso->value.dev;

Is the casting still required?


> }
> 
> SharedObjectType virtio_object_type(const QemuUUID *uuid)
>diff --git a/include/hw/virtio/virtio-dmabuf.h b/include/hw/virtio/virtio-dmabuf.h
>index 627c3b6db7..891a43162d 100644
>--- a/include/hw/virtio/virtio-dmabuf.h
>+++ b/include/hw/virtio/virtio-dmabuf.h
>@@ -16,15 +16,38 @@
> #include "qemu/uuid.h"
> #include "vhost.h"
> 
>+/**
>+ * SharedObjectType:
>+ *
>+ * Identifies the type of the underlying type that the current lookup
>+ * table entry is holding.
>+ * 
>+ * TYPE_INVALID: Invalid entry
>+ * TYPE_DMABUF: Entry is a dmabuf file descriptor that can be directly
>+ *              shared with the requestor
>+ * TYPE_VHOST_DEV: Entry is a pointer to a vhost device that is holding
>+ *                 the shared object.


nit:

+ * TYPE_INVALID:   Invalid entry.
+ * TYPE_DMABUF:    Entry is a dmabuf file descriptor that can be
+ *                 directly shared with the requestor.
+ * TYPE_VHOST_DEV: Entry is a pointer to a vhost device that is holding
+ *                 the shared object.


>+ */
> typedef enum SharedObjectType {
>     TYPE_INVALID = 0,
>     TYPE_DMABUF,
>     TYPE_VHOST_DEV,
> } SharedObjectType;
> 
>+/**
>+ * VirtioSharedObject:
>+ * @type: Shared object type identifier
>+ * @value: Union containing to the underlying type
>+ * 
>+ * The VirtioSharedObject object provides a way to distinguish,
>+ * store, and handle the different types supported by the lookup table.
>+ */
> typedef struct VirtioSharedObject {
>     SharedObjectType type;
>-    gpointer value;
>+    union {
>+        struct vhost_dev *dev;  /* TYPE_VHOST_DEV */
>+        int udma_buf;           /* TYPE_DMABUF */
>+    } value;
> } VirtioSharedObject;
> 
> /**
>-- 
>2.43.1
>
>