[v1] virtio-gpu-virgl: fix error handling in virgl_cmd_resource_create_blob

[PATCH] virtio-gpu-virgl: fix error handling in virgl_cmd_resource_create_blob

Posted by Honglei Huang 3 weeks, 5 days ago

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 <honghuan@amd.com>
---
 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) {
             cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
             return;
         }
-- 
2.34.1

Re: [PATCH] virtio-gpu-virgl: fix error handling in virgl_cmd_resource_create_blob

Posted by Markus Armbruster 3 weeks, 5 days ago

Honglei Huang <honghuan@amd.com> 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 <honghuan@amd.com>
> ---
>  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;
>          }

Re: [PATCH] virtio-gpu-virgl: fix error handling in virgl_cmd_resource_create_blob

Posted by Honglei Huang 3 weeks, 5 days ago


On 2025/11/17 15:49, Markus Armbruster wrote:
> Honglei Huang <honghuan@amd.com> 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 <honghuan@amd.com>
>> ---
>>   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;
>>           }
> 


I think this change makes sense for consistency. While the CHECK() macro 
does hide the return logic, changing to CHECK(result >= 0) makes the 
error checking convention immediately clear to code readers - that the 
function returns 0 on success and negative values on error. This follows 
the same pattern as the patch for the other virtio-gpu files.

Will update v4.

Regards,
Honglei

Re: [PATCH] virtio-gpu-virgl: fix error handling in virgl_cmd_resource_create_blob

Posted by Honglei Huang 3 weeks, 5 days ago


On 2025/11/17 15:49, Markus Armbruster wrote:
> Honglei Huang <honghuan@amd.com> 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 <honghuan@amd.com>
>> ---
>>   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

Re: [PATCH] virtio-gpu-virgl: fix error handling in virgl_cmd_resource_create_blob

Posted by Akihiko Odaki 3 weeks, 5 days ago

On 2025/11/17 14:51, Honglei Huang wrote:
> 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 <honghuan@amd.com>

Reviewed-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>

This should also have:

Fixes: 7c092f17ccee ("virtio-gpu: Handle resource blob commands")

Re: [PATCH] virtio-gpu-virgl: fix error handling in virgl_cmd_resource_create_blob

Posted by Honglei1.Huang@amd.com 3 weeks, 5 days ago


On 2025/11/17 14:39, Akihiko Odaki wrote:
> On 2025/11/17 14:51, Honglei Huang wrote:
>> 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 <honghuan@amd.com>
> 
> Reviewed-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> 
> This should also have:
> 
> Fixes: 7c092f17ccee ("virtio-gpu: Handle resource blob commands")

Really thanks for the review, will add fixes tag in v2.

Regards,
Honglei