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

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

[marcandre.lureau]

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

[Philippe Mathieu-Daudé]

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

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 1413 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 2394 bytes --]

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

[Mark Cave-Ayland]

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.

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

[Philippe Mathieu-Daudé]

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