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>
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
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
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
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 <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
>
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
© 2016 - 2025 Red Hat, Inc.