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 <[email protected]>
>> Reviewed-by: Akihiko Odaki <[email protected]>
>> Reviewed-by: Markus Armbruster <[email protected]>
>> ---
>> 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
