On 2026/02/18 15:16, Kasireddy, Vivek wrote:
Hi Akihiko,
Subject: RE: [PATCH v6 06/11] virtio-gpu-dmabuf: Improve error handling
by introducing 'Error **'
Make the error handling more robust in virtio_gpu_init_udmabuf()
by passing in 'Error **' parameter to capture errors from
virtio_gpu_create_udmabuf() and virtio_gpu_remap_dmabuf().
And, since they now take in 'Error **' parameter, have them
return a bool to adhere to best practices.
Cc: Marc-André Lureau <[email protected]>
Cc: Alex Bennée <[email protected]>
Cc: Akihiko Odaki <[email protected]>
Cc: Dmitry Osipenko <[email protected]>
Cc: Alex Williamson <[email protected]>
Cc: Cédric Le Goater <[email protected]>
Signed-off-by: Vivek Kasireddy <[email protected]>
---
hw/display/virtio-gpu-dmabuf.c | 45 ++++++++++++++++++++++------
-
-----
1 file changed, 30 insertions(+), 15 deletions(-)
diff --git a/hw/display/virtio-gpu-dmabuf.c b/hw/display/virtio-gpu-
dmabuf.c
index 8d67ef7c2a..d9b2ecaf31 100644
--- a/hw/display/virtio-gpu-dmabuf.c
+++ b/hw/display/virtio-gpu-dmabuf.c
@@ -27,7 +27,8 @@
#include "standard-headers/linux/udmabuf.h"
#include "standard-headers/drm/drm_fourcc.h"
-static void virtio_gpu_create_udmabuf(struct
virtio_gpu_simple_resource *res)
+static bool virtio_gpu_create_udmabuf(struct
virtio_gpu_simple_resource *res,
+ Error **errp)
{
g_autofree struct udmabuf_create_list *list = NULL;
RAMBlock *rb;
@@ -36,7 +37,8 @@ static void virtio_gpu_create_udmabuf(struct
virtio_gpu_simple_resource *res)
udmabuf = udmabuf_fd();
if (udmabuf < 0) {
- return;
+ error_setg(errp, "udmabuf device not available");
+ return false;
}
list = g_malloc0(sizeof(struct udmabuf_create_list) +
@@ -45,7 +47,10 @@ static void virtio_gpu_create_udmabuf(struct
virtio_gpu_simple_resource *res)
for (i = 0; i < res->iov_cnt; i++) {
rb = qemu_ram_block_from_host(res->iov[i].iov_base, false,
&offset);
if (!rb || rb->fd < 0) {
- return;
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Could not find valid ramblock\n",
+ __func__);
+ return false;
include/qapi/error.h suggests the following error handling pattern:
> Call a function, receive an error from it, and pass it to the caller
> - when the function returns a value that indicates failure, say
> false:
> if (!foo(arg, errp)) {
> handle the error...
> }
Returning false without passing an error to the caller breaks it so
please don't do that.
That's why I added an additional check in the caller:
if (local_err) {
error_report_err(local_err);
}
Having said that, I am not sure what is the right thing to do here.
IMO, the proper way would be to add error_setg() but you mentioned
doing that would be incorrect given that this is a Guest error.
So, how should I proceed?
Any further thoughts/comments here and below?
Sorry, I missed them.
Here, this function should not require its caller to have such an
additional check. Ideally, the idiom described in include/qapi/error.h
should work.
include/qapi/error.h says:
> - Whenever practical, also return a value that indicates success /
> failure. This can make the error checking more concise, and can
> avoid useless error object creation and destruction. Note that
> we still have many functions returning void. We recommend
> • bool-valued functions return true on success / false on failure,
> • pointer-valued functions return non-null / null pointer, and
> • integer-valued functions return non-negative / negative.
Since this function is deriving a file descriptor, it should return one.
The error condition should be represented by a negative value. It is a
common to return -errno and let the caller distinguish error conditions,
but the error conditions we care cannot properly represented with
-errno, so there should be an enum for the two error conditions: invalid
iov or anything else.
Thanks,
Vivek
}
list->list[i].memfd = rb->fd;
@@ -58,20 +63,26 @@ static void virtio_gpu_create_udmabuf(struct
virtio_gpu_simple_resource *res)
res->dmabuf_fd = ioctl(udmabuf, UDMABUF_CREATE_LIST, list);
if (res->dmabuf_fd < 0) {
- warn_report("%s: UDMABUF_CREATE_LIST: %s", __func__,
- strerror(errno));
+ error_setg_errno(errp, -res->dmabuf_fd,
+ "Could not create dmabuf fd via udmabuf
driver");
This is a guest error since it indicates that the guest specified wrong
addresses.
I don't have a strong opinion here but my understanding is that this
should
not be considered a Guest error because the rb (which we obtained using
Guest addresses) is valid at this point, so the UDMABUF_CREATE_LIST
IOCTL
failure should be treated as a Host error.
Regardless, it is not clear how we can precisely determine who (Guest or
Host)
is responsible for the failure here and in some other places.
rb->fd may still contain something other than memfd.
Assume anything the guest passes can be invalid. iov the guest passes
can be whatever you do not expect.
Regards,
Akihiko Odaki
Thanks,
Vivek
Regards,
Akihiko Odaki
+ return false;
}
+ return true;
}
-static void virtio_gpu_remap_dmabuf(struct
virtio_gpu_simple_resource
*res)
+static bool virtio_gpu_remap_dmabuf(struct
virtio_gpu_simple_resource
*res,
+ Error **errp)
{
- res->remapped = mmap(NULL, res->blob_size, PROT_READ,
- MAP_SHARED, res->dmabuf_fd, 0);
- if (res->remapped == MAP_FAILED) {
- warn_report("%s: dmabuf mmap failed: %s", __func__,
- strerror(errno));
+ void *map;
+
+ map = mmap(NULL, res->blob_size, PROT_READ, MAP_SHARED,
res-
dmabuf_fd, 0);
+ if (map == MAP_FAILED) {
+ error_setg_errno(errp, errno, "dmabuf mmap failed");
res->remapped = NULL;
+ return false;
}
+ res->remapped = map;
+ return true;
}
static void virtio_gpu_destroy_dmabuf(struct
virtio_gpu_simple_resource *res)
@@ -125,6 +136,7 @@ bool virtio_gpu_have_udmabuf(void)
void virtio_gpu_init_dmabuf(struct virtio_gpu_simple_resource
*res)
{
+ Error *local_err = NULL;
void *pdata = NULL;
res->dmabuf_fd = -1;
@@ -132,12 +144,15 @@ void virtio_gpu_init_dmabuf(struct
virtio_gpu_simple_resource *res)
res->iov[0].iov_len < 4096) {
pdata = res->iov[0].iov_base;
} else {
- virtio_gpu_create_udmabuf(res);
- if (res->dmabuf_fd < 0) {
+ if (!virtio_gpu_create_udmabuf(res, &local_err)) {
+ if (local_err) {
+ error_report_err(local_err);
+ }
return;
}
- virtio_gpu_remap_dmabuf(res);
- if (!res->remapped) {
+
+ if (!virtio_gpu_remap_dmabuf(res, &local_err)) {
+ error_report_err(local_err);
return;
}
pdata = res->remapped;
