[PATCH v5 xserver] xwayland: Don't block in DRI2 authentication mechanism

Kristian Høgsberg hoegsberg at gmail.com
Fri Feb 8 10:35:11 PST 2013


On Fri, Feb 08, 2013 at 02:57:01PM +0200, Tiago Vignatti wrote:
> Signed-off-by: Tiago Vignatti <tiago.vignatti at intel.com>

Cool, this all looks good now, all three patches committed.

Thanks,
Kristian

> v2: - added AuthMagic3, for passing ClientPtr
>     - fixed server internal ABI issue, by maintaining DRI2Authenticate intact
>     - fixed multiple clients accesses to DRI2 authenticate, using Wayland
>       callback mechanism to ensure they all go atomically
> 
> v3: - fixed AuthMagic and LegacyAuthMagic semantics when adding the new
>       DRI2AuthMagic3ProcPtr type
>     - unexported DRI2Authenticate
>     - fixed DRI2SendAuthReply to be more strict about its args ("Bool status")
>     - changed the new nonblocking dri2 authentication to process only one
>       request at time
> 
> v4: - fix DRI2Authenticate return values for the non-xwayland case
> 
> v5: - Don't put REQUEST_SIZE_MATCH out of ProcDRI2Authenticate; that was a
>       typo when moving code around.
>     - append DRI2Authenticate requests using xorg_list_append and not
>       xorg_list_add.
> 
>  hw/xfree86/dri2/dri2.c                 |   18 ++++++----
>  hw/xfree86/dri2/dri2.h                 |   17 ++++++----
>  hw/xfree86/dri2/dri2ext.c              |   25 ++++++++++----
>  hw/xfree86/xwayland/xwayland-drm.c     |   58 +++++++++++++++++++++++++++-----
>  hw/xfree86/xwayland/xwayland-private.h |    1 +
>  hw/xfree86/xwayland/xwayland.c         |    2 ++
>  hw/xfree86/xwayland/xwayland.h         |    2 +-
>  7 files changed, 94 insertions(+), 29 deletions(-)
> 
> diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
> index b08618a..142619c 100644
> --- a/hw/xfree86/dri2/dri2.c
> +++ b/hw/xfree86/dri2/dri2.c
> @@ -106,8 +106,9 @@ typedef struct _DRI2Screen {
>      DRI2ScheduleSwapProcPtr ScheduleSwap;
>      DRI2GetMSCProcPtr GetMSC;
>      DRI2ScheduleWaitMSCProcPtr ScheduleWaitMSC;
> -    DRI2AuthMagic2ProcPtr AuthMagic;
>      DRI2AuthMagicProcPtr LegacyAuthMagic;
> +    DRI2AuthMagic2ProcPtr LegacyAuthMagic2;
> +    DRI2AuthMagic3ProcPtr AuthMagic;
>      DRI2ReuseBufferNotifyProcPtr ReuseBufferNotify;
>      DRI2SwapLimitValidateProcPtr SwapLimitValidate;
>  
> @@ -1123,11 +1124,11 @@ DRI2AuthMagic (ScreenPtr pScreen, uint32_t magic)
>  }
>  
>  Bool
> -DRI2Authenticate(ScreenPtr pScreen, uint32_t magic)
> +DRI2Authenticate(ClientPtr client, ScreenPtr pScreen, uint32_t magic)
>  {
>      DRI2ScreenPtr ds = DRI2GetScreen(pScreen);
>  
> -    if (ds == NULL || (*ds->AuthMagic) (pScreen, magic))
> +    if (ds == NULL || (*ds->AuthMagic) (client, pScreen, magic))
>          return FALSE;
>  
>      return TRUE;
> @@ -1222,12 +1223,15 @@ DRI2ScreenInit(ScreenPtr pScreen, DRI2InfoPtr info)
>       *
>       * If a driver is built without xwayland support, we'll die before we get
>       * here. If a driver is built with xwayland support, it'll support
> -     * AuthMagic2, or crash; we don't care about xwayland ABI yet.
> +     * AuthMagic3, or crash; we don't care about xwayland ABI yet.
>       */
>      if (xorgWayland) {
> -        ds->AuthMagic = info->AuthMagic2;
> +        ds->AuthMagic = info->AuthMagic3;
> +    }
> +
> +    if (info->version >= 8) {
> +        ds->LegacyAuthMagic2 = info->AuthMagic2;
>      }
> -    
>      if (info->version >= 5) {
>          ds->LegacyAuthMagic = info->AuthMagic;
>      }
> @@ -1247,7 +1251,7 @@ DRI2ScreenInit(ScreenPtr pScreen, DRI2InfoPtr info)
>           * If the driver doesn't provide an AuthMagic function
>           * it relies on the old method (using libdrm) or fails
>           */
> -        if (!ds->LegacyAuthMagic)
> +        if (!ds->LegacyAuthMagic2 && !ds->LegacyAuthMagic)
>  #ifdef WITH_LIBDRM
>              ds->LegacyAuthMagic = drmAuthMagic;
>  #else
> diff --git a/hw/xfree86/dri2/dri2.h b/hw/xfree86/dri2/dri2.h
> index cec9634..617cbfb 100644
> --- a/hw/xfree86/dri2/dri2.h
> +++ b/hw/xfree86/dri2/dri2.h
> @@ -65,6 +65,8 @@ typedef void (*DRI2CopyRegionProcPtr) (DrawablePtr pDraw,
>  typedef void (*DRI2WaitProcPtr) (WindowPtr pWin, unsigned int sequence);
>  typedef int (*DRI2AuthMagicProcPtr) (int fd, uint32_t magic);
>  typedef int (*DRI2AuthMagic2ProcPtr) (ScreenPtr pScreen, uint32_t magic);
> +typedef int (*DRI2AuthMagic3ProcPtr) (ClientPtr client,
> +                                      ScreenPtr pScreen, uint32_t magic);
>  
>  /**
>   * Schedule a buffer swap
> @@ -213,15 +215,14 @@ typedef struct {
>      DRI2ReuseBufferNotifyProcPtr ReuseBufferNotify;
>      DRI2SwapLimitValidateProcPtr SwapLimitValidate;
>  
> -    /* added in version 8 AND in xwayland branch */
> -    /* 
> -     * MERGE NOTE: when merging xwayland to master, the commit adding this
> -     * is unnecessary, but it's slightly different in master.
> -     */
> +    /* added in version 8 */
>      /* AuthMagic callback which passes extra context */
>      /* If this is NULL the AuthMagic callback is used */
>      /* If this is non-NULL the AuthMagic callback is ignored */
>      DRI2AuthMagic2ProcPtr AuthMagic2;
> +
> +    /* MERGE NOTE: added in xwayland branch */
> +    DRI2AuthMagic3ProcPtr AuthMagic3;
>  } DRI2InfoRec, *DRI2InfoPtr;
>  
>  extern _X_EXPORT int DRI2EventBase;
> @@ -238,7 +239,11 @@ extern _X_EXPORT Bool DRI2Connect(ScreenPtr pScreen,
>                                    const char **driverName,
>                                    const char **deviceName);
>  
> -extern _X_EXPORT Bool DRI2Authenticate(ScreenPtr pScreen, uint32_t magic);
> +extern Bool DRI2Authenticate(ClientPtr client,
> +                             ScreenPtr pScreen,
> +                             uint32_t magic);
> +
> +extern _X_EXPORT void DRI2SendAuthReply(ClientPtr client, Bool status);
>  
>  extern _X_EXPORT int DRI2CreateDrawable(ClientPtr client,
>                                          DrawablePtr pDraw,
> diff --git a/hw/xfree86/dri2/dri2ext.c b/hw/xfree86/dri2/dri2ext.c
> index 2579a5c..eab3697 100644
> --- a/hw/xfree86/dri2/dri2ext.c
> +++ b/hw/xfree86/dri2/dri2ext.c
> @@ -133,11 +133,23 @@ ProcDRI2Connect(ClientPtr client)
>      return Success;
>  }
>  
> +void
> +DRI2SendAuthReply(ClientPtr client, Bool status)
> +{
> +    xDRI2AuthenticateReply rep;
> +
> +    rep.type = X_Reply;
> +    rep.sequenceNumber = client->sequence;
> +    rep.length = 0;
> +    rep.authenticated = status;
> +
> +    WriteToClient(client, sizeof(xDRI2AuthenticateReply), &rep);
> +}
> +
>  static int
>  ProcDRI2Authenticate(ClientPtr client)
>  {
>      REQUEST(xDRI2AuthenticateReq);
> -    xDRI2AuthenticateReply rep;
>      DrawablePtr pDraw;
>      int status;
>  
> @@ -146,11 +158,12 @@ ProcDRI2Authenticate(ClientPtr client)
>                         &pDraw, &status))
>          return status;
>  
> -    rep.type = X_Reply;
> -    rep.sequenceNumber = client->sequence;
> -    rep.length = 0;
> -    rep.authenticated = DRI2Authenticate(pDraw->pScreen, stuff->magic);
> -    WriteToClient(client, sizeof(xDRI2AuthenticateReply), &rep);
> +    status = DRI2Authenticate(client, pDraw->pScreen, stuff->magic);
> +
> +    /* if non-blocking authentication is in progress, then don't send a reply
> +     * now but later in the implementation (e.g. drm_handle_authenticated) */
> +    if (client->ignoreCount == 0)
> +        DRI2SendAuthReply(client, status);
>  
>      return Success;
>  }
> diff --git a/hw/xfree86/xwayland/xwayland-drm.c b/hw/xfree86/xwayland/xwayland-drm.c
> index c19eb84..8d595b0 100644
> --- a/hw/xfree86/xwayland/xwayland-drm.c
> +++ b/hw/xfree86/xwayland/xwayland-drm.c
> @@ -45,6 +45,15 @@
>  
>  #include "xwayland.h"
>  #include "xwayland-private.h"
> +#include "../dri2/dri2.h"
> +
> +struct xwl_auth_req {
> +    struct xorg_list link;
> +
> +    ClientPtr client;
> +    struct xwl_screen *xwl_screen;
> +    uint32_t magic;
> +};
>  
>  static void
>  drm_handle_device (void *data, struct wl_drm *drm, const char *device)
> @@ -63,8 +72,31 @@ static void
>  drm_handle_authenticated (void *data, struct wl_drm *drm)
>  {
>      struct xwl_screen *xwl_screen = data;
> +    struct xwl_auth_req *req;
>  
>      xwl_screen->authenticated = 1;
> +
> +    /* it does one authentication transaction at a time, so if there's an
> +     * element in the list, we call DRI2SendAuthReply for that client, remove
> +     * the head and free the struct. If there are still elements in the list,
> +     * it means that we have one or more clients waiting to be authenticated
> +     * and we send out a wl_drm authenticate request for the first client in
> +     * the list */
> +    if (xorg_list_is_empty(&xwl_screen->authenticate_client_list))
> +	return;
> +
> +    req = xorg_list_first_entry(&xwl_screen->authenticate_client_list,
> +	                        struct xwl_auth_req, link);
> +    DRI2SendAuthReply(req->client, TRUE);
> +    AttendClient(req->client);
> +    xorg_list_del(&req->link);
> +    free(req);
> +
> +    xorg_list_for_each_entry(req, &xwl_screen->authenticate_client_list,
> +	                     link) {
> +	wl_drm_authenticate (xwl_screen->drm, req->magic);
> +	return;
> +    }
>  }
>  
>  static const struct wl_drm_listener xwl_drm_listener =
> @@ -143,21 +175,29 @@ int xwl_screen_get_drm_fd(struct xwl_screen *xwl_screen)
>      return xwl_screen->drm_fd;
>  }
>  
> -int xwl_drm_authenticate(struct xwl_screen *xwl_screen,
> +int xwl_drm_authenticate(ClientPtr client, struct xwl_screen *xwl_screen,
>  			    uint32_t magic)
>  {
> -    int ret;
> +    struct xwl_auth_req *req;
>  
> -    xwl_screen->authenticated = 0;
> +    if (!xwl_screen->drm)
> +	return BadAccess;
>  
> -    if (xwl_screen->drm)
> +    req = malloc (sizeof *req);
> +    if (req == NULL)
> +	return BadAlloc;
> +
> +    req->client = client;
> +    req->xwl_screen = xwl_screen;
> +    req->magic = magic;
> +
> +    if (xorg_list_is_empty(&xwl_screen->authenticate_client_list))
>  	wl_drm_authenticate (xwl_screen->drm, magic);
>  
> -    ret = wl_display_roundtrip(xwl_screen->display);
> -    if (ret == -1)
> -	return BadAlloc;
> -    if (!xwl_screen->authenticated)
> -	return BadAlloc;
> +    xorg_list_append(&req->link, &xwl_screen->authenticate_client_list);
> +
> +    IgnoreClient(req->client);
> +    xwl_screen->authenticated = 0;
>  
>      return Success;
>  }
> diff --git a/hw/xfree86/xwayland/xwayland-private.h b/hw/xfree86/xwayland/xwayland-private.h
> index 23263d0..5b602d7 100644
> --- a/hw/xfree86/xwayland/xwayland-private.h
> +++ b/hw/xfree86/xwayland/xwayland-private.h
> @@ -61,6 +61,7 @@ struct xwl_screen {
>      struct xorg_list		 seat_list;
>      struct xorg_list		 damage_window_list;
>      struct xorg_list		 window_list;
> +    struct xorg_list		 authenticate_client_list;
>      uint32_t			 serial;
>  
>      CreateWindowProcPtr		 CreateWindow;
> diff --git a/hw/xfree86/xwayland/xwayland.c b/hw/xfree86/xwayland/xwayland.c
> index 98e036e..d97f4ee 100644
> --- a/hw/xfree86/xwayland/xwayland.c
> +++ b/hw/xfree86/xwayland/xwayland.c
> @@ -233,6 +233,7 @@ xwl_screen_pre_init(ScrnInfoPtr scrninfo, struct xwl_screen *xwl_screen,
>      xorg_list_init(&xwl_screen->seat_list);
>      xorg_list_init(&xwl_screen->damage_window_list);
>      xorg_list_init(&xwl_screen->window_list);
> +    xorg_list_init(&xwl_screen->authenticate_client_list);
>      xwl_screen->scrninfo = scrninfo;
>      xwl_screen->driver = driver;
>      xwl_screen->flags = flags;
> @@ -307,6 +308,7 @@ void xwl_screen_close(struct xwl_screen *xwl_screen)
>      xorg_list_init(&xwl_screen->seat_list);
>      xorg_list_init(&xwl_screen->damage_window_list);
>      xorg_list_init(&xwl_screen->window_list);
> +    xorg_list_init(&xwl_screen->authenticate_client_list);
>  
>      wl_display_roundtrip(xwl_screen->display);
>  }
> diff --git a/hw/xfree86/xwayland/xwayland.h b/hw/xfree86/xwayland/xwayland.h
> index e536c42..f268366 100644
> --- a/hw/xfree86/xwayland/xwayland.h
> +++ b/hw/xfree86/xwayland/xwayland.h
> @@ -69,7 +69,7 @@ extern _X_EXPORT void
>  xwl_screen_post_damage(struct xwl_screen *xwl_screen);
>  
>  extern _X_EXPORT int
> -xwl_drm_authenticate(struct xwl_screen *xwl_screen,
> +xwl_drm_authenticate(ClientPtr client, struct xwl_screen *xwl_screen,
>  		     uint32_t magic);
>  
>  extern _X_EXPORT int
> -- 
> 1.7.9.5
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel


More information about the wayland-devel mailing list