On Thu, Dec 7, 2023 at 3:55 PM Albert Esteve <aest...@redhat.com> wrote:
> Functions in the virtio-dmabuf module
> start with 'virtio_*', which is too
> generic and may not correctly identify
> them as part of the virtio dmabuf API.
>
> Rename all functions to 'virtio_dmabuf_*'
> instead to avoid confusion.
>
> Signed-off-by: Albert Esteve <aest...@redhat.com>
> ---
> hw/display/virtio-dmabuf.c | 14 +++++-----
> hw/virtio/vhost-user.c | 14 +++++-----
> include/hw/virtio/virtio-dmabuf.h | 33 ++++++++++++-----------
> tests/unit/test-virtio-dmabuf.c | 44 +++++++++++++++----------------
> 4 files changed, 53 insertions(+), 52 deletions(-)
>
> diff --git a/hw/display/virtio-dmabuf.c b/hw/display/virtio-dmabuf.c
> index 6688809777..42495f87ec 100644
> --- a/hw/display/virtio-dmabuf.c
> +++ b/hw/display/virtio-dmabuf.c
> @@ -48,7 +48,7 @@ static bool virtio_add_resource(QemuUUID *uuid,
> VirtioSharedObject *value)
> return result;
> }
>
> -bool virtio_add_dmabuf(QemuUUID *uuid, int udmabuf_fd)
> +bool virtio_dmabuf_add(QemuUUID *uuid, int udmabuf_fd)
> {
> bool result;
> VirtioSharedObject *vso;
> @@ -66,7 +66,7 @@ bool virtio_add_dmabuf(QemuUUID *uuid, int udmabuf_fd)
> return result;
> }
>
> -bool virtio_add_vhost_device(QemuUUID *uuid, struct vhost_dev *dev)
> +bool virtio_dmabuf_add_vhost_device(QemuUUID *uuid, struct vhost_dev *dev)
> {
> bool result;
> VirtioSharedObject *vso;
> @@ -84,7 +84,7 @@ bool virtio_add_vhost_device(QemuUUID *uuid, struct
> vhost_dev *dev)
> return result;
> }
>
> -bool virtio_remove_resource(const QemuUUID *uuid)
> +bool virtio_dmabuf_remove_resource(const QemuUUID *uuid)
> {
> bool result;
> g_mutex_lock(&lock);
> @@ -107,7 +107,7 @@ static VirtioSharedObject *get_shared_object(const
> QemuUUID *uuid)
> return (VirtioSharedObject *) lookup_res;
> }
>
> -int virtio_lookup_dmabuf(const QemuUUID *uuid)
> +int virtio_dmabuf_lookup(const QemuUUID *uuid)
> {
> VirtioSharedObject *vso = get_shared_object(uuid);
> if (vso == NULL) {
> @@ -117,7 +117,7 @@ int virtio_lookup_dmabuf(const QemuUUID *uuid)
> return GPOINTER_TO_INT(vso->value);
> }
>
> -struct vhost_dev *virtio_lookup_vhost_device(const QemuUUID *uuid)
> +struct vhost_dev *virtio_dmabuf_lookup_vhost_device(const QemuUUID *uuid)
> {
> VirtioSharedObject *vso = get_shared_object(uuid);
> if (vso == NULL) {
> @@ -127,7 +127,7 @@ struct vhost_dev *virtio_lookup_vhost_device(const
> QemuUUID *uuid)
> return (struct vhost_dev *) vso->value;
> }
>
> -SharedObjectType virtio_object_type(const QemuUUID *uuid)
> +SharedObjectType virtio_dmabuf_object_type(const QemuUUID *uuid)
> {
> VirtioSharedObject *vso = get_shared_object(uuid);
> if (vso == NULL) {
> @@ -158,7 +158,7 @@ int virtio_dmabuf_vhost_cleanup(struct vhost_dev *dev)
> return num_removed;
> }
>
> -void virtio_free_resources(void)
> +void virtio_dmabuf_free_resources(void)
> {
> g_mutex_lock(&lock);
> g_hash_table_destroy(resource_uuids);
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 1c3f2357be..2ab9e13f9e 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -1607,7 +1607,7 @@ vhost_user_backend_handle_shared_object_add(struct
> vhost_dev *dev,
> QemuUUID uuid;
>
> memcpy(uuid.data, object->uuid, sizeof(object->uuid));
> - return virtio_add_vhost_device(&uuid, dev);
> + return virtio_dmabuf_add_vhost_device(&uuid, dev);
> }
>
> static int
> @@ -1617,10 +1617,10 @@
> vhost_user_backend_handle_shared_object_remove(struct vhost_dev *dev,
> QemuUUID uuid;
>
> memcpy(uuid.data, object->uuid, sizeof(object->uuid));
> - switch (virtio_object_type(&uuid)) {
> + switch (virtio_dmabuf_object_type(&uuid)) {
> case TYPE_VHOST_DEV:
> {
> - struct vhost_dev *owner = virtio_lookup_vhost_device(&uuid);
> + struct vhost_dev *owner =
> virtio_dmabuf_lookup_vhost_device(&uuid);
> if (owner == NULL || dev != owner) {
> /* Not allowed to remove non-owned entries */
> return 0;
> @@ -1632,7 +1632,7 @@
> vhost_user_backend_handle_shared_object_remove(struct vhost_dev *dev,
> return 0;
> }
>
> - return virtio_remove_resource(&uuid);
> + return virtio_dmabuf_remove_resource(&uuid);
> }
>
> static bool vhost_user_send_resp(QIOChannel *ioc, VhostUserHeader *hdr,
> @@ -1710,13 +1710,13 @@
> vhost_user_backend_handle_shared_object_lookup(struct vhost_user *u,
> memcpy(uuid.data, payload->object.uuid, sizeof(payload->object.uuid));
>
> payload->u64 = 0;
> - switch (virtio_object_type(&uuid)) {
> + switch (virtio_dmabuf_object_type(&uuid)) {
> case TYPE_DMABUF:
> - dmabuf_fd = virtio_lookup_dmabuf(&uuid);
> + dmabuf_fd = virtio_dmabuf_lookup(&uuid);
> break;
> case TYPE_VHOST_DEV:
> {
> - struct vhost_dev *dev = virtio_lookup_vhost_device(&uuid);
> + struct vhost_dev *dev = virtio_dmabuf_lookup_vhost_device(&uuid);
> if (dev == NULL) {
> payload->u64 = -EINVAL;
> break;
> diff --git a/include/hw/virtio/virtio-dmabuf.h
> b/include/hw/virtio/virtio-dmabuf.h
> index 73f70fb482..186a18a33b 100644
> --- a/include/hw/virtio/virtio-dmabuf.h
> +++ b/include/hw/virtio/virtio-dmabuf.h
> @@ -28,7 +28,7 @@ typedef struct VirtioSharedObject {
> } VirtioSharedObject;
>
> /**
> - * virtio_add_dmabuf() - Add a new dma-buf resource to the lookup table
> + * virtio_dmabuf_add() - Add a new dma-buf resource to the lookup table
> * @uuid: new resource's UUID
> * @dmabuf_fd: the dma-buf descriptor that will be stored and shared with
> * other virtio devices. The caller retains ownership over the
> @@ -41,11 +41,11 @@ typedef struct VirtioSharedObject {
> * Note that if it finds a repeated UUID, the resource is not inserted in
> * the lookup table.
> */
> -bool virtio_add_dmabuf(QemuUUID *uuid, int dmabuf_fd);
> +bool virtio_dmabuf_add(QemuUUID *uuid, int dmabuf_fd);
>
> /**
> - * virtio_add_vhost_device() - Add a new exporter vhost device that holds
> the
> - * resource with the associated UUID
> + * virtio_dmabuf_add_vhost_device() - Add a new exporter vhost device that
> + * holds the resource with the associated UUID
> * @uuid: new resource's UUID
> * @dev: the pointer to the vhost device that holds the resource. The
> caller
> * retains ownership over the device struct and its lifecycle.
> @@ -55,41 +55,42 @@ bool virtio_add_dmabuf(QemuUUID *uuid, int dmabuf_fd);
> * Note that if it finds a repeated UUID, the resource is not inserted in
> * the lookup table.
> */
> -bool virtio_add_vhost_device(QemuUUID *uuid, struct vhost_dev *dev);
> +bool virtio_dmabuf_add_vhost_device(QemuUUID *uuid, struct vhost_dev
> *dev);
>
> /**
> - * virtio_remove_resource() - Removes a resource from the lookup table
> + * virtio_dmabuf_remove_resource() - Removes a resource from the lookup
> table
> * @uuid: resource's UUID
> *
> * Return: true if the UUID has been found and removed from the lookup
> table.
> */
> -bool virtio_remove_resource(const QemuUUID *uuid);
> +bool virtio_dmabuf_remove_resource(const QemuUUID *uuid);
>
> /**
> - * virtio_lookup_dmabuf() - Looks for a dma-buf resource in the lookup
> table
> + * virtio_dmabuf_lookup() - Looks for a dma-buf resource in the lookup
> table
> * @uuid: resource's UUID
> *
> * Return: the dma-buf file descriptor integer, or -1 if the key is not
> found.
> */
> -int virtio_lookup_dmabuf(const QemuUUID *uuid);
> +int virtio_dmabuf_lookup(const QemuUUID *uuid);
>
> /**
> - * virtio_lookup_vhost_device() - Looks for an exporter vhost device in
> the
> - * lookup table
> + * virtio_dmabuf_lookup_vhost_device() - Looks for an exporter vhost
> device
> + * in the lookup table
> * @uuid: resource's UUID
> *
> * Return: pointer to the vhost_dev struct, or NULL if the key is not
> found.
> */
> -struct vhost_dev *virtio_lookup_vhost_device(const QemuUUID *uuid);
> +struct vhost_dev *virtio_dmabuf_lookup_vhost_device(const QemuUUID *uuid);
>
> /**
> - * virtio_object_type() - Looks for the type of resource in the lookup
> table
> + * virtio_dmabuf_object_type() - Looks for the type of resource in the
> + * lookup table
> * @uuid: resource's UUID
> *
> * Return: the type of resource associated with the UUID, or TYPE_INVALID
> if
> * the key is not found.
> */
> -SharedObjectType virtio_object_type(const QemuUUID *uuid);
> +SharedObjectType virtio_dmabuf_object_type(const QemuUUID *uuid);
>
> /**
> * virtio_dmabuf_vhost_cleanup() - Destroys all entries of the shared
> @@ -102,9 +103,9 @@ SharedObjectType virtio_object_type(const QemuUUID
> *uuid);
> int virtio_dmabuf_vhost_cleanup(struct vhost_dev *dev);
>
> /**
> - * virtio_free_resources() - Destroys all keys and values of the shared
> + * virtio_dmabuf_free_resources() - Destroys all keys and values of the
> shared
> * resources lookup table, and frees them
> */
> -void virtio_free_resources(void);
> +void virtio_dmabuf_free_resources(void);
>
> #endif /* VIRTIO_DMABUF_H */
> diff --git a/tests/unit/test-virtio-dmabuf.c
> b/tests/unit/test-virtio-dmabuf.c
> index 1c8123c2d2..7471ed4fbd 100644
> --- a/tests/unit/test-virtio-dmabuf.c
> +++ b/tests/unit/test-virtio-dmabuf.c
> @@ -31,12 +31,12 @@ static void test_add_remove_resources(void)
> qemu_uuid_generate(&uuid);
> dmabuf_fd = g_random_int_range(3, 500);
> /* Add a new resource */
> - g_assert(virtio_add_dmabuf(&uuid, dmabuf_fd));
> - g_assert_cmpint(virtio_lookup_dmabuf(&uuid), ==, dmabuf_fd);
> + g_assert(virtio_dmabuf_add(&uuid, dmabuf_fd));
> + g_assert_cmpint(virtio_dmabuf_lookup(&uuid), ==, dmabuf_fd);
> /* Remove the resource */
> - g_assert(virtio_remove_resource(&uuid));
> + g_assert(virtio_dmabuf_remove_resource(&uuid));
> /* Resource is not found anymore */
> - g_assert_cmpint(virtio_lookup_dmabuf(&uuid), ==, -1);
> + g_assert_cmpint(virtio_dmabuf_lookup(&uuid), ==, -1);
> }
> }
>
> @@ -48,13 +48,13 @@ static void test_add_remove_dev(void)
>
> for (i = 0; i < 100; ++i) {
> qemu_uuid_generate(&uuid);
> - virtio_add_vhost_device(&uuid, dev);
> + virtio_dmabuf_add_vhost_device(&uuid, dev);
> /* vhost device is found */
> - g_assert(virtio_lookup_vhost_device(&uuid) != NULL);
> + g_assert(virtio_dmabuf_lookup_vhost_device(&uuid) != NULL);
> /* Remove the vhost device */
> - g_assert(virtio_remove_resource(&uuid));
> + g_assert(virtio_dmabuf_remove_resource(&uuid));
> /* vhost device is not found anymore */
> - g_assert(virtio_lookup_vhost_device(&uuid) == NULL);
> + g_assert(virtio_dmabuf_lookup_vhost_device(&uuid) == NULL);
> }
> g_free(dev);
> }
> @@ -66,9 +66,9 @@ static void test_remove_invalid_resource(void)
>
> for (i = 0; i < 20; ++i) {
> qemu_uuid_generate(&uuid);
> - g_assert_cmpint(virtio_lookup_dmabuf(&uuid), ==, -1);
> + g_assert_cmpint(virtio_dmabuf_lookup(&uuid), ==, -1);
> /* Removing a resource that does not exist returns false */
> - g_assert_false(virtio_remove_resource(&uuid));
> + g_assert_false(virtio_dmabuf_remove_resource(&uuid));
> }
> }
>
> @@ -81,25 +81,25 @@ static void test_add_invalid_resource(void)
> for (i = 0; i < 20; ++i) {
> qemu_uuid_generate(&uuid);
> /* Add a new resource with invalid (negative) resource fd */
> - g_assert_false(virtio_add_dmabuf(&uuid, dmabuf_fd));
> + g_assert_false(virtio_dmabuf_add(&uuid, dmabuf_fd));
> /* Resource is not found */
> - g_assert_cmpint(virtio_lookup_dmabuf(&uuid), ==, -1);
> + g_assert_cmpint(virtio_dmabuf_lookup(&uuid), ==, -1);
> /* Add a new vhost device with invalid (NULL) pointer */
> - g_assert_false(virtio_add_vhost_device(&uuid, dev));
> + g_assert_false(virtio_dmabuf_add_vhost_device(&uuid, dev));
> /* vhost device is not found */
> - g_assert(virtio_lookup_vhost_device(&uuid) == NULL);
> + g_assert(virtio_dmabuf_lookup_vhost_device(&uuid) == NULL);
> }
>
> for (i = 0; i < 20; ++i) {
> /* Add a valid resource */
> qemu_uuid_generate(&uuid);
> dmabuf_fd = g_random_int_range(3, 500);
> - g_assert(virtio_add_dmabuf(&uuid, dmabuf_fd));
> - g_assert_cmpint(virtio_lookup_dmabuf(&uuid), ==, dmabuf_fd);
> + g_assert(virtio_dmabuf_add(&uuid, dmabuf_fd));
> + g_assert_cmpint(virtio_dmabuf_lookup(&uuid), ==, dmabuf_fd);
> /* Add a new resource with repeated uuid returns false */
> - g_assert_false(virtio_add_dmabuf(&uuid, alt_dmabuf));
> + g_assert_false(virtio_dmabuf_add(&uuid, alt_dmabuf));
> /* The value for the uuid key is not replaced */
> - g_assert_cmpint(virtio_lookup_dmabuf(&uuid), ==, dmabuf_fd);
> + g_assert_cmpint(virtio_dmabuf_lookup(&uuid), ==, dmabuf_fd);
> }
> }
>
> @@ -143,13 +143,13 @@ static void test_free_resources(void)
> for (i = 0; i < ARRAY_SIZE(uuids); ++i) {
> qemu_uuid_generate(&uuids[i]);
> dmabuf_fd = g_random_int_range(3, 500);
> - g_assert(virtio_add_dmabuf(&uuids[i], dmabuf_fd));
> - g_assert_cmpint(virtio_lookup_dmabuf(&uuids[i]), ==, dmabuf_fd);
> + g_assert(virtio_dmabuf_add(&uuids[i], dmabuf_fd));
> + g_assert_cmpint(virtio_dmabuf_lookup(&uuids[i]), ==, dmabuf_fd);
> }
> - virtio_free_resources();
> + virtio_dmabuf_free_resources();
> for (i = 0; i < ARRAY_SIZE(uuids); ++i) {
> /* None of the resources is found after free'd */
> - g_assert_cmpint(virtio_lookup_dmabuf(&uuids[i]), ==, -1);
> + g_assert_cmpint(virtio_dmabuf_lookup(&uuids[i]), ==, -1);
> }
>
> }
> --
> 2.43.0
>
>
Agh I forgot to rename the function in the new test after applying the last
review.
I have it fixed in my local branch and passing the tests. I'll wait for
feedback
on the other commits and push a new version after.
