[PATCH] Save major/minor opcodes in ClientRec for RecordAReply
Jamey Sharp
jamey at minilop.net
Mon Nov 14 12:54:27 PST 2011
Reviewed-by: Jamey Sharp <jamey at minilop.net>
On Mon, Nov 14, 2011 at 09:04:18AM -0800, Keith Packard wrote:
> On Tue, 8 Nov 2011 10:47:44 -0800, Jamey Sharp <jamey at minilop.net> wrote:
>
> > And can I persuade you to just inline this directly into its only call
> > site, below, in Dispatch? I don't think factoring it out enhances
> > clarity in this case, especially since this way you need to look up the
> > major opcode in both places.
>
> Yeah, that's looks a lot simpler overall. Here's an updated version
>
> From 9b54a88f8e17a2ab720bb1aa3eb2640b201be4d8 Mon Sep 17 00:00:00 2001
> From: Keith Packard <keithp at keithp.com>
> Date: Tue, 8 Nov 2011 10:13:15 -0800
> Subject: [PATCH] Save major/minor opcodes in ClientRec for RecordAReply
>
> The record extension needs the major and minor opcodes in the reply
> hook, but the request buffer may have been freed by the time the hook
> is invoked. Saving the request major and minor codes as the request is
> executed avoids fetching from the defunct request buffer.
>
> This patch also eliminates the public MinorOpcodeOfRequest function,
> inlining it into Dispatch. Usages of that function have been replaced
> with direct access to the new ClientRec field.
>
> Signed-off-by: Keith Packard <keithp at keithp.com>
> ---
> Xext/security.c | 4 +---
> Xext/xselinux_hooks.c | 4 ++--
> dix/dispatch.c | 23 +++++++++++++----------
> dix/extension.c | 14 --------------
> include/dixstruct.h | 1 +
> include/extension.h | 2 --
> record/record.c | 8 +++-----
> 7 files changed, 20 insertions(+), 36 deletions(-)
>
> diff --git a/Xext/security.c b/Xext/security.c
> index 08d8158..b0d82ab 100644
> --- a/Xext/security.c
> +++ b/Xext/security.c
> @@ -148,9 +148,7 @@ SecurityLabelInitial(void)
> static _X_INLINE const char *
> SecurityLookupRequestName(ClientPtr client)
> {
> - int major = ((xReq *)client->requestBuffer)->reqType;
> - int minor = MinorOpcodeOfRequest(client);
> - return LookupRequestName(major, minor);
> + return LookupRequestName(client->majorOp, client->minorOp);
> }
>
>
> diff --git a/Xext/xselinux_hooks.c b/Xext/xselinux_hooks.c
> index f1d8e5d..0d4c9ab 100644
> --- a/Xext/xselinux_hooks.c
> +++ b/Xext/xselinux_hooks.c
> @@ -263,8 +263,8 @@ SELinuxAudit(void *auditdata,
> if (client) {
> REQUEST(xReq);
> if (stuff) {
> - major = stuff->reqType;
> - minor = MinorOpcodeOfRequest(client);
> + major = client->majorOp;
> + minor = client->minorOp;
> }
> }
> if (audit->id)
> diff --git a/dix/dispatch.c b/dix/dispatch.c
> index 6e33615..b39271f 100644
> --- a/dix/dispatch.c
> +++ b/dix/dispatch.c
> @@ -337,8 +337,6 @@ DisableLimitedSchedulingLatency(void)
> SmartScheduleLatencyLimited = 0;
> }
>
> -#define MAJOROP ((xReq *)client->requestBuffer)->reqType
> -
> void
> Dispatch(void)
> {
> @@ -419,21 +417,28 @@ Dispatch(void)
> }
>
> client->sequence++;
> + client->majorOp = ((xReq *)client->requestBuffer)->reqType;
> + client->minorOp = 0;
> + if (client->majorOp >= EXTENSION_BASE) {
> + ExtensionEntry *ext = GetExtensionEntry(client->majorOp);
> + if (ext)
> + client->minorOp = ext->MinorOpcode(client);
> + }
> #ifdef XSERVER_DTRACE
> - XSERVER_REQUEST_START(LookupMajorName(MAJOROP), MAJOROP,
> + XSERVER_REQUEST_START(LookupMajorName(client->majorOp), client->majorOp,
> ((xReq *)client->requestBuffer)->length,
> client->index, client->requestBuffer);
> #endif
> if (result > (maxBigRequestSize << 2))
> result = BadLength;
> else {
> - result = XaceHookDispatch(client, MAJOROP);
> + result = XaceHookDispatch(client, client->majorOp);
> if (result == Success)
> - result = (* client->requestVector[MAJOROP])(client);
> + result = (* client->requestVector[client->majorOp])(client);
> XaceHookAuditEnd(client, result);
> }
> #ifdef XSERVER_DTRACE
> - XSERVER_REQUEST_DONE(LookupMajorName(MAJOROP), MAJOROP,
> + XSERVER_REQUEST_DONE(LookupMajorName(client->majorOp), client->majorOp,
> client->sequence, client->index, result);
> #endif
>
> @@ -444,8 +449,8 @@ Dispatch(void)
> }
> else if (result != Success)
> {
> - SendErrorToClient(client, MAJOROP,
> - MinorOpcodeOfRequest(client),
> + SendErrorToClient(client, client->majorOp,
> + client->minorOp,
> client->errorValue, result);
> break;
> }
> @@ -466,8 +471,6 @@ Dispatch(void)
> SmartScheduleLatencyLimited = 0;
> }
>
> -#undef MAJOROP
> -
> static int VendorRelease = VENDOR_RELEASE;
> static char *VendorString = VENDOR_NAME;
>
> diff --git a/dix/extension.c b/dix/extension.c
> index c7bbac5..b677cdb 100644
> --- a/dix/extension.c
> +++ b/dix/extension.c
> @@ -228,20 +228,6 @@ StandardMinorOpcode(ClientPtr client)
> return ((xReq *)client->requestBuffer)->data;
> }
>
> -unsigned short
> -MinorOpcodeOfRequest(ClientPtr client)
> -{
> - unsigned char major;
> -
> - major = ((xReq *)client->requestBuffer)->reqType;
> - if (major < EXTENSION_BASE)
> - return 0;
> - major -= EXTENSION_BASE;
> - if (major >= NumExtensions)
> - return 0;
> - return (*extensions[major]->MinorOpcode)(client);
> -}
> -
> void
> CloseDownExtensions(void)
> {
> diff --git a/include/dixstruct.h b/include/dixstruct.h
> index 6cc9614..0a85f40 100644
> --- a/include/dixstruct.h
> +++ b/include/dixstruct.h
> @@ -122,6 +122,7 @@ typedef struct _Client {
>
> DeviceIntPtr clientPtr;
> ClientIdPtr clientIds;
> + unsigned short majorOp, minorOp;
> } ClientRec;
>
> /*
> diff --git a/include/extension.h b/include/extension.h
> index 29a11c3..9249951 100644
> --- a/include/extension.h
> +++ b/include/extension.h
> @@ -52,8 +52,6 @@ _XFUNCPROTOBEGIN
>
> extern _X_EXPORT unsigned short StandardMinorOpcode(ClientPtr /*client*/);
>
> -extern _X_EXPORT unsigned short MinorOpcodeOfRequest(ClientPtr /*client*/);
> -
> extern _X_EXPORT Bool EnableDisableExtension(char *name, Bool enable);
>
> extern _X_EXPORT void EnableDisableExtensionError(char *name, Bool enable);
> diff --git a/record/record.c b/record/record.c
> index 68311ac..4a0fe23 100644
> --- a/record/record.c
> +++ b/record/record.c
> @@ -546,7 +546,7 @@ RecordARequest(ClientPtr client)
> }
> else /* extension, check minor opcode */
> {
> - int minorop = MinorOpcodeOfRequest(client);
> + int minorop = client->minorOp;
> int numMinOpInfo;
> RecordMinorOpPtr pMinorOpInfo = pRCAP->pRequestMinOpInfo;
>
> @@ -603,12 +603,9 @@ RecordAReply(CallbackListPtr *pcbl, pointer nulldata, pointer calldata)
> RecordContextPtr pContext;
> RecordClientsAndProtocolPtr pRCAP;
> int eci;
> - int majorop;
> ReplyInfoRec *pri = (ReplyInfoRec *)calldata;
> ClientPtr client = pri->client;
> - REQUEST(xReq);
>
> - majorop = stuff->reqType;
> for (eci = 0; eci < numEnabledContexts; eci++)
> {
> pContext = ppAllContexts[eci];
> @@ -616,6 +613,7 @@ RecordAReply(CallbackListPtr *pcbl, pointer nulldata, pointer calldata)
> NULL);
> if (pRCAP)
> {
> + int majorop = client->majorOp;
> if (pContext->continuedReply)
> {
> RecordAProtocolElement(pContext, client, XRecordFromServer,
> @@ -635,7 +633,7 @@ RecordAReply(CallbackListPtr *pcbl, pointer nulldata, pointer calldata)
> }
> else /* extension, check minor opcode */
> {
> - int minorop = MinorOpcodeOfRequest(client);
> + int minorop = client->minorOp;
> int numMinOpInfo;
> RecordMinorOpPtr pMinorOpInfo = pRCAP->pReplyMinOpInfo;
> assert (pMinorOpInfo);
> --
> 1.7.7
>
>
>
> --
> keith.packard at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.x.org/archives/xorg-devel/attachments/20111114/0517381c/attachment.pgp>
More information about the xorg-devel
mailing list