[PATCHv10 4/5] dri2: Support the DRI2InvalidateBuffers event.

[Keith Packard]

On Tue, 16 Mar 2010 10:26:54 -0400, Kristian Høgsberg <krh at bitplanet.net> wrote:

> Yes, the series look good and I've updated my branch:

I don't like this code. It adds an unnecessary layer between
DRI2TrackClient and the dri2ext code, with a 'priv' element,
'invalidate' and 'destroy' function pointers which are only set to a
single value.

I see this resulting in abuse of the X server resource management
functions:

+    if (!AddResource(*id, dri2ClientRefRes, pDraw))
+	goto fail;

The function here has allocated memory (*id) but that memory is only
incidentally associated with the resource ID which ends up pointing at
the drawable.

I want to see resource IDs point at the allocated object. In this case,
we've got a nice structure, the DRI2ClientRefRec all set to be pointed
at by the resource.

A nice side-effect of this is that we'd have a single allocation for
each tracking object. Plus, we'd have all of the tracking stuff in one
file instead of spread across two with this weird interface between the
two halves.

I think there are also bugs in resource management -- one obvious
concern is that track_client calls DRI2InvalidateDestroy when
AddResource fails, and yet DRI2InvalidateDestroy calls FreeResource on
the failing ID. That's not likely to cause a problem, but it's
sloppy. It's really hard to tell though, there's too many interfaces
involved in creating and deleting stuff.

Take a look at something like the shape extension's ProcShapeSelectInput
function:

	/* build the entry */
    	pNewShapeEvent = xalloc (sizeof (ShapeEventRec));
    	if (!pNewShapeEvent)
	    return BadAlloc;
    	pNewShapeEvent->next = 0;
    	pNewShapeEvent->client = client;
    	pNewShapeEvent->window = pWin;
    	/*
 	 * add a resource that will be deleted when
     	 * the client goes away
     	 */
   	clientResource = FakeClientID (client->index);
    	pNewShapeEvent->clientResource = clientResource;
    	if (!AddResource (clientResource, ClientType, (pointer)pNewShapeEvent))
	    return BadAlloc;

Nice and simple -- it allocates and completely initializes the object
before calling AddResource; if AddResource fails, it will call
ShapeFreeClient which frees the object. The only function necessary to
free this object is FreeResource, which calls ShapeFreeClient to unhook
the object from various lists and free the memory.

I've merged the three patches before the event changes to master to
avoid an ABI change if the event changes look reasonable for a 1.8.x
stable release.

-- 
keith.packard at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg-devel/attachments/20100321/4fc20d06/attachment.pgp>