On 2025/11/20 4:16, Dmitry Osipenko wrote:
On 11/18/25 15:32, Honglei Huang wrote:
On 2025/11/18 09:48, Dmitry Osipenko wrote:
On 11/17/25 16:22, Markus Armbruster wrote:
Dmitry Osipenko <[email protected]> writes:
On 11/17/25 13:51, Honglei Huang wrote:
diff --git a/hw/display/virtio-gpu-rutabaga.c b/hw/display/virtio-
gpu-rutabaga.c
index ed5ae52acb..ea2928b706 100644
--- a/hw/display/virtio-gpu-rutabaga.c
+++ b/hw/display/virtio-gpu-rutabaga.c
@@ -466,7 +466,7 @@ rutabaga_cmd_attach_backing(VirtIOGPU *g,
struct virtio_gpu_ctrl_command *cmd)
ret = virtio_gpu_create_mapping_iov(g, att_rb.nr_entries,
sizeof(att_rb),
cmd, NULL, &res->iov,
&res->iov_cnt);
- CHECK(!ret, cmd);
+ CHECK(ret >= 0, cmd);
virtio_gpu_create_mapping_iov() doesn't return positive values, don't
see how this change improves anything. You now saying that ret > 0 is
okay, while it shall never happen.
Please see
Subject: Re: [PATCH] virtio-gpu-virgl: fix error handling in
virgl_cmd_resource_create_blob
Date: Mon, 17 Nov 2025 08:49:42 +0100
Message-ID: <[email protected]>
https://lore.kernel.org/qemu-devel/[email protected]/
It's a rather common bug when errno isn't negated by mistake and a
positive error code is returned. Ignoring positive values when they
aren't expected opens door to unnecessary problems, IMO.
How about apply the v2 or v3 firstly to fix the
virtio_gpu_create_mapping_iov() block issue in virtio-gpu?
I will create another thread for the `CHECK(!ret, cmd);` thing in rutabaga.
There was a precedent of virtio-gpu not handling positive error codes
properly [1]. To me there is no problem that needs to be fixed when
virtio_gpu_create_mapping_iov() is never expected to return positive
values and doesn't return them.
[1]
https://lore.kernel.org/qemu-devel/[email protected]/
It's a common expectation that errors are negative. But in practice it's
not always true, especially when interacting with external code.
Functionally this patch doesn't change anything. Will leave to Alex and
Akihiko to decide on it.
I'm fine either way.
This is a "trade-off" scenario where both two options have upsides and
downsides. I often avoid making an argument in that case because it
tends to be subjective and cannot be confirmed, leading to bikeshedding.
In my opinion, such a function should not return int in the first place,
but should return bool. The wide range of values int can take causes
problems; bool allows compilers to enforce that it only returns two values.
Sign is one of those problems. int leads to a mistake like returning
errno instead of -errno.
Functions may have completely different ideas for integer return values.
In case of virtio-gpu, virtio_gpu_create_mapping_iov() returns -1 but
virtio_gpu_update_dmabuf() returns -EINVAL. This is confusing.
Sometimes it is still preferred to keep functions returning int for
consistency, but looking at include/hw/virtio/virtio-gpu.h, three return
bool and three return int, so returning int here does not improve
consistency.
That all said, this is a bug fix, not refactoring. This patch does fix a
bug and avoids polluting the codebase, so it is fine for me. Perhaps it
may be nice to have a patch to convert all functions returning int to
bool to avoid pitfalls and improve consistency, but that can be done later.
Regards,
Akihiko Odaki
