hw/display/virtio-gpu-rutabaga.c | 4 ++-- hw/display/virtio-gpu-virgl.c | 4 ++-- hw/display/virtio-gpu.c | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-)
Fix error handling logic in virgl_cmd_resource_create_blob and improve
consistency across the codebase.
virtio_gpu_create_mapping_iov() returns 0 on success and negative values
on error, but the original code was inconsistently checking for error
conditions using different patterns.
Change all virtio_gpu_create_mapping_iov() error checks to use consistent
'ret < 0' or 'ret >= 0' patterns, following the preferred QEMU coding
convention for functions that return 0 on success and negative on error.
This makes the return value convention immediately clear to code readers
without needing to look up the function definition.
Updated locations:
- hw/display/virtio-gpu-virgl.c: virgl_cmd_resource_create_blob()
- hw/display/virtio-gpu-virgl.c: virgl_cmd_resource_attach_backing()
- hw/display/virtio-gpu.c: virtio_gpu_resource_create_blob()
- hw/display/virtio-gpu.c: virtio_gpu_resource_attach_backing()
- hw/display/virtio-gpu-rutabaga.c: rutabaga_cmd_attach_backing()
- hw/display/virtio-gpu-rutabaga.c: rutabaga_cmd_resource_create_blob()
Changes since v3:
- Extended consistency improvements to virtio-gpu-rutabaga.c
- Changed CHECK(!ret) to CHECK(ret >= 0) and CHECK(!result) to
CHECK(result >= 0) in rutabaga functions for consistency
- Now covers all virtio-gpu files that use virtio_gpu_create_mapping_iov()
Changes since v2:
- Use 'if (ret < 0)' instead of 'if (ret != 0)' following maintainer's
feedback on preferred QEMU coding style for error checking functions
that return 0 on success and negative on error
- Updated all similar usages across virtio-gpu files for consistency
- Expanded scope from single function fix to codebase-wide style consistency
Fixes: 7c092f17ccee ("virtio-gpu: Handle resource blob commands")
Signed-off-by: Honglei Huang <honghuan@amd.com>
Reviewed-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
hw/display/virtio-gpu-rutabaga.c | 4 ++--
hw/display/virtio-gpu-virgl.c | 4 ++--
hw/display/virtio-gpu.c | 4 ++--
3 files changed, 6 insertions(+), 6 deletions(-)
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);
vecs.iovecs = res->iov;
vecs.num_iovecs = res->iov_cnt;
@@ -616,7 +616,7 @@ rutabaga_cmd_resource_create_blob(VirtIOGPU *g,
result = virtio_gpu_create_mapping_iov(g, cblob.nr_entries,
sizeof(cblob), cmd, &res->addrs,
&res->iov, &res->iov_cnt);
- CHECK(!result, cmd);
+ CHECK(result >= 0, cmd);
}
rc_blob.blob_id = cblob.blob_id;
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 94ddc01f91..6ebd9293e5 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -557,7 +557,7 @@ static void virgl_resource_attach_backing(VirtIOGPU *g,
ret = virtio_gpu_create_mapping_iov(g, att_rb.nr_entries, sizeof(att_rb),
cmd, NULL, &res_iovs, &res_niov);
- if (ret != 0) {
+ if (ret < 0) {
cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
return;
}
@@ -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) {
cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
return;
}
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 0a1a625b0e..1038c6a49f 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -352,7 +352,7 @@ static void virtio_gpu_resource_create_blob(VirtIOGPU *g,
ret = virtio_gpu_create_mapping_iov(g, cblob.nr_entries, sizeof(cblob),
cmd, &res->addrs, &res->iov,
&res->iov_cnt);
- if (ret != 0) {
+ if (ret < 0) {
cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
g_free(res);
return;
@@ -931,7 +931,7 @@ virtio_gpu_resource_attach_backing(VirtIOGPU *g,
ret = virtio_gpu_create_mapping_iov(g, ab.nr_entries, sizeof(ab), cmd,
&res->addrs, &res->iov, &res->iov_cnt);
- if (ret != 0) {
+ if (ret < 0) {
cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
return;
}
--
2.34.1On 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. -- Best regards, Dmitry
Dmitry Osipenko <dmitry.osipenko@collabora.com> 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: <87ms4lrtd5.fsf@pond.sub.org>
https://lore.kernel.org/qemu-devel/87ms4lrtd5.fsf@pond.sub.org/
On 11/17/25 16:22, Markus Armbruster wrote: > Dmitry Osipenko <dmitry.osipenko@collabora.com> 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: <87ms4lrtd5.fsf@pond.sub.org> > https://lore.kernel.org/qemu-devel/87ms4lrtd5.fsf@pond.sub.org/ 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. -- Best regards, Dmitry
On 2025/11/18 09:48, Dmitry Osipenko wrote: > On 11/17/25 16:22, Markus Armbruster wrote: >> Dmitry Osipenko <dmitry.osipenko@collabora.com> 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: <87ms4lrtd5.fsf@pond.sub.org> >> https://lore.kernel.org/qemu-devel/87ms4lrtd5.fsf@pond.sub.org/ > > 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. Regards, Honglei
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 <dmitry.osipenko@collabora.com> 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: <87ms4lrtd5.fsf@pond.sub.org> >>> https://lore.kernel.org/qemu-devel/87ms4lrtd5.fsf@pond.sub.org/ >> >> 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/20240129073921.446869-1-dmitry.osipenko@collabora.com/ 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. -- Best regards, Dmitry
On 11/19/25 22: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 <dmitry.osipenko@collabora.com> 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: <87ms4lrtd5.fsf@pond.sub.org> >>>> https://lore.kernel.org/qemu-devel/87ms4lrtd5.fsf@pond.sub.org/ >>> >>> 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/20240129073921.446869-1-dmitry.osipenko@collabora.com/ > > 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. Alright, I changed my mind after just typing a code where a fact of error occurrence needs to be propagated. For a code that is internal to QEMU, it should be fine to check for err < 0 because static analysis tools should catch invalid error handling and using same style makes code look more consistent and pleasant to read. For errors originated externally, I'll continue to advocate for checking of both pos and neg values. -- Best regards, Dmitry
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 <dmitry.osipenko@collabora.com> 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: <87ms4lrtd5.fsf@pond.sub.org> >>>> https://lore.kernel.org/qemu-devel/87ms4lrtd5.fsf@pond.sub.org/ >>> >>> 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/20240129073921.446869-1-dmitry.osipenko@collabora.com/ > > 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
On 11/20/25 04:29, Akihiko Odaki wrote: > 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 <dmitry.osipenko@collabora.com> 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: <87ms4lrtd5.fsf@pond.sub.org> >>>>> https://lore.kernel.org/qemu-devel/87ms4lrtd5.fsf@pond.sub.org/ >>>> >>>> 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/20240129073921.446869-1- >> dmitry.osipenko@collabora.com/ >> >> 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. Let's settle with the current variant, I'm okay with it now. -- Best regards, Dmitry
On 2025/11/20 03: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 <dmitry.osipenko@collabora.com> 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: <87ms4lrtd5.fsf@pond.sub.org> >>>> https://lore.kernel.org/qemu-devel/87ms4lrtd5.fsf@pond.sub.org/ >>> >>> 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/20240129073921.446869-1-dmitry.osipenko@collabora.com/ > > 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. > Hi Dmitry, Totally agreed with you. And actually the origin purpose of this patch is to fix virtio-gpu not handling error codes properly. Please see the V2: [1]. The error handling logic was incorrect. 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). So I am suggesting to apply the V2. [1]: https://lore.kernel.org/qemu-devel/20251117073827.114891-1-honghuan@amd.com/ v1: https://lore.kernel.org/qemu-devel/27e24af2-683a-48f2-9b10-e6f4061d49d4@amd.com/ Regards, Honglei
Dmitry Osipenko <dmitry.osipenko@collabora.com> writes: > On 11/17/25 16:22, Markus Armbruster wrote: >> Dmitry Osipenko <dmitry.osipenko@collabora.com> 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: <87ms4lrtd5.fsf@pond.sub.org> >> https://lore.kernel.org/qemu-devel/87ms4lrtd5.fsf@pond.sub.org/ > > 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. No convention can avoid *all* possible problems there. I know which one has created more grief for *me*. Your experience may be different. virtio_gpu_create_mapping_iov() returns -1 on error, 0 on success. We could change it to return false on error, true on success.
On 11/18/25 08:45, Markus Armbruster wrote: > Dmitry Osipenko <dmitry.osipenko@collabora.com> writes: > >> On 11/17/25 16:22, Markus Armbruster wrote: >>> Dmitry Osipenko <dmitry.osipenko@collabora.com> 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: <87ms4lrtd5.fsf@pond.sub.org> >>> https://lore.kernel.org/qemu-devel/87ms4lrtd5.fsf@pond.sub.org/ >> >> 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. > > No convention can avoid *all* possible problems there. I know which one > has created more grief for *me*. Your experience may be different. > > virtio_gpu_create_mapping_iov() returns -1 on error, 0 on success. > We could change it to return false on error, true on success. True/false are also often confusing. As was written in the other email, this variant is now good to me. In the end I'm glad for having this discussion, it will help with further improvement of virtio-gpu code. -- Best regards, Dmitry
Honglei Huang <honghuan@amd.com> writes:
> Fix error handling logic in virgl_cmd_resource_create_blob and improve
> consistency across the codebase.
>
> virtio_gpu_create_mapping_iov() returns 0 on success and negative values
> on error, but the original code was inconsistently checking for error
> conditions using different patterns.
>
> Change all virtio_gpu_create_mapping_iov() error checks to use consistent
> 'ret < 0' or 'ret >= 0' patterns, following the preferred QEMU coding
> convention for functions that return 0 on success and negative on error.
> This makes the return value convention immediately clear to code readers
> without needing to look up the function definition.
>
> Updated locations:
> - hw/display/virtio-gpu-virgl.c: virgl_cmd_resource_create_blob()
> - hw/display/virtio-gpu-virgl.c: virgl_cmd_resource_attach_backing()
> - hw/display/virtio-gpu.c: virtio_gpu_resource_create_blob()
> - hw/display/virtio-gpu.c: virtio_gpu_resource_attach_backing()
> - hw/display/virtio-gpu-rutabaga.c: rutabaga_cmd_attach_backing()
> - hw/display/virtio-gpu-rutabaga.c: rutabaga_cmd_resource_create_blob()
>
> Changes since v3:
> - Extended consistency improvements to virtio-gpu-rutabaga.c
> - Changed CHECK(!ret) to CHECK(ret >= 0) and CHECK(!result) to
> CHECK(result >= 0) in rutabaga functions for consistency
> - Now covers all virtio-gpu files that use virtio_gpu_create_mapping_iov()
>
> Changes since v2:
> - Use 'if (ret < 0)' instead of 'if (ret != 0)' following maintainer's
> feedback on preferred QEMU coding style for error checking functions
> that return 0 on success and negative on error
> - Updated all similar usages across virtio-gpu files for consistency
> - Expanded scope from single function fix to codebase-wide style consistency
>
> Fixes: 7c092f17ccee ("virtio-gpu: Handle resource blob commands")
> Signed-off-by: Honglei Huang <honghuan@amd.com>
> Reviewed-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
© 2016 - 2025 Red Hat, Inc.