[v4] virtio-gpu: use consistent error checking style for virtio_gpu_create_mapping_iov

[v4] virtio-gpu: use consistent error checking style for virtio_gpu_create_mapping_iov

[Honglei Huang]

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.1

Re: [v4] virtio-gpu: use consistent error checking style for virtio_gpu_create_mapping_iov

[Markus Armbruster]

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>

Re: [v4] virtio-gpu: use consistent error checking style for virtio_gpu_create_mapping_iov

[Dmitry Osipenko]

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.

-- 
Best regards,
Dmitry

Re: [v4] virtio-gpu: use consistent error checking style for virtio_gpu_create_mapping_iov

[Markus Armbruster]

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/

Re: [v4] virtio-gpu: use consistent error checking style for virtio_gpu_create_mapping_iov

[Dmitry Osipenko]

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

Re: [v4] virtio-gpu: use consistent error checking style for virtio_gpu_create_mapping_iov

[Markus Armbruster]

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.

Re: [v4] virtio-gpu: use consistent error checking style for virtio_gpu_create_mapping_iov

[Honglei Huang]

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

Re: [v4] virtio-gpu: use consistent error checking style for virtio_gpu_create_mapping_iov

[Dmitry Osipenko]

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

Re: [v4] virtio-gpu: use consistent error checking style for virtio_gpu_create_mapping_iov

[Akihiko Odaki]

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

Re: [v4] virtio-gpu: use consistent error checking style for virtio_gpu_create_mapping_iov

[Honglei Huang]

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

Re: [v4] virtio-gpu: use consistent error checking style for virtio_gpu_create_mapping_iov

[Dmitry Osipenko]

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

Re: [v4] virtio-gpu: use consistent error checking style for virtio_gpu_create_mapping_iov

[Dmitry Osipenko]

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

Re: [v4] virtio-gpu: use consistent error checking style for virtio_gpu_create_mapping_iov

[Dmitry Osipenko]

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

[v4] virtio-gpu: use consistent error checking style for virtio_gpu_create_mapping_iov

[Honglei Huang]

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>
---
 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.1

Re: [v4] virtio-gpu: use consistent error checking style for virtio_gpu_create_mapping_iov

[Honglei Huang]

On 2025/11/21 15:58, Honglei Huang wrote:
> 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>
> ---
>   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;
>       }

This v4 patch started as a bug fix for error handling in 
`virgl_cmd_resource_create_blob()` but evolved through community 
feedback to include comprehensive code style consistency improvements, 
unifying error checking patterns (`ret < 0`) across all virtio-gpu files.

  This version appears to have gained community consensus and may be 
ready for acceptance.

Correct me if I am wrong.

Regards,
Honglei

Re: [v4] virtio-gpu: use consistent error checking style for virtio_gpu_create_mapping_iov

[Dmitry Osipenko]

On 11/21/25 11:14, Honglei Huang wrote:
> 
> 
> On 2025/11/21 15:58, Honglei Huang wrote:
>> 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>
>> ---
>>   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;
>>       }
> 
> This v4 patch started as a bug fix for error handling in
> `virgl_cmd_resource_create_blob()` but evolved through community
> feedback to include comprehensive code style consistency improvements,
> unifying error checking patterns (`ret < 0`) across all virtio-gpu files.
> 
>  This version appears to have gained community consensus and may be
> ready for acceptance.
> 
> Correct me if I am wrong.

I was previously very puzzled by what bug is fixed and didn't notice it,
seeing only the err < 0 changes. Now I see which code is fixed after you
pointed it out explicitly.

The common practice is:

1. Bug fix patches should contain only fixes
2. All code improvements should be done in separate patches
3. Bugfix patches should be first in the series, improvements follow
them on top of the fixes

So here should be two patches:

1. The virgl_cmd_resource_create_blob() fix
2. virtio_gpu_create_mapping_iov() err handling improvement

-- 
Best regards,
Dmitry