Honglei Huang <[email protected]> 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 incorrectly checking 'if (!ret)' for
> error conditions.
>
> Change all virtio_gpu_create_mapping_iov() error checks to use 'if (ret < 0)'
> instead of 'if (ret != 0)', 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()
>
> 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
Appreciated!
> - Expanded scope from single function fix to codebase-wide style consistency
Likewise.
> Fixes: 7c092f17ccee ("virtio-gpu: Handle resource blob commands")
> Signed-off-by: Honglei Huang <[email protected]>
> Reviewed-by: Akihiko Odaki <[email protected]>
> ---
> hw/display/virtio-gpu-virgl.c | 4 ++--
> hw/display/virtio-gpu.c | 4 ++--
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> 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;
> }
There are two calls hw/display/virtio-gpu-rutabaga.c:
ret = virtio_gpu_create_mapping_iov(g, att_rb.nr_entries, sizeof(att_rb),
cmd, NULL, &res->iov, &res->iov_cnt);
CHECK(!ret, cmd);
and
if (cblob.blob_mem != VIRTIO_GPU_BLOB_MEM_HOST3D) {
result = virtio_gpu_create_mapping_iov(g, cblob.nr_entries,
sizeof(cblob), cmd,
&res->addrs,
&res->iov, &res->iov_cnt);
CHECK(!result, cmd);
}
CHECK() hides return in a macro, which strikes me as a Bad Idea, but
that's a separate issue. Would it make sense to change to
CHECK(result >= 0)?
