On Wed, Mar 14, 2012 at 4:30 PM, Peter Hutterer
<peter.hutte...@who-t.net> 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 <peter.hutte...@who-t.net>
> 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 <peter.hutte...@who-t.net>
> ---
> 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 <peter.hutte...@who-t.net> 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
>> > _______________________________________________
>> > xorg-devel@lists.x.org: X.Org development
>> > Archives: http://lists.x.org/archives/xorg-devel
>> > Info: http://lists.x.org/mailman/listinfo/xorg-devel
>> >
From 23b099dee39fdbf77d991b88a6bf6c20204766cb Mon Sep 17 00:00:00 2001
From: Jamey Sharp <ja...@minilop.net>
Date: Wed, 14 Mar 2012 17:22:18 -0700
Subject: [PATCH] sync: Use a linked list instead of an array for
SysCounterList.
Signed-off-by: Jamey Sharp <ja...@minilop.net>
---
Xext/sync.c | 56 ++++++++++++++------------------------------------------
Xext/syncsrv.h | 4 ++++
2 files changed, 18 insertions(+), 42 deletions(-)
diff --git a/Xext/sync.c b/Xext/sync.c
index 23360f0..a73521d 100644
--- a/Xext/sync.c
+++ b/Xext/sync.c
@@ -88,8 +88,7 @@ static RESTYPE RTAwait;
static RESTYPE RTAlarm;
static RESTYPE RTAlarmClient;
static RESTYPE RTFence;
-static int SyncNumSystemCounters = 0;
-static SyncCounter **SysCounterList = NULL;
+static struct xorg_list SysCounterList;
static int SyncNumInvalidCounterWarnings = 0;
#define MAX_INVALID_COUNTER_WARNINGS 5
@@ -984,11 +983,6 @@ SyncCreateSystemCounter(
{
SyncCounter *pCounter;
- SysCounterList = realloc(SysCounterList,
- (SyncNumSystemCounters+1)*sizeof(SyncCounter *));
- if (!SysCounterList)
- return NULL;
-
/* this function may be called before SYNC has been initialized, so we
* have to make sure RTCounter is created.
*/
@@ -1014,6 +1008,7 @@ SyncCreateSystemCounter(
return pCounter;
}
pCounter->pSysCounterInfo = psci;
+ psci->pCounter = pCounter;
psci->name = name;
psci->resolution = resolution;
psci->counterType = counterType;
@@ -1021,7 +1016,7 @@ SyncCreateSystemCounter(
psci->BracketValues = BracketValues;
XSyncMaxValue(&psci->bracket_greater);
XSyncMinValue(&psci->bracket_less);
- SysCounterList[SyncNumSystemCounters++] = pCounter;
+ xorg_list_add(&psci->entry, &SysCounterList);
}
return pCounter;
}
@@ -1176,31 +1171,8 @@ FreeCounter(void *env, XID id)
}
if (IsSystemCounter(pCounter))
{
- int i, found = 0;
-
+ xorg_list_del(&pCounter->pSysCounterInfo->entry);
free(pCounter->pSysCounterInfo);
-
- /* find the counter in the list of system counters and remove it */
-
- if (SysCounterList)
- {
- for (i = 0; i < SyncNumSystemCounters; i++)
- {
- if (SysCounterList[i] == pCounter)
- {
- found = i;
- break;
- }
- }
- if (found < (SyncNumSystemCounters-1))
- {
- for (i = found; i < SyncNumSystemCounters-1; i++)
- {
- SysCounterList[i] = SysCounterList[i+1];
- }
- }
- }
- SyncNumSystemCounters--;
}
free(pCounter);
return Success;
@@ -1297,20 +1269,21 @@ static int
ProcSyncListSystemCounters(ClientPtr client)
{
xSyncListSystemCountersReply rep;
- int i, len;
+ SysCounterInfo *psci;
+ int len = 0;
xSyncSystemCounter *list = NULL, *walklist = NULL;
REQUEST_SIZE_MATCH(xSyncListSystemCountersReq);
rep.type = X_Reply;
rep.sequenceNumber = client->sequence;
- rep.nCounters = SyncNumSystemCounters;
+ rep.nCounters = 0;
- for (i = len = 0; i < SyncNumSystemCounters; i++)
+ xorg_list_for_each_entry(psci, &SysCounterList, entry)
{
- const char *name = SysCounterList[i]->pSysCounterInfo->name;
/* pad to 4 byte boundary */
- len += pad_to_int32(sz_xSyncSystemCounter + strlen(name));
+ len += pad_to_int32(sz_xSyncSystemCounter + strlen(psci->name));
+ ++rep.nCounters;
}
if (len)
@@ -1329,13 +1302,12 @@ ProcSyncListSystemCounters(ClientPtr client)
swapl(&rep.nCounters);
}
- for (i = 0; i < SyncNumSystemCounters; i++)
+ xorg_list_for_each_entry(psci, &SysCounterList, entry)
{
int namelen;
char *pname_in_reply;
- SysCounterInfo *psci = SysCounterList[i]->pSysCounterInfo;
- walklist->counter = SysCounterList[i]->sync.id;
+ walklist->counter = psci->pCounter->sync.id;
walklist->resolution_hi = XSyncValueHigh32(psci->resolution);
walklist->resolution_lo = XSyncValueLow32(psci->resolution);
namelen = strlen(psci->name);
@@ -2553,8 +2525,6 @@ SAlarmNotifyEvent(xSyncAlarmNotifyEvent *from, xSyncAlarmNotifyEvent *to)
static void
SyncResetProc(ExtensionEntry *extEntry)
{
- free(SysCounterList);
- SysCounterList = NULL;
RTCounter = 0;
}
@@ -2567,6 +2537,8 @@ SyncExtensionInit(void)
ExtensionEntry *extEntry;
int s;
+ xorg_list_init(&SysCounterList);
+
for (s = 0; s < screenInfo.numScreens; s++)
miSyncSetup(screenInfo.screens[s]);
diff --git a/Xext/syncsrv.h b/Xext/syncsrv.h
index 2b70773..b439494 100644
--- a/Xext/syncsrv.h
+++ b/Xext/syncsrv.h
@@ -51,6 +51,7 @@ PERFORMANCE OF THIS SOFTWARE.
#ifndef _SYNCSRV_H_
#define _SYNCSRV_H_
+#include "list.h"
#include "misync.h"
#include "misyncstr.h"
@@ -66,6 +67,7 @@ typedef enum {
} SyncCounterType;
typedef struct _SysCounterInfo {
+ SyncCounter *pCounter;
const char *name;
CARD64 resolution;
CARD64 bracket_greater;
@@ -80,6 +82,8 @@ typedef struct _SysCounterInfo {
CARD64 * /*lessthan*/,
CARD64 * /*greaterthan*/
);
+
+ struct xorg_list entry;
} SysCounterInfo;
--
1.7.5.4
_______________________________________________
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
