For the original series and Peter's update, with or without Jamey's change to linked lists, Reviewed-by: Jeremy Huddleston <[email protected]>
On Mar 14, 2012, at 5:26 PM, Jamey Sharp <[email protected]> wrote: > On Wed, Mar 14, 2012 at 4:30 PM, Peter Hutterer > <[email protected]> wrote: >> On Wed, Mar 14, 2012 at 09:01:13AM -0700, Jamey Sharp wrote: >>> This series seems like a good idea to me! I have a few review comments >>> though: >>> >>> Patch 1 seems strange. Shouldn't the counters themselves get freed at >>> reset? If they are, shouldn't that lead to the number of counters >>> reaching 0? >> >> you're right but the order is a tad weird. The ResetProc is called before >> the counters are cleaned up, so we free the container list but the number is >> still correct. Later, the number goes down to 0 when the counters are >> actually freed. Technically correct, but weird. I think the diff below would >> make sense to merge into patch 1/9. This way, we remove all knowledge of >> system counters in the ResetProc and don't get any weird effects. >> >> diff --git a/Xext/sync.c b/Xext/sync.c >> index 6c8c564..bf47c93 100644 >> --- a/Xext/sync.c >> +++ b/Xext/sync.c >> @@ -1200,8 +1200,8 @@ FreeCounter(void *env, XID id) >> SysCounterList[i] = SysCounterList[i+1]; >> } >> } >> + SyncNumSystemCounters--; >> } >> - SyncNumSystemCounters--; >> } >> free(pCounter); >> return Success; > > This seems like increasing the fragility of the code... > > I ran out of time to actually test, but the attached alternative patch > compiles, at least, and the diffstat makes me happy. What say you? > >> in configure.ac:1312, it's always defined, so no conditionals are necessary. >> AC_DEFINE(XSYNC, 1, [Support XSync extension]) > > Good. I assume disabling it at runtime doesn't hurt anything either? > If that's possible... > >>> Here's a request you can ignore if you like: Please consider making >>> SYSCOUNTERPRIV a static inline instead of a macro, and removing the >>> cast from inside it. That'd be different from the usual idiom in the X >>> server, but the usual idiom is dumb. :-) I'd prefer to see all the >>> callers assign their "pointer pCounter"s to an appropriately-typed >>> local once right at the beginning of the function, which also serves >>> as documentation for what type you're supposed to pass in there. >> >> How does this one look then? I'd squash this in but it's easier to review as >> a diff on top. > > Looks perfect, except I'm a bit baffled at the NULL check: I'm hoping > the pCounters can't actually ever be null, but you're making me > wonder...? > >> From cc02b58e95822a50658ef7fe13e862c3f569ee22 Mon Sep 17 00:00:00 2001 >> From: Peter Hutterer <[email protected]> >> Date: Thu, 15 Mar 2012 09:27:24 +1000 >> Subject: [PATCH] Xext: replace the macro with a static inline function. >> >> Signed-off-by: Peter Hutterer <[email protected]> >> --- >> Xext/sync.c | 24 ++++++++++++++++++------ >> Xext/syncsrv.h | 2 -- >> 2 files changed, 18 insertions(+), 8 deletions(-) >> >> diff --git a/Xext/sync.c b/Xext/sync.c >> index 6c8c564..5f74dab 100644 >> --- a/Xext/sync.c >> +++ b/Xext/sync.c >> @@ -115,6 +115,15 @@ static void SyncInitServerTime(void); >> >> static void SyncInitIdleTime(void); >> >> +static inline void* >> +SysCounterGetPrivate(SyncCounter *counter) >> +{ >> + BUG_WARN(!IsSystemCounter(counter)); >> + >> + return counter->pSysCounterInfo ? counter->pSysCounterInfo->private : >> NULL; >> +} >> + >> + >> static Bool >> SyncCheckWarnIsCounter(const SyncObject* pSync, const char *warning) >> { >> @@ -2747,7 +2756,8 @@ IdleTimeQueryValue (pointer pCounter, CARD64 >> *pValue_return) >> CARD32 idle; >> >> if (pCounter) { >> - IdleCounterPriv *priv = SYSCOUNTERPRIV(pCounter); >> + SyncCounter *counter = pCounter; >> + IdleCounterPriv *priv = SysCounterGetPrivate(counter); >> deviceid = priv->deviceid; >> } else >> deviceid = XIAllDevices; >> @@ -2758,8 +2768,8 @@ IdleTimeQueryValue (pointer pCounter, CARD64 >> *pValue_return) >> static void >> IdleTimeBlockHandler(pointer pCounter, struct timeval **wt, pointer >> LastSelectMask) >> { >> - IdleCounterPriv *priv = SYSCOUNTERPRIV(pCounter); >> SyncCounter *counter = pCounter; >> + IdleCounterPriv *priv = SysCounterGetPrivate(counter); >> XSyncValue *less = priv->value_less, >> *greater = priv->value_greater; >> XSyncValue idle, old_idle; >> @@ -2835,7 +2845,8 @@ IdleTimeBlockHandler(pointer pCounter, struct timeval >> **wt, pointer LastSelectMa >> static void >> IdleTimeWakeupHandler (pointer pCounter, int rc, pointer LastSelectMask) >> { >> - IdleCounterPriv *priv = SYSCOUNTERPRIV(pCounter); >> + SyncCounter *counter = pCounter; >> + IdleCounterPriv *priv = SysCounterGetPrivate(counter); >> XSyncValue *less = priv->value_less, >> *greater = priv->value_greater; >> XSyncValue idle; >> @@ -2856,7 +2867,8 @@ static void >> IdleTimeBracketValues (pointer pCounter, CARD64 *pbracket_less, >> CARD64 *pbracket_greater) >> { >> - IdleCounterPriv *priv = SYSCOUNTERPRIV(pCounter); >> + SyncCounter *counter = pCounter; >> + IdleCounterPriv *priv = SysCounterGetPrivate(counter); >> XSyncValue *less = priv->value_less, >> *greater = priv->value_greater; >> Bool registered = (less || greater); >> @@ -2897,7 +2909,7 @@ init_system_idle_counter(const char *name, int >> deviceid) >> priv->deviceid = deviceid; >> priv->value_less = priv->value_greater = NULL; >> >> - SYSCOUNTERPRIV(idle_time_counter) = priv; >> + idle_time_counter->pSysCounterInfo->private = priv; >> >> return idle_time_counter; >> } >> diff --git a/Xext/syncsrv.h b/Xext/syncsrv.h >> index 361b68d..6c0bf74 100644 >> --- a/Xext/syncsrv.h >> +++ b/Xext/syncsrv.h >> @@ -71,8 +71,6 @@ typedef void (*SyncSystemCounterBracketValues)(pointer >> counter, >> CARD64 *pbracket_less, >> CARD64 *pbracket_greater); >> >> -#define SYSCOUNTERPRIV(counter) >> (((SyncCounter*)(counter))->pSysCounterInfo->private) >> - >> typedef struct _SysCounterInfo { >> char *name; >> CARD64 resolution; >> -- >> 1.7.7.6 >> >> >> >> >> >>> >>> The series looks fine generally; these are just minor details. >>> >>> Jamey >>> >>> On 3/13/12, Peter Hutterer <[email protected]> wrote: >>>> >>>> We currently have one global idlecounter. This patch series adds additional >>>> counters named "DEVICEIDLETIME n" (where n is the device id) that apply to >>>> that device only. >>>> >>>> One use-case here is as syndaemon replacement. A client can simply put an >>>> idlecounter on the keyboard above the touchpad. When that keyboard goes out >>>> of idle, disable the touchpad, then re-enable when the keyboard goes idle >>>> again. >>>> >>>> Cheers, >>>> Peter >>>> _______________________________________________ >>>> [email protected]: X.Org development >>>> Archives: http://lists.x.org/archives/xorg-devel >>>> Info: http://lists.x.org/mailman/listinfo/xorg-devel >>>> > <0001-sync-Use-a-linked-list-instead-of-an-array-for-SysCo.patch>_______________________________________________ > [email protected]: X.Org development > Archives: http://lists.x.org/archives/xorg-devel > Info: http://lists.x.org/mailman/listinfo/xorg-devel _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
