Make virtio_gpu_virgl_unmap_resource_blob() return -1 for more consistency
of error propagation style in the code, adhering to QEMU's devel/style
recommendations and preparing code for further code changes utilizing this
function.

Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 hw/display/virtio-gpu-virgl.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index edcdad0af232..a7b14a312c83 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -199,7 +199,7 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
             qemu_log_mask(LOG_GUEST_ERROR,
                           "%s: failed to unmap virgl resource: %s\n",
                           __func__, strerror(-ret));
-            return ret;
+            return -1;
         }
     } else {
         *cmd_suspended = true;
@@ -333,7 +333,7 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
     }
 
 #if VIRGL_VERSION_MAJOR >= 1
-    if (virtio_gpu_virgl_unmap_resource_blob(g, res, cmd_suspended)) {
+    if (virtio_gpu_virgl_unmap_resource_blob(g, res, cmd_suspended) < 0) {
         cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
         return;
     }
@@ -829,7 +829,7 @@ static void virgl_cmd_resource_unmap_blob(VirtIOGPU *g,
     }
 
     ret = virtio_gpu_virgl_unmap_resource_blob(g, res, cmd_suspended);
-    if (ret) {
+    if (ret < 0) {
         cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
         return;
     }
-- 
2.51.1

Dmitry Osipenko <dmitry.osipenko@collabora.com> writes:

> Make virtio_gpu_virgl_unmap_resource_blob() return -1 for more consistency
> of error propagation style in the code, adhering to QEMU's devel/style
> recommendations and preparing code for further code changes utilizing this
> function.

I'm not so sure of that. If the function is a pass/fail then we tend to
prefer using bools in newer code. If you need richer internal error
reporting then start using your errno defines. If this is user visible
(i.e. during configuration) then we can make more of Error* and friends.

>
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> ---
>  hw/display/virtio-gpu-virgl.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index edcdad0af232..a7b14a312c83 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -199,7 +199,7 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g,
>              qemu_log_mask(LOG_GUEST_ERROR,
>                            "%s: failed to unmap virgl resource: %s\n",
>                            __func__, strerror(-ret));
> -            return ret;
> +            return -1;
>          }
>      } else {
>          *cmd_suspended = true;
> @@ -333,7 +333,7 @@ static void virgl_cmd_resource_unref(VirtIOGPU *g,
>      }
>  
>  #if VIRGL_VERSION_MAJOR >= 1
> -    if (virtio_gpu_virgl_unmap_resource_blob(g, res, cmd_suspended)) {
> +    if (virtio_gpu_virgl_unmap_resource_blob(g, res, cmd_suspended) < 0) {
>          cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
>          return;
>      }
> @@ -829,7 +829,7 @@ static void virgl_cmd_resource_unmap_blob(VirtIOGPU *g,
>      }
>  
>      ret = virtio_gpu_virgl_unmap_resource_blob(g, res, cmd_suspended);
> -    if (ret) {
> +    if (ret < 0) {
>          cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
>          return;
>      }

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

On 11/25/25 15:09, Alex Bennée wrote:
> Dmitry Osipenko <dmitry.osipenko@collabora.com> writes:
> 
>> Make virtio_gpu_virgl_unmap_resource_blob() return -1 for more consistency
>> of error propagation style in the code, adhering to QEMU's devel/style
>> recommendations and preparing code for further code changes utilizing this
>> function.
> I'm not so sure of that. If the function is a pass/fail then we tend to
> prefer using bools in newer code. If you need richer internal error
> reporting then start using your errno defines. If this is user visible
> (i.e. during configuration) then we can make more of Error* and friends.

The current code is a mix of all variants. Will be good to stick with one.

Akihiko, what are your thoughts on the best option for the errors
handling? Would you also prefer bools?

-- 
Best regards,
Dmitry

On 2025/11/26 0:49, Dmitry Osipenko wrote:
> On 11/25/25 15:09, Alex Bennée wrote:
>> Dmitry Osipenko <dmitry.osipenko@collabora.com> writes:
>>
>>> Make virtio_gpu_virgl_unmap_resource_blob() return -1 for more consistency
>>> of error propagation style in the code, adhering to QEMU's devel/style
>>> recommendations and preparing code for further code changes utilizing this
>>> function.
>> I'm not so sure of that. If the function is a pass/fail then we tend to
>> prefer using bools in newer code. If you need richer internal error
>> reporting then start using your errno defines. If this is user visible
>> (i.e. during configuration) then we can make more of Error* and friends.
> 
> The current code is a mix of all variants. Will be good to stick with one.
> 
> Akihiko, what are your thoughts on the best option for the errors
> handling? Would you also prefer bools?

Sorry, I missed this.

I'm for bools. It allows compilers to enforce having either of two 
values, whose semantics is defined in include/qapi/error.h "true on 
success / false on failure"; it says functions should also have "Error 
**errp" but I think the semantics of bool can be applied for those 
without the parameter.

If you are still not sure or there are disagreements, I think it is 
better to ask Markus Armbruster, who maintains the error reporting 
facility shared for the whole codebase.

Regards,
Akihiko Odaki

On 12/5/25 07:08, Akihiko Odaki wrote:
> On 2025/11/26 0:49, Dmitry Osipenko wrote:
>> On 11/25/25 15:09, Alex Bennée wrote:
>>> Dmitry Osipenko <dmitry.osipenko@collabora.com> writes:
>>>
>>>> Make virtio_gpu_virgl_unmap_resource_blob() return -1 for more
>>>> consistency
>>>> of error propagation style in the code, adhering to QEMU's devel/style
>>>> recommendations and preparing code for further code changes
>>>> utilizing this
>>>> function.
>>> I'm not so sure of that. If the function is a pass/fail then we tend to
>>> prefer using bools in newer code. If you need richer internal error
>>> reporting then start using your errno defines. If this is user visible
>>> (i.e. during configuration) then we can make more of Error* and friends.
>>
>> The current code is a mix of all variants. Will be good to stick with
>> one.
>>
>> Akihiko, what are your thoughts on the best option for the errors
>> handling? Would you also prefer bools?
> 
> Sorry, I missed this.
> 
> I'm for bools. It allows compilers to enforce having either of two
> values, whose semantics is defined in include/qapi/error.h "true on
> success / false on failure"; it says functions should also have "Error
> **errp" but I think the semantics of bool can be applied for those
> without the parameter.
> 
> If you are still not sure or there are disagreements, I think it is
> better to ask Markus Armbruster, who maintains the error reporting
> facility shared for the whole codebase.

Thanks for the reply. The bools are fine with me. I removed patches
changing the error handling in a next version of this patchset as they
are a bit controversial and not essential for this series.

-- 
Best regards,
Dmitry