[PATCH xserver] X Sync Cleanups
James Jones
jajones at nvidia.com
Wed Dec 15 14:24:07 PST 2010
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;
+ }
+
pCounter = (SyncCounter *)pTrigger->pSync;
return (pCounter == NULL ||
@@ -224,7 +234,16 @@ SyncCheckTriggerNegativeComparison(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;
+ }
+
pCounter = (SyncCounter *)pTrigger->pSync;
return (pCounter == NULL ||
@@ -236,7 +255,16 @@ SyncCheckTriggerPositiveTransition(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;
+ }
+
pCounter = (SyncCounter *)pTrigger->pSync;
return (pCounter == NULL ||
@@ -249,7 +277,16 @@ SyncCheckTriggerNegativeTransition(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;
+ }
+
pCounter = (SyncCounter *)pTrigger->pSync;
return (pCounter == NULL ||
@@ -326,14 +363,6 @@ SyncInitTrigger(ClientPtr client, SyncTrigger *pTrigger, XID syncObject,
}
else
{
- if (pTrigger->test_type != XSyncPositiveTransition &&
- pTrigger->test_type != XSyncNegativeTransition &&
- pTrigger->test_type != XSyncPositiveComparison &&
- pTrigger->test_type != XSyncNegativeComparison)
- {
- client->errorValue = pTrigger->test_type;
- return BadValue;
- }
/* select appropriate CheckTrigger function */
switch (pTrigger->test_type)
@@ -350,6 +379,9 @@ SyncInitTrigger(ClientPtr client, SyncTrigger *pTrigger, XID syncObject,
case XSyncNegativeComparison:
pTrigger->CheckTrigger = SyncCheckTriggerNegativeComparison;
break;
+ default:
+ client->errorValue = pTrigger->test_type;
+ return BadValue;
}
}
}
@@ -402,7 +434,13 @@ SyncSendAlarmNotifyEvents(SyncAlarm *pAlarm)
SyncTrigger *pTrigger = &pAlarm->trigger;
SyncCounter *pCounter;
- assert(!pTrigger->pSync || (SYNC_COUNTER == pTrigger->pSync->type));
+ if (pTrigger->pSync && (SYNC_COUNTER != pTrigger->pSync->type))
+ {
+ ErrorF("Warning: Non-counter XSync object used in alarm. This is\n");
+ ErrorF(" the result of a programming error in the X server.\n");
+ ErrorF(" Counter type: %d\n", pTrigger->pSync->type);
+ return;
+ }
pCounter = (SyncCounter *)pTrigger->pSync;
@@ -507,7 +545,16 @@ SyncAlarmTriggerFired(SyncTrigger *pTrigger)
SyncCounter *pCounter;
CARD64 new_test_value;
- assert(!pTrigger->pSync || (SYNC_COUNTER == pTrigger->pSync->type));
+ if (pTrigger->pSync && (SYNC_COUNTER != pTrigger->pSync->type))
+ {
+ ErrorF("Warning: Non-counter XSync object used in alarm. This "
+ "is\n");
+ ErrorF(" the result of a programming error in the X "
+ "server.\n");
+ ErrorF(" Counter type: %d\n", pTrigger->pSync->type);
+ return;
+ }
+
pCounter = (SyncCounter *)pTrigger->pSync;
/* no need to check alarm unless it's active */
@@ -534,7 +581,15 @@ SyncAlarmTriggerFired(SyncTrigger *pTrigger)
SyncTrigger *paTrigger = &pAlarm->trigger;
SyncCounter *paCounter;
- assert(!paTrigger->pSync || (SYNC_COUNTER == paTrigger->pSync->type));
+ if (paTrigger->pSync && (SYNC_COUNTER != paTrigger->pSync->type))
+ {
+ ErrorF("Warning: Non-counter XSync object used in alarm. This "
+ "is\n");
+ ErrorF(" the result of a programming error in the X "
+ "server.\n");
+ ErrorF(" Counter type: %d\n", pTrigger->pSync->type);
+ return;
+ }
paCounter = (SyncCounter *)pTrigger->pSync;
/* "The alarm is updated by repeatedly adding delta to the
@@ -758,17 +813,15 @@ SyncEventSelectForAlarm(SyncAlarm *pAlarm, ClientPtr client, Bool wantevents)
*/
pClients->delete_id = FakeClientID(client->index);
- if (!AddResource(pClients->delete_id, RTAlarmClient, pAlarm))
- {
- free(pClients);
- return BadAlloc;
- }
/* link it into list after we know all the allocations succeed */
-
pClients->next = pAlarm->pEventClients;
pAlarm->pEventClients = pClients;
pClients->client = client;
+
+ if (!AddResource(pClients->delete_id, RTAlarmClient, pAlarm))
+ return BadAlloc;
+
return Success;
}
@@ -877,17 +930,14 @@ static SyncObject *
SyncCreate(ClientPtr client, XID id, unsigned char type)
{
SyncObject *pSync;
- RESTYPE resType;
switch (type) {
case SYNC_COUNTER:
- resType = RTCounter;
pSync = malloc(sizeof(SyncCounter));
break;
case SYNC_FENCE:
- resType = RTFence;
- pSync = dixAllocateObjectWithPrivates(SyncFence,
- PRIVATE_SYNC_FENCE);
+ pSync = (SyncObject*)dixAllocateObjectWithPrivates(SyncFence,
+ PRIVATE_SYNC_FENCE);
break;
default:
return NULL;
@@ -896,19 +946,6 @@ SyncCreate(ClientPtr client, XID id, unsigned char type)
if (!pSync)
return NULL;
- if (!AddResource(id, resType, (pointer) pSync))
- {
- switch (type) {
- case SYNC_FENCE:
- dixFreeObjectWithPrivates((SyncFence *)pSync, PRIVATE_SYNC_FENCE);
- break;
- default:
- free(pSync);
- }
-
- return NULL;
- }
-
pSync->client = client;
pSync->id = id;
pSync->pTriglist = NULL;
@@ -931,6 +968,10 @@ SyncCreateCounter(ClientPtr client, XSyncCounter id, CARD64 initialvalue)
pCounter->value = initialvalue;
pCounter->pSysCounterInfo = NULL;
+
+ if (!AddResource(id, RTCounter, (pointer) pCounter))
+ return NULL;
+
return pCounter;
}
@@ -1519,15 +1560,12 @@ SyncAwaitPrologue(ClientPtr client, int items)
/* first item is the header, remainder are real wait conditions */
pAwaitUnion->header.delete_id = FakeClientID(client->index);
- if (!AddResource(pAwaitUnion->header.delete_id, RTAwait, pAwaitUnion))
- {
- free(pAwaitUnion);
- return NULL;
- }
-
pAwaitUnion->header.client = client;
pAwaitUnion->header.num_waitconditions = 0;
+ if (!AddResource(pAwaitUnion->header.delete_id, RTAwait, pAwaitUnion))
+ return NULL;
+
return pAwaitUnion;
}
@@ -1754,10 +1792,7 @@ ProcSyncCreateAlarm(ClientPtr client)
}
if (!AddResource(stuff->id, RTAlarm, pAlarm))
- {
- free(pAlarm);
return BadAlloc;
- }
/* see if alarm already triggered. NULL counter will not trigger
* in CreateAlarm and sets alarm state to Inactive.
@@ -1771,7 +1806,17 @@ ProcSyncCreateAlarm(ClientPtr client)
{
SyncCounter *pCounter;
- assert(SYNC_COUNTER == pTrigger->pSync->type);
+ if (pTrigger->pSync && (SYNC_COUNTER != pTrigger->pSync->type))
+ {
+ ErrorF("Warning: Non-counter XSync object used in alarm. This "
+ "is\n");
+ ErrorF(" the result of a programming error in the X "
+ "server.\n");
+ ErrorF(" Counter type: %d\n", pTrigger->pSync->type);
+ FreeResource(stuff->id, RT_NONE);
+ return BadAlloc;
+ }
+
pCounter = (SyncCounter *)pTrigger->pSync;
if ((*pTrigger->CheckTrigger)(pTrigger, pCounter->value))
@@ -1812,8 +1857,18 @@ ProcSyncChangeAlarm(ClientPtr client)
if (pAlarm->trigger.pSync)
{
- assert(SYNC_COUNTER == pAlarm->trigger.pSync->type);
- pCounter = (SyncCounter *)pAlarm->trigger.pSync;
+ if (SYNC_COUNTER == pAlarm->trigger.pSync->type)
+ {
+ pCounter = (SyncCounter *)pAlarm->trigger.pSync;
+ }
+ else
+ {
+ ErrorF("Warning: Non-counter XSync object used in alarm. This "
+ "is\n");
+ ErrorF(" the result of a programming error in the X "
+ "server.\n");
+ ErrorF(" Counter type: %d\n", pAlarm->trigger.pSync->type);
+ }
}
/* see if alarm already triggered. NULL counter WILL trigger
@@ -1928,6 +1983,9 @@ ProcSyncCreateFence(ClientPtr client)
miSyncInitFence(pDraw->pScreen, pFence, stuff->initially_triggered);
+ if (!AddResource(stuff->fid, RTFence, (pointer) pFence))
+ return BadAlloc;
+
return client->noClientException;
}
@@ -2070,7 +2128,7 @@ ProcSyncAwaitFence(ClientPtr client)
}
if (items == 0)
{
- client->errorValue = items; /* XXX protocol change */
+ client->errorValue = items;
return BadValue;
}
@@ -2084,14 +2142,14 @@ ProcSyncAwaitFence(ClientPtr client)
pAwait = &(pAwaitUnion+1)->await; /* skip over header */
for (i = 0; i < items; i++, pProtocolFences++, pAwait++)
{
- if (*pProtocolFences == None) /* XXX protocol change */
+ if (*pProtocolFences == None)
{
/* this should take care of removing any triggers created by
* this request that have already been registered on sync objects
*/
FreeResource(pAwaitUnion->header.delete_id, RT_NONE);
client->errorValue = *pProtocolFences;
- return SyncErrorBase + XSyncBadCounter;
+ return SyncErrorBase + XSyncBadFence;
}
pAwait->trigger.pSync = NULL;
--
1.7.1
More information about the xorg-devel
mailing list