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; + } + 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 _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
