On 2025/11/17 15:49, Markus Armbruster wrote:
Honglei Huang <[email protected]> writes:
The error handling logic was incorrect in virgl_cmd_resource_create_blob.
virtio_gpu_create_mapping_iov() returns 0 on success and non-zero on
failure, but the code was checking whether to set the error response.
The fix changes the condition from 'if (!ret)' to 'if (ret != 0)' to
properly handle the return value, consistent with other usage patterns
in the same codebase (see virtio-gpu.c:932 and virtio-gpu.c:354).
Signed-off-by: Honglei Huang <[email protected]>
---
hw/display/virtio-gpu-virgl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 94ddc01f91..e60e1059df 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -701,7 +701,7 @@ static void virgl_cmd_resource_create_blob(VirtIOGPU *g,
ret = virtio_gpu_create_mapping_iov(g, cblob.nr_entries,
sizeof(cblob),
cmd, &res->base.addrs,
&res->base.iov,
&res->base.iov_cnt);
- if (!ret) {
+ if (ret != 0) {
I recommend
if (ret < 0) {
Why?
When a function returns true on success, false on error, we check for
error with
if (!fn(...)) {
Same for functions returning a non-null pointer on success, null on
error.
When a function returns non-negative integer on success, negative
integer on error, we use
if (fn(...) < 0) {
When a function returns zero on success, negative on error, both
if (fn(...) < 0) {
and
if (fn(...)) {
work. I strongly prefer the former. Why?
If fn() returns an integer, fn(...) < 0 is very likely correct (it's
incorrect only if fn() deviates from "return negative on error", which
is a bad idea). If it returns a pointer or bool, fn(...) < 0 won't
compile.
If fn() returns an integer, fn(...) or fn(...) != 0 are likely correct
(same argument). If it doesn't, they are likely backwards.
Because of this, an error check fn(...) == 0 triggers my spider sense
when I read the code: I stop and look up fn(...) to verify the error
check is correct.
Please don't write code that makes me stop and look up things when I
read it :)
cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
return;
}
Hi Markus,
Thank you for the detailed explanation about error checking conventions
in QEMU.
You're absolutely right - using `if (ret < 0)` is much clearer and
follows the established pattern for functions that return 0 on success
and negative on error.
Can I update the patch to use `if (ret < 0)` for all
virtio_gpu_create_mapping_iov() calls in virtio-gpu-virgl.c to maintain
consistency with this convention?
Regards,
Honglei
