1. Patchew
  2. QEMU
  3. View series

[PATCH v2] hw/display: fail early when multiple virgl devices are requested

marcandre.lureau@redhat.com posted 1 patch 4 years, 7 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20210705104218.1161101-1-marcandre.lureau@redhat.com
hw/display/virtio-gpu-gl.c | 5 +++++
1 file changed, 5 insertions(+)
[PATCH v2] hw/display: fail early when multiple virgl devices are requested
Posted by marcandre.lureau@redhat.com 4 years, 7 months ago 
From: Marc-André Lureau <marcandre.lureau@redhat.com>

This avoids failing to initialize virgl and crashing later on, and clear
the user expectations.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/display/virtio-gpu-gl.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
index aea9700d5c..bc55c4767e 100644
--- a/hw/display/virtio-gpu-gl.c
+++ b/hw/display/virtio-gpu-gl.c
@@ -113,6 +113,11 @@ static void virtio_gpu_gl_device_realize(DeviceState *qdev, Error **errp)
     return;
 #endif
 
+    if (!object_resolve_path_type("", TYPE_VIRTIO_GPU_GL, NULL)) {
+        error_setg(errp, "at most one %s device is permitted", TYPE_VIRTIO_GPU_GL);
+        return;
+    }
+
     if (!display_opengl) {
         error_setg(errp, "opengl is not available");
         return;
-- 
2.29.0
Re: [PATCH v2] hw/display: fail early when multiple virgl devices are requested
Posted by Philippe Mathieu-Daudé 4 years, 7 months ago 
On 7/5/21 12:42 PM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> This avoids failing to initialize virgl and crashing later on, and clear
> the user expectations.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/display/virtio-gpu-gl.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
> index aea9700d5c..bc55c4767e 100644
> --- a/hw/display/virtio-gpu-gl.c
> +++ b/hw/display/virtio-gpu-gl.c
> @@ -113,6 +113,11 @@ static void virtio_gpu_gl_device_realize(DeviceState *qdev, Error **errp)
>      return;
>  #endif
>  
> +    if (!object_resolve_path_type("", TYPE_VIRTIO_GPU_GL, NULL)) {

Isn't the condition inverted?

> +        error_setg(errp, "at most one %s device is permitted", TYPE_VIRTIO_GPU_GL);
> +        return;
> +    }
> +
>      if (!display_opengl) {
>          error_setg(errp, "opengl is not available");
>          return;
>
Re: [PATCH v2] hw/display: fail early when multiple virgl devices are requested
Posted by Marc-André Lureau 4 years, 7 months ago 
Hi

On Mon, Jul 5, 2021 at 3:03 PM Philippe Mathieu-Daudé <philmd@redhat.com>
wrote:

> On 7/5/21 12:42 PM, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > This avoids failing to initialize virgl and crashing later on, and clear
> > the user expectations.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  hw/display/virtio-gpu-gl.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
> > index aea9700d5c..bc55c4767e 100644
> > --- a/hw/display/virtio-gpu-gl.c
> > +++ b/hw/display/virtio-gpu-gl.c
> > @@ -113,6 +113,11 @@ static void
> virtio_gpu_gl_device_realize(DeviceState *qdev, Error **errp)
> >      return;
> >  #endif
> >
> > +    if (!object_resolve_path_type("", TYPE_VIRTIO_GPU_GL, NULL)) {
>
> Isn't the condition inverted?
>

No, it's easy to misread though. It returns NULL if there are no or
multiple instances.

When realize() is reached the first time, we should have only one instance,
and thus !NULL.


> > +        error_setg(errp, "at most one %s device is permitted",
> TYPE_VIRTIO_GPU_GL);
> > +        return;
> > +    }
> > +
> >      if (!display_opengl) {
> >          error_setg(errp, "opengl is not available");
> >          return;
> >
>
>
>

-- 
Marc-André Lureau
Re: [PATCH v2] hw/display: fail early when multiple virgl devices are requested
Posted by Philippe Mathieu-Daudé 4 years, 7 months ago 
On 7/5/21 1:08 PM, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Jul 5, 2021 at 3:03 PM Philippe Mathieu-Daudé <philmd@redhat.com
> <mailto:philmd@redhat.com>> wrote:
> 
>     On 7/5/21 12:42 PM, marcandre.lureau@redhat.com
>     <mailto:marcandre.lureau@redhat.com> wrote:
>     > From: Marc-André Lureau <marcandre.lureau@redhat.com
>     <mailto:marcandre.lureau@redhat.com>>
>     >
>     > This avoids failing to initialize virgl and crashing later on, and
>     clear
>     > the user expectations.
>     >
>     > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com
>     <mailto:marcandre.lureau@redhat.com>>
>     > ---
>     >  hw/display/virtio-gpu-gl.c | 5 +++++
>     >  1 file changed, 5 insertions(+)
>     >
>     > diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
>     > index aea9700d5c..bc55c4767e 100644
>     > --- a/hw/display/virtio-gpu-gl.c
>     > +++ b/hw/display/virtio-gpu-gl.c
>     > @@ -113,6 +113,11 @@ static void
>     virtio_gpu_gl_device_realize(DeviceState *qdev, Error **errp)
>     >      return;
>     >  #endif
>     > 
>     > +    if (!object_resolve_path_type("", TYPE_VIRTIO_GPU_GL, NULL)) {
> 
>     Isn't the condition inverted?
> 
> 
> No, it's easy to misread though. It returns NULL if there are no or
> multiple instances.
> 
> When realize() is reached the first time, we should have only one
> instance, and thus !NULL.

/**
 * object_resolve_path_type:
 * @path: the path to resolve
 * @typename: the type to look for.
 * @ambiguous: returns true if the path resolution failed because of an
 *   ambiguous match
 *
 * This is similar to object_resolve_path.  However, when looking for a
 * partial path only matches that implement the given type are considered.
 ...
 *
 * Returns: The matched object or NULL on path lookup failure.
 */

/**
 * object_resolve_path:
 ...
 * A successful result is only returned if
 * only one match is found.  If more than one match is found, a flag is
 * returned to indicate that the match was ambiguous.
 *
 * Returns: The matched object or NULL on path lookup failure.
 */

OK... but kinda obfuscated.

Could we add some helper to simplify code review, such:

bool object_type_instance_is_singleton(const char *typename);

(or better name)?

>     > +        error_setg(errp, "at most one %s device is permitted",
>     TYPE_VIRTIO_GPU_GL);
>     > +        return;
>     > +    }
>     > +
>     >      if (!display_opengl) {
>     >          error_setg(errp, "opengl is not available");
>     >          return;
>     >
> 
> 
> 
> 
> -- 
> Marc-André Lureau
Re: [PATCH v2] hw/display: fail early when multiple virgl devices are requested
Posted by Mark Cave-Ayland 4 years, 7 months ago 
On 05/07/2021 11:42, marcandre.lureau@redhat.com wrote:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> This avoids failing to initialize virgl and crashing later on, and clear
> the user expectations.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   hw/display/virtio-gpu-gl.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
> index aea9700d5c..bc55c4767e 100644
> --- a/hw/display/virtio-gpu-gl.c
> +++ b/hw/display/virtio-gpu-gl.c
> @@ -113,6 +113,11 @@ static void virtio_gpu_gl_device_realize(DeviceState *qdev, Error **errp)
>       return;
>   #endif
>   
> +    if (!object_resolve_path_type("", TYPE_VIRTIO_GPU_GL, NULL)) {
> +        error_setg(errp, "at most one %s device is permitted", TYPE_VIRTIO_GPU_GL);
> +        return;
> +    }
> +
>       if (!display_opengl) {
>           error_setg(errp, "opengl is not available");
>           return;

Looks good to me :)

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.