[PATCH v2 xserver] xwayland: Don't roundtrip in DRI2 authentication mechanism
Kristian Høgsberg
hoegsberg at gmail.com
Tue Feb 5 17:38:40 PST 2013
On Mon, Feb 04, 2013 at 06:54:59PM +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
>
> hw/xfree86/dri2/dri2.c | 16 ++++++++++--
> hw/xfree86/dri2/dri2.h | 12 ++++++---
> hw/xfree86/dri2/dri2ext.c | 45 +++++++++++++++++++++++++++++++--
> hw/xfree86/xwayland/xwayland-drm.c | 49 +++++++++++++++++++++++++++++-------
> hw/xfree86/xwayland/xwayland.h | 2 +-
> 5 files changed, 107 insertions(+), 17 deletions(-)
>
> diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
> index b08618a..54efeae 100644
> --- a/hw/xfree86/dri2/dri2.c
> +++ b/hw/xfree86/dri2/dri2.c
> @@ -107,6 +107,7 @@ typedef struct _DRI2Screen {
> DRI2GetMSCProcPtr GetMSC;
> DRI2ScheduleWaitMSCProcPtr ScheduleWaitMSC;
> DRI2AuthMagic2ProcPtr AuthMagic;
> + DRI2AuthMagic3ProcPtr AuthMagic3;
The convention here is that the most recent AuthMagic funtion is just
called "AuthMagic" and all older functions called LegacyAuthMagic[n].
So the DRI2AuthMagic2ProcPtr should be LegacyAuthMagic2 and
DRI2AuthMagic3ProcPtr should just be AuthMagic. The implementation
always calls ->AuthMagic, and if the driver didn't set a
DRI2AuthMagic3ProcPtr handler, we set AuthMagic to the DRI2AuthMagic
helper. The helper will handle authentication by calling
->LegacyAuthMagic. As we're adding yet another auth function,
DRI2AuthMagic needs to also handle the case where the driver only sets
a DRI2AuthMagic2ProcPtr in as well.
> DRI2AuthMagicProcPtr LegacyAuthMagic;
> DRI2ReuseBufferNotifyProcPtr ReuseBufferNotify;
> DRI2SwapLimitValidateProcPtr SwapLimitValidate;
> @@ -1133,6 +1134,17 @@ DRI2Authenticate(ScreenPtr pScreen, uint32_t magic)
> return TRUE;
> }
>
> +Bool
> +DRI2XWaylandAuthenticate(ClientPtr client, ScreenPtr pScreen, uint32_t magic)
This isn't technically xwayland specific, so DRI2Authenticate2 might
be a better name. On the other hand, I had a closer look: even though
we export DRI2Authenticate, which makes it part of the ABI, there's
nothing outside dri2.c that ever calls this. It's not something
drivers ever need to call and AIGLX doesn't need to authenticate
either, since it uses the server drm fd. So we could just unexport it
- leave it in the header file but remote _X_EXPORT, and then change
the prototype to take the client pointer.
> +{
> + DRI2ScreenPtr ds = DRI2GetScreen(pScreen);
> +
> + if (ds == NULL || (*ds->AuthMagic3) (client, pScreen, magic))
> + return FALSE;
> +
> + return TRUE;
> +}
> +
> static int
> DRI2ConfigNotify(WindowPtr pWin, int x, int y, int w, int h, int bw,
> WindowPtr pSib)
> @@ -1222,10 +1234,10 @@ 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->AuthMagic3 = info->AuthMagic3;
> }
>
> if (info->version >= 5) {
> diff --git a/hw/xfree86/dri2/dri2.h b/hw/xfree86/dri2/dri2.h
> index cec9634..1323cd7 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
> @@ -219,9 +221,7 @@ typedef struct {
> * is unnecessary, but it's slightly different in master.
> */
> /* 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;
> + DRI2AuthMagic3ProcPtr AuthMagic3;
> } DRI2InfoRec, *DRI2InfoPtr;
The DRI2InfoRec is also X server ABI, and this one *is* used by
drivers so we can't change it. However, the xwayland branch has
already diverged from master. In master there's
/* added in version 7 */
DRI2GetParamProcPtr GetParam;
before the AuthMagic2 field, so we're already incompatible with
master. In that light, replacing AuthMagic2 with AuthMagic3 is fine,
we just need to be careful when we rebase xwayland onto master.
> extern _X_EXPORT int DRI2EventBase;
> @@ -240,6 +240,12 @@ extern _X_EXPORT Bool DRI2Connect(ScreenPtr pScreen,
>
> extern _X_EXPORT Bool DRI2Authenticate(ScreenPtr pScreen, uint32_t magic);
In other words, what I was talking about above was, remove _X_EXPORT
here and add ClientPtr to the arguments.
> +extern _X_EXPORT void DRI2SendAuthReply(ClientPtr client, int status);
> +
> +extern _X_EXPORT Bool DRI2XWaylandAuthenticate(ClientPtr client,
> + ScreenPtr pScreen,
> + uint32_t magic);
> +
> extern _X_EXPORT int DRI2CreateDrawable(ClientPtr client,
> DrawablePtr pDraw,
> XID id,
> diff --git a/hw/xfree86/dri2/dri2ext.c b/hw/xfree86/dri2/dri2ext.c
> index 2579a5c..30add45 100644
> --- a/hw/xfree86/dri2/dri2ext.c
> +++ b/hw/xfree86/dri2/dri2ext.c
> @@ -46,8 +46,8 @@
> #include "dri2.h"
> #include "protocol-versions.h"
>
> -/* The only xf86 include */
> #include "xf86Module.h"
> +#include "xf86Priv.h" /* xorgWayland */
>
> static ExtensionEntry *dri2Extension;
> extern Bool DRI2ModuleSetup(void);
> @@ -155,6 +155,44 @@ ProcDRI2Authenticate(ClientPtr client)
> return Success;
> }
>
> +void
> +DRI2SendAuthReply(ClientPtr client, int 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);
> +
> + if (client->ignoreCount > 0)
> + AttendClient(client);
> +}
> +
> +static int
> +ProcDRI2XWaylandAuthenticate(ClientPtr client)
> +{
> + REQUEST(xDRI2AuthenticateReq);
> + DrawablePtr pDraw;
> + int status;
> +
> + if (!validDrawable(client, stuff->window, DixGetAttrAccess,
> + &pDraw, &status))
> + return status;
> +
> + if (DRI2XWaylandAuthenticate(client, pDraw->pScreen, stuff->magic)) {
> + IgnoreClient(client);
> + return Success;
> + }
> +
> + /* BadAccess or BadDrawable */
> + DRI2SendAuthReply(client, status);
The rep.authenticated field in the reply is a bool, not the X server
error codes: 1 for successful authentication, 0 for failure. The X
server Success status code is 0, which the clients interpret as "not
authenticated".
> + return Success;
> +}
> +
> static void
> DRI2InvalidateBuffersEvent(DrawablePtr pDraw, void *priv, XID id)
> {
> @@ -551,7 +589,10 @@ ProcDRI2Dispatch(ClientPtr client)
> case X_DRI2Connect:
> return ProcDRI2Connect(client);
> case X_DRI2Authenticate:
> - return ProcDRI2Authenticate(client);
> + if (xorgWayland)
> + return ProcDRI2XWaylandAuthenticate(client);
> + else
> + return ProcDRI2Authenticate(client);
This is not right - we don't want an xorgWayland dependency in here.
The idea was have the callback (the xwayland code is our case) call
IgnoreClient if they don't want to send the reply immdiately. Then we
modify ProcDRI2Authenticate to check client->ignoreCount after when
returning from DRI2Authenticate, and only send the reply if
ignoreCount is 0.
> case X_DRI2CreateDrawable:
> return ProcDRI2CreateDrawable(client);
> case X_DRI2DestroyDrawable:
> diff --git a/hw/xfree86/xwayland/xwayland-drm.c b/hw/xfree86/xwayland/xwayland-drm.c
> index c19eb84..5dd4af4 100644
> --- a/hw/xfree86/xwayland/xwayland-drm.c
> +++ b/hw/xfree86/xwayland/xwayland-drm.c
> @@ -45,6 +45,7 @@
>
> #include "xwayland.h"
> #include "xwayland-private.h"
> +#include "../dri2/dri2.h"
>
> static void
> drm_handle_device (void *data, struct wl_drm *drm, const char *device)
> @@ -143,22 +144,52 @@ 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,
> - uint32_t magic)
> +static DevPrivateKeyRec xwl_client_private_key;
> +
> +static void
> +sync_callback(void *data, struct wl_callback *callback, uint32_t serial)
> {
> - int ret;
> + ClientPtr client = data;
> + struct xwl_screen *xwl_screen;
> + int status;
> +
> + xwl_screen =
> + dixLookupPrivate(&client->devPrivates, &xwl_client_private_key);
> +
> + if (xwl_screen->authenticated)
> + status = Success;
> + else
> + status = BadAccess;
> +
> + DRI2SendAuthReply(client, status);
Same problem as above, the status argument is written into the reply
but the reply uses a bool, not X error codes. You can just pass
xwl_screen->authenticated to DRI2SendAuthReply.
> xwl_screen->authenticated = 0;
> + wl_callback_destroy(callback);
> +}
>
> - if (xwl_screen->drm)
> - wl_drm_authenticate (xwl_screen->drm, magic);
> +static const struct wl_callback_listener sync_listener = {
> + sync_callback
> +};
>
> - ret = wl_display_roundtrip(xwl_screen->display);
> - if (ret == -1)
> - return BadAlloc;
> - if (!xwl_screen->authenticated)
> +int xwl_drm_authenticate(ClientPtr client, struct xwl_screen *xwl_screen,
> + uint32_t magic)
> +{
> + struct wl_callback *callback;
> +
> + if (!xwl_screen->drm)
> + return BadAccess;
> +
> + if (!dixRegisterPrivateKey(&xwl_client_private_key, PRIVATE_CLIENT, 0))
> return BadAlloc;
>
> + dixSetPrivate(&client->devPrivates, &xwl_client_private_key, xwl_screen);
> +
> + xwl_screen->authenticated = 0;
> +
> + wl_drm_authenticate (xwl_screen->drm, magic);
> + callback = wl_display_sync(xwl_screen->display);
> + wl_callback_add_listener(callback, &sync_listener, client);
> +
Looking at this and the way we have to fiddle with client privates,
xwl_screen->authenticated and extra callbacks, I'm thinking that maybe
we're better off just doing one authentication transaction at a time.
When an authentication request comes in we allocate a
struct {
ClientPtr client;
struct xwl_screen *xwl_screen;
struct xorg_list link;
};
and append that to a xwl_screen->authenticate_client_list and call
IgnoreClient for the client. If the list was empty, we can send the
authenticate request for that client immediately, but if there was at
least one other element in the list, it means that we have a
transaction in progress for the head element.
In drm_handle_authenticated, 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.
> return Success;
> }
>
> 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