On 2025/11/22 02:00, Dmitry Osipenko wrote:
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


You are absolutely correct. I apologize for the confusion in my approach.

Will prepare v5 with this two-patch structure, ensuring the bug fix is isolated and can be easily reviewed and backported if needed, while the style improvements are clearly separated as code quality enhancements.

Regards,
Honglei




Reply via email to