[PATCH] Save major/minor opcodes in ClientRec for RecordAReply

walter harms wharms at bfs.de
Mon Nov 14 14:24:26 PST 2011



Am 14.11.2011 18:04, schrieb Keith Packard:
> 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


It seems that XSERVER_REQUEST_DONE depends on XSERVER_DTRACE perhaps you can do a
#ifndef SERVER_DTRACE
#define XSERVER_REQUEST_DONE()
#endif

and keep the #ifdef XSERVER_DTRACE stuff outside the code ?

re,
 wh


>  
> @@ -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);
> 
> 
> 
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel


More information about the xorg-devel mailing list