[PATCH xserver] X Sync Cleanups

[Keith Packard]

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.

> -	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 <keithp at keithp.com>

-- 
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/20101220/b8ac862b/attachment.pgp>