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