[PATCH v4 xserver] xwayland: Don't block in DRI2 authentication mechanism
Kristian Høgsberg
hoegsberg at gmail.com
Thu Feb 7 17:20:04 PST 2013
On Thu, Feb 07, 2013 at 12:52:27PM +0200, Tiago Vignatti wrote:
> Signed-off-by: Tiago Vignatti <tiago.vignatti at intel.com>
> ---
> 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
Ok, we're almost there - I almost pushed this, but I caught two things
in the patch that needs to be fixed.
> hw/xfree86/dri2/dri2.c | 18 ++++++----
> hw/xfree86/dri2/dri2.h | 17 ++++++----
> hw/xfree86/dri2/dri2ext.c | 27 +++++++++++----
> 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, 95 insertions(+), 30 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..1b37abe 100644
> --- a/hw/xfree86/dri2/dri2ext.c
> +++ b/hw/xfree86/dri2/dri2ext.c
> @@ -133,24 +133,37 @@ ProcDRI2Connect(ClientPtr client)
> return Success;
> }
>
> +void
> +DRI2SendAuthReply(ClientPtr client, Bool status)
> +{
> + xDRI2AuthenticateReply rep;
> +
> + REQUEST_SIZE_MATCH(xDRI2AuthenticateReq);
> + 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;
>
> - REQUEST_SIZE_MATCH(xDRI2AuthenticateReq);
I don't know why you moved REQUEST_SIZE_MATCH to DRI2SendAuthReply, it
needs to be here. The macro checks that the current request from the
client is exactly the size of xDRI2AuthenticateReq and we need to
verify that before we start to deference anything in stuff.
> if (!validDrawable(client, stuff->window, DixGetAttrAccess,
> &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..26a5448 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_add(&req->link, &xwl_screen->authenticate_client_list);
We need to append (use xorg_list_append) the requests. When we get
the authenticate event from wayland, the handler assumes that it's the
response to the head of the list corresponds. xorg_list_add prepends
to the list and makes the new request the list head. When the wl_drm
authenticate event comes in, we end up calling DRI2SendAuthReply for a
drm magic we haven't even sent to the wayland server yet. (This was
also in v3, I just didn't catch it then).
> +
> + 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