[PATCH xserver] X Sync Cleanups

[James Jones]

On Monday 20 December 2010 12:45:55 am Keith Packard wrote:
> * PGP Signed by an unknown key
> 
> On Wed, 15 Dec 2010 14:24:07 -0800, James Jones <jajones at nvidia.com> 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 <jajones at nvidia.com>
> > ---
> > 
> >  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.

Fixed in v2.

> > -	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.

Verified again that I fixed all of them in the sync code at least.

> Reviewed-by: Keith Packard <keithp at keithp.com>

Thanks for the review

-James