On Wed, 15 Dec 2010 14:24:07 -0800, James Jones <[email protected]> wrote: > Various cleanups identified during review of the > X Sync Fence Object patches. > > -Correctly handle failure of AddResource() > > -Don't assert when data structures are corrupt. > > -Use the default switch label rather than reimplementing > it. > > -Re-introduce cast of result of dixAllocateObjectWithPrivate() > to kill an incompatible pointer type warning. > > -Remove comments claiming protocol updates are needed. One > wasn't true and the other was addressed with a xextproto > change. > > -Return BadFence, not BadCounter from XSyncAwaitFence() > > Signed-off-by: James Jones <[email protected]> > --- > Xext/sync.c | 166 > ++++++++++++++++++++++++++++++++++++++++------------------- > 1 files changed, 112 insertions(+), 54 deletions(-) > > diff --git a/Xext/sync.c b/Xext/sync.c > index ab8f20d..c636263 100644 > --- a/Xext/sync.c > +++ b/Xext/sync.c > @@ -212,7 +212,17 @@ SyncCheckTriggerPositiveComparison(SyncTrigger > *pTrigger, CARD64 oldval) > { > SyncCounter *pCounter; > > - assert(!pTrigger->pSync || (SYNC_COUNTER == pTrigger->pSync->type)); > + > + /* Non-counter sync objects should never get here because they > + * never trigger this comparison. */ > + if (pTrigger->pSync && (SYNC_COUNTER != pTrigger->pSync->type)) > + { > + ErrorF("Warning: Non-counter XSync object using Counter-only\n"); > + ErrorF(" comparison. Result will never be true.\n"); > + ErrorF(" Counter type: %d\n", pTrigger->pSync->type); > + return FALSE; > + } > +
You've got lots of copies of this message; seems like it should be in a shared function. I also worry that you'll end up filling someone's disk with these messages, and so it might be reasonable to limit the server to printing it just once. > - resType = RTFence; > - pSync = dixAllocateObjectWithPrivates(SyncFence, > - PRIVATE_SYNC_FENCE); > + pSync = (SyncObject*)dixAllocateObjectWithPrivates(SyncFence, > + > PRIVATE_SYNC_FENCE); This looks correct; almost makes me wonder if dixAllocateObjectWithPrivates shouldn't be returning a void *... (The whole point of the dixAllocateObjectWithPrivates macro was to avoid needing a cast while providing some type checking.) I read through the AddResources changes and they look correct in patch form, I have not reviewed the rest of the code to make sure you caught all of these cases though. Reviewed-by: Keith Packard <[email protected]> -- [email protected]
pgpQjhLzeetie.pgp
Description: PGP signature
_______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
