[RFC PATCH] Let XineramaGetImageData look up drawables itself.

Jamey Sharp jamey at minilop.net
Fri Apr 23 18:22:58 PDT 2010


Advantages:
- Spares callers from allocating a MAXSCREENS-sized temporary array.
- Hides implementation details of the PanoramiXRes info array.
- Avoids looking up drawables on screens that are entirely clipped out
  of the image bounds.

Disadvantages:

When called in a loop, may look up the same drawable multiple times.
This only affects XYPixmaps and images larger than 256kB, so it should
have minimal real impact.

Validates Xinerama-allocated XIDs late, so if the Xinerama wrapper
didn't initialize the PanoramiXRes info array correctly, that error
can't always be reported correctly. It seems to me that if we have a
valid PanoramiXRes that this client is allowed access to, we ought to be
able to get all the per-screen drawables that go with it too, so I don't
think this error case should be possible.

Signed-off-by: Jamey Sharp <jamey at minilop.net>
---

Have I analyzed this correctly? And especially, can I assume that
per-screen drawables are accessible if the PanoramiXRes is? If so,
perhaps errors should just be ignored here?

 Xext/panoramiX.c      |   28 ++++++++++++++++++++--------
 Xext/panoramiXprocs.c |   38 ++++++++++++++++----------------------
 Xext/panoramiXsrv.h   |    6 ++++--
 Xext/shm.c            |   31 ++++++++++---------------------
 4 files changed, 50 insertions(+), 53 deletions(-)

diff --git a/Xext/panoramiX.c b/Xext/panoramiX.c
index 96eb8f9..50fa970 100644
--- a/Xext/panoramiX.c
+++ b/Xext/panoramiX.c
@@ -1135,9 +1135,11 @@ CopyBits(char *dst, int shiftL, char *src, int bytes)
    1 bpp and planar data to be already cleared when presented
    to this function */
 
-void
+int
 XineramaGetImageData(
-    DrawablePtr *pDrawables,
+    PanoramiXRes *xr_drawable,
+    ClientPtr client,
+    Mask access_mode,
     int left,
     int top,
     int width, 
@@ -1150,10 +1152,14 @@ XineramaGetImageData(
 ){
     RegionRec SrcRegion, GrabRegion;
     BoxRec SrcBox, *pbox;
-    int x, y, w, h, i, j, nbox, size, sizeNeeded, ScratchPitch, inOut, depth;
-    DrawablePtr pDraw = pDrawables[0];
+    int x, y, w, h, i, j, nbox, size, sizeNeeded, ScratchPitch, inOut, depth, rc;
+    DrawablePtr pDraw;
     char *ScratchMem = NULL;
 
+    rc = dixLookupDrawable(&pDraw, xr_drawable->info[0].id, client, 0, access_mode);
+    if (rc != Success)
+	return rc;
+
     size = 0;
 
     /* find box in logical screen space */
@@ -1172,9 +1178,15 @@ XineramaGetImageData(
     depth = (format == XYPixmap) ? 1 : pDraw->depth;
 
     for(i = 0; i < PanoramiXNumScreens; i++) {
-	pDraw = pDrawables[i];
-
 	inOut = RECT_IN_REGION(pScreen,&XineramaScreenRegions[i],&SrcBox);
+	if (inOut == rgnOUT)
+	    continue;
+
+	if (i != 0) { /* already found first drawable outside the loop */
+	    rc = dixLookupDrawable(&pDraw, xr_drawable->info[i].id, client, 0, access_mode);
+	    if (rc != Success)
+		break;
+	}
 
 	if(inOut == rgnIN) {	   
 	    (*pDraw->pScreen->GetImage)(pDraw, 
@@ -1182,8 +1194,7 @@ XineramaGetImageData(
 			SrcBox.y1 - pDraw->y - panoramiXdataPtr[i].y, 
 			width, height, format, planemask, data);
 	    break;
-	} else if (inOut == rgnOUT)
-	    continue;
+	}
 
 	REGION_INTERSECT(pScreen, &GrabRegion, &SrcRegion, 
 					&XineramaScreenRegions[i]);
@@ -1280,4 +1291,5 @@ XineramaGetImageData(
 
     REGION_UNINIT(pScreen, &SrcRegion);
     REGION_UNINIT(pScreen, &GrabRegion);
+    return rc;
 }
diff --git a/Xext/panoramiXprocs.c b/Xext/panoramiXprocs.c
index 6834efb..3baa8f9 100644
--- a/Xext/panoramiXprocs.c
+++ b/Xext/panoramiXprocs.c
@@ -1050,31 +1050,30 @@ int PanoramiXCopyArea(ClientPtr client)
     srcx = stuff->srcX; srcy = stuff->srcY;
     dstx = stuff->dstX; dsty = stuff->dstY;
     if((dst->type == XRT_PIXMAP) && (src->type == XRT_WINDOW)) {
-	DrawablePtr drawables[MAXSCREENS];
-	DrawablePtr pDst;
+	DrawablePtr pSrc, pDst;
 	GCPtr pGC;
         char *data;
 	int pitch, rc;
 
-	FOR_NSCREENS(j) {
-	    rc = dixLookupDrawable(drawables+j, src->info[j].id, client, 0,
+	rc = dixLookupDrawable(&pSrc, src->info[0].id, client, 0,
 				   DixGetAttrAccess);
-	    if (rc != Success)
-		return rc;
-	}
+	if (rc != Success)
+	    return rc;
 
-	pitch = PixmapBytePad(stuff->width, drawables[0]->depth); 
+	pitch = PixmapBytePad(stuff->width, pSrc->depth);
 	if(!(data = xcalloc(1, stuff->height * pitch)))
 	    return BadAlloc;
 
-	XineramaGetImageData(drawables, srcx, srcy, 
+	rc = XineramaGetImageData(src, client, DixGetAttrAccess, srcx, srcy,
 		stuff->width, stuff->height, ZPixmap, ~0, data, pitch, 
 		srcIsRoot);
+	if (rc != Success)
+	    return rc;
 
 	FOR_NSCREENS_BACKWARD(j) {
 	    stuff->gc = gc->info[j].id;
 	    VALIDATE_DRAWABLE_AND_GC(dst->info[j].id, pDst, DixWriteAccess);
-	    if(drawables[0]->depth != pDst->depth) {
+	    if(pSrc->depth != pDst->depth) {
 		client->errorValue = stuff->dstDrawable;
 		xfree(data);
 		return (BadMatch);
@@ -1805,13 +1804,12 @@ int PanoramiXPutImage(ClientPtr client)
 
 int PanoramiXGetImage(ClientPtr client)
 {
-    DrawablePtr 	drawables[MAXSCREENS];
     DrawablePtr 	pDraw;
     PanoramiXRes	*draw;
     xGetImageReply	xgi;
     Bool		isRoot;
     char		*pBuf;
-    int         	i, x, y, w, h, format, rc;
+    int         	x, y, w, h, format, rc;
     Mask		plane = 0, planemask;
     int			linesDone, nlines, linesPerBuf;
     long		widthBytesLine, length;
@@ -1869,14 +1867,6 @@ int PanoramiXGetImage(ClientPtr client)
 	    return(BadMatch);
     }
 
-    drawables[0] = pDraw;
-    for(i = 1; i < PanoramiXNumScreens; i++) {
-	rc = dixLookupDrawable(drawables+i, draw->info[i].id, client, 0,
-			       DixGetAttrAccess);
-	if (rc != Success)
-	    return rc;
-    }
-
     xgi.visual = wVisual (((WindowPtr) pDraw));
     xgi.type = X_Reply;
     xgi.sequenceNumber = client->sequence;
@@ -1923,8 +1913,10 @@ int PanoramiXGetImage(ClientPtr client)
 	    if(pDraw->depth == 1)
 		bzero(pBuf, nlines * widthBytesLine);
 
-	    XineramaGetImageData(drawables, x, y + linesDone, w, nlines,
+	    rc = XineramaGetImageData(draw, client, DixGetAttrAccess, x, y + linesDone, w, nlines,
 			format, planemask, pBuf, widthBytesLine, isRoot);
+	    if (rc != Success)
+		ErrorF("tried to read from unknown Xinerama drawables\n");
 
 		(void)WriteToClient(client,
 				    (int)(nlines * widthBytesLine),
@@ -1940,9 +1932,11 @@ int PanoramiXGetImage(ClientPtr client)
 
 		    bzero(pBuf, nlines * widthBytesLine);
 
-		    XineramaGetImageData(drawables, x, y + linesDone, w, 
+		    rc = XineramaGetImageData(draw, client, DixGetAttrAccess, x, y + linesDone, w,
 					nlines, format, plane, pBuf,
 					widthBytesLine, isRoot);
+		    if (rc != Success)
+			ErrorF("tried to read from unknown Xinerama drawables\n");
 
 		    (void)WriteToClient(client,
 				    (int)(nlines * widthBytesLine),
diff --git a/Xext/panoramiXsrv.h b/Xext/panoramiXsrv.h
index c77b119..f4e0c61 100644
--- a/Xext/panoramiXsrv.h
+++ b/Xext/panoramiXsrv.h
@@ -40,8 +40,10 @@ extern _X_EXPORT unsigned long XRT_COLORMAP;
 typedef Bool (*XineramaVisualsEqualProcPtr)(VisualPtr, ScreenPtr, VisualPtr);
 extern _X_EXPORT XineramaVisualsEqualProcPtr XineramaVisualsEqualPtr;
 
-extern _X_EXPORT void XineramaGetImageData(
-    DrawablePtr *pDrawables,
+extern _X_EXPORT int XineramaGetImageData(
+    PanoramiXRes *xr_drawable,
+    ClientPtr client,
+    Mask access_mode,
     int left,
     int top,
     int width, 
diff --git a/Xext/shm.c b/Xext/shm.c
index ab58c27..7fc6675 100644
--- a/Xext/shm.c
+++ b/Xext/shm.c
@@ -610,11 +610,10 @@ static int
 ProcPanoramiXShmGetImage(ClientPtr client)
 {
     PanoramiXRes	*draw;
-    DrawablePtr 	*drawables;
     DrawablePtr 	pDraw;
     xShmGetImageReply	xgi;
     ShmDescPtr		shmdesc;
-    int         	i, x, y, w, h, format, rc;
+    int         	x, y, w, h, format, rc;
     Mask		plane = 0, planemask;
     long		lenPer = 0, length, widthBytesLine;
     Bool		isRoot;
@@ -671,21 +670,6 @@ ProcPanoramiXShmGetImage(ClientPtr client)
 	    return(BadMatch);
     }
 
-    drawables = xcalloc(PanoramiXNumScreens, sizeof(DrawablePtr));
-    if(!drawables)
-	return(BadAlloc);
-
-    drawables[0] = pDraw;
-    for(i = 1; i < PanoramiXNumScreens; i++) {
-	rc = dixLookupDrawable(drawables+i, draw->info[i].id, client, 0, 
-			       DixReadAccess);
-	if (rc != Success)
-	{
-	    xfree(drawables);
-	    return rc;
-	}
-    }
-
     xgi.visual = wVisual(((WindowPtr)pDraw));
     xgi.type = X_Reply;
     xgi.length = 0;
@@ -707,22 +691,27 @@ ProcPanoramiXShmGetImage(ClientPtr client)
 
     if (length == 0) {/* nothing to do */ }
     else if (format == ZPixmap) {
-	    XineramaGetImageData(drawables, x, y, w, h, format, planemask,
+	rc = XineramaGetImageData(draw, client, DixReadAccess,
+					x, y, w, h, format, planemask,
 					shmdesc->addr + stuff->offset,
 					widthBytesLine, isRoot);
+	if (rc != Success)
+	    return rc;
     } else {
 
 	length = stuff->offset;
         for (; plane; plane >>= 1) {
 	    if (planemask & plane) {
-		XineramaGetImageData(drawables, x, y, w, h, 
-				     format, plane, shmdesc->addr + length,
+		rc = XineramaGetImageData(draw, client, DixReadAccess,
+				     x, y, w, h, format, plane,
+				     shmdesc->addr + length,
 				     widthBytesLine, isRoot);
+		if (rc != Success)
+		    return rc;
 		length += lenPer;
 	    }
 	}
     }
-    xfree(drawables);
     
     if (client->swapped) {
 	int n;
-- 
1.7.0



More information about the xorg-devel mailing list