[Mesa-dev] [PATCH] glx: Demand success from CreateContext requests

[Olivier Fourdan]

Hey

On Fri, Aug 3, 2018 at 7:44 PM Adam Jackson <ajax at redhat.com> wrote:
>
> GLXCreate{,New}Context, like most X resource creation requests, does not
> emit a reply and therefore is emitted into the X stream asynchronously.
> However, unlike most resource creation requests, the GLXContext we
> return is a handle to library state instead of an XID. So if context
> creation fails for any reason - say, the server doesn't support indirect
> contexts - then we will fail in strange places for strange reasons.
>
> We could make every GLX entrypoint robust against half-created contexts,
> or we could just verify that context creation worked. Reuse the
> __glXIsDirect code to do this, as a cheap way of verifying that the
> XID is real.
>
> glXCreateContextAttribsARB solves this by using the _checked version of
> the xcb command, so effectively this change makes the classic context
> creation paths as robust as CreateContextAttribs.
>
> Signed-off-by: Adam Jackson <ajax at redhat.com>
> ---
>  src/glx/glxcmds.c | 92 +++++++++++++++++++++++++++--------------------
>  1 file changed, 54 insertions(+), 38 deletions(-)
>
> diff --git a/src/glx/glxcmds.c b/src/glx/glxcmds.c
> index 4db0228eaba..3789f55d038 100644
> --- a/src/glx/glxcmds.c
> +++ b/src/glx/glxcmds.c
> @@ -272,6 +272,43 @@ glx_context_init(struct glx_context *gc,
>     return True;
>  }
>
> +/**
> + * Determine if a context uses direct rendering.
> + *
> + * \param dpy        Display where the context was created.
> + * \param contextID  ID of the context to be tested.
> + * \param error      Out parameter, set to True on error if not NULL
> + *
> + * \returns \c True if the context is direct rendering or not.
> + */
> +static Bool
> +__glXIsDirect(Display * dpy, GLXContextID contextID, int *error)

Nitpicking, maybe a Bool would be more explicit here (even if it's the
same), it's set to “None” and later set to “True”.

> +{
> +   CARD8 opcode;
> +   xcb_connection_t *c;
> +   xcb_generic_error_t *err;
> +   xcb_glx_is_direct_reply_t *reply;
> +   Bool is_direct;
> +
> +   opcode = __glXSetupForCommand(dpy);
> +   if (!opcode) {
> +      return False;
> +   }
> +
> +   c = XGetXCBConnection(dpy);
> +   reply = xcb_glx_is_direct_reply(c, xcb_glx_is_direct(c, contextID), &err);
> +   is_direct = (reply != NULL && reply->is_direct) ? True : False;
> +
> +   if (err != NULL) {
> +      *error = True;

glXIsDirect() passes “NULL” as “error”, but it's set unconditionally here.

> +      __glXSendErrorForXcb(dpy, err);
> +      free(err);
> +   }
> +
> +   free(reply);
> +
> +   return is_direct;
> +}
>
>  /**
>   * Create a new context.
> @@ -376,6 +413,21 @@ CreateContext(Display *dpy, int generic_id, struct glx_config *config,
>     gc->share_xid = shareList ? shareList->xid : None;
>     gc->imported = GL_FALSE;
>
> +   /* Unlike most X resource creation requests, we're about to return a handle
> +    * with client-side state, not just an XID. To simplify error handling
> +    * elsewhere in libGL, force a round-trip here to ensure the CreateContext
> +    * request above succeeded.
> +    */
> +   {
> +      int error = None;
> +      int isDirect = __glXIsDirect(dpy, gc->xid, &error);
> +
> +      if (error != None || isDirect != gc->isDirect) {
> +         gc->vtable->destroy(gc);
> +         gc = NULL;
> +      }
> +   }
> +
>     return (GLXContext) gc;
>  }
>
> @@ -612,42 +664,6 @@ glXCopyContext(Display * dpy, GLXContext source_user,
>  }
>
>
> -/**
> - * Determine if a context uses direct rendering.
> - *
> - * \param dpy        Display where the context was created.
> - * \param contextID  ID of the context to be tested.
> - *
> - * \returns \c True if the context is direct rendering or not.
> - */
> -static Bool
> -__glXIsDirect(Display * dpy, GLXContextID contextID)
> -{
> -   CARD8 opcode;
> -   xcb_connection_t *c;
> -   xcb_generic_error_t *err;
> -   xcb_glx_is_direct_reply_t *reply;
> -   Bool is_direct;
> -
> -   opcode = __glXSetupForCommand(dpy);
> -   if (!opcode) {
> -      return False;
> -   }
> -
> -   c = XGetXCBConnection(dpy);
> -   reply = xcb_glx_is_direct_reply(c, xcb_glx_is_direct(c, contextID), &err);
> -   is_direct = (reply != NULL && reply->is_direct) ? True : False;
> -
> -   if (err != NULL) {
> -      __glXSendErrorForXcb(dpy, err);
> -      free(err);
> -   }
> -
> -   free(reply);
> -
> -   return is_direct;
> -}
> -
>  /**
>   * \todo
>   * Shouldn't this function \b always return \c False when
> @@ -668,7 +684,7 @@ glXIsDirect(Display * dpy, GLXContext gc_user)
>  #ifdef GLX_USE_APPLEGL  /* TODO: indirect on darwin */
>     return False;
>  #else
> -   return __glXIsDirect(dpy, gc->xid);
> +   return __glXIsDirect(dpy, gc->xid, NULL);
>  #endif
>  }
>
> @@ -1428,7 +1444,7 @@ glXImportContextEXT(Display *dpy, GLXContextID contextID)
>        return NULL;
>     }
>
> -   if (__glXIsDirect(dpy, contextID))
> +   if (__glXIsDirect(dpy, contextID, NULL))
>        return NULL;
>
>     opcode = __glXSetupForCommand(dpy);
> --
> 2.17.0

OIther than the two points above, it works in my test case and avoids
the [xcb] “Unknown sequence number while processing queue” when trying
to catch the XError is IsDirect().


Cheers,
Olivier