Re: [PATCH 1/2] Add RegionInitBoxes(), and fix some buggy callers of RegionInit().

[Soeren Sandmann]

Keith Packard <kei...@keithp.com> writes:

> On Wed, 30 Mar 2011 11:34:09 -0400, Søren Sandmann <sandm...@cs.au.dk> wrote:
>
>> This patch adds a new function RegionInitBoxes() and fixes those
>> instances to call that instead.
>
> What's the memory allocation story here? Does RegionInitBoxes allocate
> space and copy the provided boxes in (in which case a bit of error
> detection is required)? Or does the region just end up pointing at the
> provided boxes (in which case I'd like to see how the region gets
> freed or resized)

RegionInitBoxes() allocates space and copies the boxes. If memory
allocation fails, it will create a not-a-region which will generally
survive anything you do with it.

But it probably still does make sense to handle these errors
explicitly. New patch below.

Thanks,
Soren



commit 43e47433cf55c98db5308a51c1c514c59901f598
Author: Søren Sandmann Pedersen <s...@redhat.com>
Date:   Tue Mar 29 13:06:36 2011 -0400

    Add RegionInitBoxes(), and fix some buggy callers of RegionInit().
    
    The interface to RegionInit():
    
        RegionInit (RegionPtr pReg, BoxPtr rect, int size);
    
    is very confusing because it doesn't take a list of boxes, it takes
    *one* box, but if that box is NULL, it initializes an empty region
    with 'size' rectangles preallocated.
    
    Most callers of this function were correctly passing either NULL or
    just one box, but there were three confused cases, where the code
    seems to expect a region to be created from a list of boxes.
    
    This patch adds a new function RegionInitBoxes() and fixes those
    instances to call that instead.
    
    And yes, the pixman function to initialize a region from a list of
    boxes is called init_rects() because pixman is also awesome.
    
    V2: Make RegionInitBoxes() return a Bool indicating whether the call
        succeeded, and fix the callers to check this return value.
    
    Signed-off-by: Soren Sandmann <s...@redhat.com>

diff --git a/exa/exa_unaccel.c b/exa/exa_unaccel.c
index bd533c4..078b91c 100644
--- a/exa/exa_unaccel.c
+++ b/exa/exa_unaccel.c
@@ -127,11 +127,10 @@ ExaCheckCopyNtoN (DrawablePtr pSrc, DrawablePtr pDst,  
GCPtr pGC,
     EXA_FALLBACK(("from %p to %p (%c,%c)\n", pSrc, pDst,
                  exaDrawableLocation(pSrc), exaDrawableLocation(pDst)));
 
-    if (pExaScr->prepare_access_reg) {
+    if (pExaScr->prepare_access_reg && RegionInitBoxes(&reg, pbox, nbox)) {
        PixmapPtr pPixmap = exaGetDrawablePixmap(pSrc);
 
        exaGetDrawableDeltas(pSrc, pPixmap, &xoff, &yoff);
-       RegionInit(&reg, pbox, nbox);
        RegionTranslate(&reg, xoff + dx, yoff + dy);
        pExaScr->prepare_access_reg(pPixmap, EXA_PREPARE_SRC, &reg);
        RegionUninit(&reg);
@@ -140,11 +139,11 @@ ExaCheckCopyNtoN (DrawablePtr pSrc, DrawablePtr pDst,  
GCPtr pGC,
 
     if (pExaScr->prepare_access_reg &&
        !exaGCReadsDestination(pDst, pGC->planemask, pGC->fillStyle,
-                              pGC->alu, pGC->clientClipType)) {
+                              pGC->alu, pGC->clientClipType) &&
+       RegionInitBoxes (&reg, pbox, nbox)) {
        PixmapPtr pPixmap = exaGetDrawablePixmap(pDst);
 
        exaGetDrawableDeltas(pSrc, pPixmap, &xoff, &yoff);
-       RegionInit(&reg, pbox, nbox);
        RegionTranslate(&reg, xoff, yoff);
        pExaScr->prepare_access_reg(pPixmap, EXA_PREPARE_DEST, &reg);
        RegionUninit(&reg);
diff --git a/glx/glxdri.c b/glx/glxdri.c
index 3a57337..a2afbc2 100644
--- a/glx/glxdri.c
+++ b/glx/glxdri.c
@@ -831,11 +831,20 @@ static void __glXReportDamage(__DRIdrawable *driDraw,
 
     __glXenterServer(GL_FALSE);
 
-    RegionInit(&region, (BoxPtr) rects, num_rects);
-    RegionTranslate(&region, pDraw->x, pDraw->y);
-    DamageDamageRegion(pDraw, &region);
-    RegionUninit(&region);
-
+    if (RegionInitBoxes(&region, (BoxPtr) rects, num_rects)) {
+       RegionTranslate(&region, pDraw->x, pDraw->y);
+       DamageDamageRegion(pDraw, &region);
+       RegionUninit(&region);
+    }
+    else {
+       while (num_rects--) {
+           RegionInit (&region, (BoxPtr) rects++, 1);
+           RegionTranslate(&region, pDraw->x, pDraw->y);
+           DamageDamageRegion(pDraw, &region);
+           RegionUninit(&region);
+       }
+    }
+           
     __glXleaveServer(GL_FALSE);
 }
 
diff --git a/include/regionstr.h b/include/regionstr.h
index 3759fe1..3dfef5c 100644
--- a/include/regionstr.h
+++ b/include/regionstr.h
@@ -132,6 +132,11 @@ static inline void RegionInit(RegionPtr _pReg, BoxPtr 
_rect, int _size)
     }
 }
 
+static inline Bool RegionInitBoxes(RegionPtr pReg, BoxPtr boxes, int nBoxes)
+{
+    return pixman_region_init_rects (pReg, boxes, nBoxes);
+}
+
 static inline void RegionUninit(RegionPtr _pReg)
 {
     if ((_pReg)->data && (_pReg)->data->size) {
_______________________________________________
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel