Re: [PATCH v2] virtio-gpu: Correct virgl_renderer_resource_get_info() error check

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

Hi Dmitry,

On 29/1/24 08:39, Dmitry Osipenko wrote:
> virgl_renderer_resource_get_info() returns errno and not -1 on error.

So basically we were ignoring all errors...

Could some errors just be safely ignored? Because apparently
this patch now gives troubles, see:
https://gitlab.com/qemu-project/qemu/-/issues/2490
Can you help us there?

Regards,

Phil.

> Correct the return-value check.
> 
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> ---
> 
> v2: - Fixed similar incorrect error-checking in vhost-user-gpu
>      - Added r-b from Marc
> 
>   contrib/vhost-user-gpu/virgl.c | 6 +++---
>   hw/display/virtio-gpu-virgl.c  | 2 +-
>   2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/contrib/vhost-user-gpu/virgl.c b/contrib/vhost-user-gpu/virgl.c
> index d1ccdf7d0668..51da0e3667f9 100644
> --- a/contrib/vhost-user-gpu/virgl.c
> +++ b/contrib/vhost-user-gpu/virgl.c
> @@ -327,7 +327,7 @@ virgl_get_resource_info_modifiers(uint32_t resource_id,
>   #ifdef VIRGL_RENDERER_RESOURCE_INFO_EXT_VERSION
>       struct virgl_renderer_resource_info_ext info_ext;
>       ret = virgl_renderer_resource_get_info_ext(resource_id, &info_ext);
> -    if (ret < 0) {
> +    if (ret) {
>           return ret;
>       }
>   
> @@ -335,7 +335,7 @@ virgl_get_resource_info_modifiers(uint32_t resource_id,
>       *modifiers = info_ext.modifiers;
>   #else
>       ret = virgl_renderer_resource_get_info(resource_id, info);
> -    if (ret < 0) {
> +    if (ret) {
>           return ret;
>       }
>   
> @@ -372,7 +372,7 @@ virgl_cmd_set_scanout(VuGpu *g,
>           uint64_t modifiers = 0;
>           ret = virgl_get_resource_info_modifiers(ss.resource_id, &info,
>                                                   &modifiers);
> -        if (ret == -1) {
> +        if (ret) {
>               g_critical("%s: illegal resource specified %d\n",
>                          __func__, ss.resource_id);
>               cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_RESOURCE_ID;
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index 8bb7a2c21fe7..9f34d0e6619c 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -181,7 +181,7 @@ static void virgl_cmd_set_scanout(VirtIOGPU *g,
>           memset(&info, 0, sizeof(info));
>           ret = virgl_renderer_resource_get_info(ss.resource_id, &info);
>   #endif
> -        if (ret == -1) {
> +        if (ret) {
>               qemu_log_mask(LOG_GUEST_ERROR,
>                             "%s: illegal resource specified %d\n",
>                             __func__, ss.resource_id);