[PATCHv2] Remove static MAXSCREENS limit from Xext/shm.c.

[Eric Anholt]

On Thu, 2009-10-01 at 10:47 -0700, Jamey Sharp wrote:
> ---
> I didn't test my previous patch right. Sorry. This version doesn't seem
> to crash the server at startup. :-)
> 
> Review would still be greatly appreciated.

Thanks for jumping in!  Comments inline.

>  Xext/shm.c |   67 +++++++++++++++++++++++++++++++++++++++++++----------------
>  1 files changed, 49 insertions(+), 18 deletions(-)
> 
> diff --git a/Xext/shm.c b/Xext/shm.c
> index a6f804c..b4167ac 100644
> --- a/Xext/shm.c
> +++ b/Xext/shm.c
> @@ -99,6 +99,11 @@ typedef struct _ShmDesc {
>      unsigned long size;
>  } ShmDescRec, *ShmDescPtr;
>  
> +typedef struct _ShmScrPrivateRec {
> +    ShmFuncsPtr shmFuncs;
> +    DestroyPixmapProcPtr destroyPixmap;
> +} ShmScrPrivateRec;
> +
>  static PixmapPtr fbShmCreatePixmap(XSHM_CREATE_PIXMAP_ARGS);
>  static int ShmDetachSegment(
>      pointer		/* value */,
> @@ -135,13 +140,16 @@ int BadShmSegCode;
>  RESTYPE ShmSegType;
>  static ShmDescPtr Shmsegs;
>  static Bool sharedPixmaps;
> -static ShmFuncsPtr shmFuncs[MAXSCREENS];
> -static DestroyPixmapProcPtr destroyPixmap[MAXSCREENS];
> +static DrawablePtr *drawables;
> +static int shmScrPrivateKeyIndex;
> +static DevPrivateKey shmScrPrivateKey = &shmScrPrivateKeyIndex;
>  static int shmPixmapPrivateIndex;
>  static DevPrivateKey shmPixmapPrivate = &shmPixmapPrivateIndex;
>  static ShmFuncs miFuncs = {NULL, NULL};
>  static ShmFuncs fbFuncs = {fbShmCreatePixmap, NULL};
>  
> +#define ShmGetScreenPriv(s) ((ShmScrPrivateRec *)dixLookupPrivate(&(s)->devPrivates, shmScrPrivateKey))
> +
>  #define VERIFY_SHMSEG(shmseg,shmdesc,client) \
>  { \
>      int rc; \
> @@ -212,6 +220,18 @@ static Bool CheckForShmSyscall(void)
>  
>  #endif
>  
> +static ShmScrPrivateRec *
> +ShmInitScreenPriv(ScreenPtr pScreen)
> +{
> +    ShmScrPrivateRec *priv = ShmGetScreenPriv(pScreen);
> +    if (!priv)
> +    {
> +	priv = xcalloc (1, sizeof (ShmScrPrivateRec));
> +	dixSetPrivate(&pScreen->devPrivates, shmScrPrivateKey, priv);
> +    }
> +    return priv;
> +}
> +
>  void
>  ShmExtensionInit(INITARGS)
>  {
> @@ -226,20 +246,29 @@ ShmExtensionInit(INITARGS)
>      }
>  #endif
>  
> +    drawables = xcalloc(screenInfo.numScreens, sizeof(DrawablePtr));
> +    if (!drawables)
> +    {
> +	ErrorF("MIT-SHM extension disabled: no memory for per-screen drawables\n");
> +	return;
> +    }

Seems like this drawable pointer should be part of the screen private.

> +
>      sharedPixmaps = xFalse;
>      {
>        sharedPixmaps = xTrue;
>        for (i = 0; i < screenInfo.numScreens; i++)
>        {
> -	if (!shmFuncs[i])
> -	    shmFuncs[i] = &miFuncs;
> -	if (!shmFuncs[i]->CreatePixmap)
> +	ShmScrPrivateRec *priv = ShmInitScreenPriv(screenInfo.screens[i]);
> +	if (!priv->shmFuncs)
> +	    priv->shmFuncs = &miFuncs;
> +	if (!priv->shmFuncs->CreatePixmap)
>  	    sharedPixmaps = xFalse;
>        }
>        if (sharedPixmaps)
>  	for (i = 0; i < screenInfo.numScreens; i++)
>  	{
> -	    destroyPixmap[i] = screenInfo.screens[i]->DestroyPixmap;
> +	    ShmScrPrivateRec *priv = ShmGetScreenPriv(screenInfo.screens[i]);
> +	    priv->destroyPixmap = screenInfo.screens[i]->DestroyPixmap;
>  	    screenInfo.screens[i]->DestroyPixmap = ShmDestroyPixmap;
>  	}
>      }
> @@ -261,23 +290,21 @@ static void
>  ShmResetProc(ExtensionEntry *extEntry)
>  {
>      int i;
> -
> -    for (i = 0; i < MAXSCREENS; i++)
> -    {
> -	shmFuncs[i] = NULL;
> -    }
> +    for (i = 0; i < screenInfo.numScreens; i++)
> +	ShmRegisterFuncs(screenInfo.screens[i], NULL);
>  }
>  
>  void
>  ShmRegisterFuncs(ScreenPtr pScreen, ShmFuncsPtr funcs)
>  {
> -    shmFuncs[pScreen->myNum] = funcs;
> +    ShmInitScreenPriv(pScreen)->shmFuncs = funcs;
>  }

I think this one might be a GetScreenPriv instead?  I'm guessing that
ShmExtensionInit happens (set up protocol stuff), then later on DDXes
call ShmRegisterFuncs on their screen.

>  
>  static Bool
>  ShmDestroyPixmap (PixmapPtr pPixmap)
>  {
>      ScreenPtr	    pScreen = pPixmap->drawable.pScreen;
> +    ShmScrPrivateRec *priv;
>      Bool	    ret;
>      if (pPixmap->refcnt == 1)
>      {
> @@ -288,9 +315,10 @@ ShmDestroyPixmap (PixmapPtr pPixmap)
>  	    ShmDetachSegment ((pointer) shmdesc, pPixmap->drawable.id);
>      }
>      
> -    pScreen->DestroyPixmap = destroyPixmap[pScreen->myNum];
> +    priv = ShmGetScreenPriv(pScreen);
> +    pScreen->DestroyPixmap = priv->destroyPixmap;
>      ret = (*pScreen->DestroyPixmap) (pPixmap);
> -    destroyPixmap[pScreen->myNum] = pScreen->DestroyPixmap;
> +    priv->destroyPixmap = pScreen->DestroyPixmap;
>      pScreen->DestroyPixmap = ShmDestroyPixmap;
>      return ret;
>  }
> @@ -298,7 +326,7 @@ ShmDestroyPixmap (PixmapPtr pPixmap)
>  void
>  ShmRegisterFbFuncs(ScreenPtr pScreen)
>  {
> -    shmFuncs[pScreen->myNum] = &fbFuncs;
> +    ShmRegisterFuncs(pScreen, &fbFuncs);
>  }
>  
>  static int
> @@ -578,7 +606,6 @@ static int
>  ProcPanoramiXShmGetImage(ClientPtr client)
>  {
>      PanoramiXRes	*draw;
> -    DrawablePtr 	drawables[MAXSCREENS];
>      DrawablePtr 	pDraw;
>      xShmGetImageReply	xgi;
>      ShmDescPtr		shmdesc;
> @@ -767,9 +794,11 @@ CreatePmap:
>      result = (client->noClientException);
>  
>      FOR_NSCREENS(j) {
> +	ShmScrPrivateRec *priv;
>  	pScreen = screenInfo.screens[j];
>  
> -	pMap = (*shmFuncs[j]->CreatePixmap)(pScreen, 
> +	priv = ShmGetScreenPriv(pScreen);

Stylistically, I like private fetching to be part of the declaration.
Less chance for early dereferencing.  Also, as many layers have screen
privates, pixmap privates, gc privates, etc., it can be nice to name the
variables for the privates screen_priv, pixmap_priv, etc.

> +	pMap = (*priv->shmFuncs->CreatePixmap)(pScreen,
>  				stuff->width, stuff->height, stuff->depth,
>  				shmdesc->addr + stuff->offset);
>  
> @@ -1052,6 +1081,7 @@ ProcShmCreatePixmap(ClientPtr client)
>      DepthPtr pDepth;
>      int i, rc;
>      ShmDescPtr shmdesc;
> +    ShmScrPrivateRec *priv;
>      REQUEST(xShmCreatePixmapReq);
>      unsigned int width, height, depth;
>      unsigned long size;
> @@ -1100,7 +1130,8 @@ CreatePmap:
>  	return BadAlloc;
>  
>      VERIFY_SHMSIZE(shmdesc, stuff->offset, size, client);
> -    pMap = (*shmFuncs[pDraw->pScreen->myNum]->CreatePixmap)(
> +    priv = ShmGetScreenPriv(pDraw->pScreen);
> +    pMap = (*priv->shmFuncs->CreatePixmap)(
>  			    pDraw->pScreen, stuff->width,
>  			    stuff->height, stuff->depth,
>  			    shmdesc->addr + stuff->offset);
-- 
Eric Anholt
eric at anholt.net                         eric.anholt at intel.com


-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part
Url : http://lists.x.org/archives/xorg-devel/attachments/20091001/2fbc59eb/attachment.pgp