I propose applying the attached patch early next week. But I would very
much like someone who knows the area both to sanity-check it and to
run-time check it, please.
Axiom: Part of the heartbeat ethos is that compiler warnings should be
regarded as potential errors. Indeed, the default Linux compile actually
treats warnings as errors. On Solaris, however, we haven't yet got to
that stage, but we are trying to get there. And a new set of warnings
appeared about five weeks ago...
Problem: In "lib/clplumbing/GSource.c", Solaris (probably more accurately
sparc-architecture) compilations get multiple warnings of the form:
[...]/GSource.c:239: warning: cast increases required alignment of target
type
caused by the use of " longclock_t detecttime" in structures. This is a
large type (typically "long long"), which, on some architectures, requires
the space to be similarly aligned. But the code cannot guarantee this
and this is detected at runtime.
Solution: Replace the declaration:
longclock_t detecttime;
by a char array (which doesn't apply alignment constraints):
char detecttime[sizeof(longclock_t)];
and replace references to such fields by two internal routines to store
and fetch the value (internally a "memcpy()" operation).
I think the patch is clean... no casts (always a potential source of
problems!) etc.
Comments? Screams of horror? Raptures of delight?
Incidentally, a casual eye reveals some code duplication: for instance
routines "G_SIG_prepare()" and "G_SIG_check()" look very similar.
Wouldn't it be better if these were merged as far as possible, perhaps
with one acting as a wrapper (+ a line or two of code) for the other?
--
: David Lee I.T. Service :
: Senior Systems Programmer Computer Centre :
: Durham University :
: http://www.dur.ac.uk/t.d.lee/ South Road :
: Durham DH1 3LE :
: Phone: +44 191 334 2752 U.K. :
--- lib/clplumbing/GSource.c.orig Fri Mar 3 10:35:48 2006
+++ lib/clplumbing/GSource.c Thu Mar 9 17:02:16 2006
@@ -64,12 +64,35 @@
unsigned magno; /* Magic number */ \
long maxdispatchms; /* Time limit for dispatch function */ \
long maxdispatchdelayms; /* Max delay before processing */ \
-longclock_t detecttime; /* Time last input detected */ \
+char detecttime[sizeof(longclock_t)]; \
+ /* Time last input detected */ \
void* udata; /* User-defined data */ \
guint gsourceid; /* Source id of this source */ \
const char * description; /* Description of this source */ \
GDestroyNotify dnotify
+/*
+ * On architectures with alignment constraints, our casting between
+ * "(GSource*)" and "(GFDSource_s*)" etc. causes trouble, because of
+ * the massive alignment requirements of "longclock_t".
+ *
+ * Use the following to store and fetch.
+ */
+static
+void
+lc_store(char *destptr, longclock_t value) {
+ longclock_t _ltt;
+ _ltt = value;
+ memcpy((destptr), &_ltt, sizeof(longclock_t));
+}
+
+static
+longclock_t
+lc_fetch(char *ptr) {
+ longclock_t _ltt;
+ memcpy(&_ltt, (ptr), sizeof(longclock_t));
+ return _ltt;
+}
struct GFDSource_s {
COMMON_STRUCTSTART;
@@ -140,12 +163,14 @@
#define CHECK_DISPATCH_DELAY(i) {
\
unsigned long ms; \
+ longclock_t dettime; \
dispstart = time_longclock(); \
- ms = longclockto_ms(sub_longclock(dispstart,(i)->detecttime)); \
+ dettime = lc_fetch((i)->detecttime); \
+ ms = longclockto_ms(sub_longclock(dispstart,dettime)); \
if ((i)->maxdispatchdelayms > 0 \
&& ms > (i)->maxdispatchdelayms) { \
WARN_DELAY(ms, (i)); \
- EXPLAINDELAY(dispstart, (i)->detecttime); \
+ EXPLAINDELAY(dispstart, dettime); \
} \
}
@@ -156,7 +181,7 @@
if ((i)->maxdispatchms > 0 && ms > (i)->maxdispatchms) { \
WARN_TOOLONG(ms, (i)); \
} \
- (i)->detecttime = zero_longclock; \
+ lc_store(((i)->detecttime), zero_longclock); \
}
#define WARN_TOOMUCH(ms, input) cl_log(LOG_WARNING
\
@@ -236,7 +261,7 @@
ret->gpfd.events = DEF_EVENTS;
ret->gpfd.revents = 0;
ret->dnotify = notify;
- ret->detecttime = zero_longclock;
+ lc_store((ret->detecttime), zero_longclock);
g_source_add_poll(source, &ret->gpfd);
@@ -310,7 +335,7 @@
GFDSource* fdp = (GFDSource*)source;
g_assert(IS_FDSOURCE(fdp));
if (fdp->gpfd.revents) {
- fdp->detecttime = time_longclock();
+ lc_store((fdp->detecttime), time_longclock());
return TRUE;
}
return FALSE;
@@ -421,7 +446,7 @@
chp->magno = MAG_GCHSOURCE;
chp->maxdispatchdelayms = DEFAULT_MAXDELAY;
chp->maxdispatchms = DEFAULT_MAXDISPATCH;
- chp->detecttime = zero_longclock;
+ lc_store((chp->detecttime), zero_longclock);
chp->ch = ch;
chp->dispatch = dispatch;
chp->udata=userdata;
@@ -550,7 +575,7 @@
}
ret = chp->ch->ops->is_message_pending(chp->ch);
if (ret) {
- chp->detecttime = time_longclock();
+ lc_store((chp->detecttime), time_longclock());
}
CHECKEND(chp);
return ret;
@@ -582,7 +607,7 @@
|| (!chp->fd_fdx && chp->outfd.revents != 0)
|| chp->ch->ops->is_message_pending(chp->ch));
if (ret) {
- chp->detecttime = time_longclock();
+ lc_store((chp->detecttime), time_longclock());
}
CHECKEND(chp);
return ret;
@@ -733,7 +758,7 @@
wcp->magno = MAG_GWCSOURCE;
wcp->maxdispatchdelayms = DEFAULT_MAXDELAY;
wcp->maxdispatchms = DEFAULT_MAXDISPATCH;
- wcp->detecttime = zero_longclock;
+ lc_store((wcp->detecttime), zero_longclock);
wcp->udata = userdata;
wcp->gpfd.fd = wch->ops->get_select_fd(wch);
wcp->gpfd.events = DEF_EVENTS;
@@ -809,7 +834,7 @@
g_assert(IS_WCSOURCE(wcp));
if (wcp->gpfd.revents != 0) {
- wcp->detecttime = time_longclock();
+ lc_store((wcp->detecttime), time_longclock());
return TRUE;
}
return FALSE;
@@ -1019,15 +1044,17 @@
clock_t diff;
/* detecttime is reset in the dispatch function */
- if (cmp_longclock(sig_src->detecttime, zero_longclock) != 0) {
+ if (cmp_longclock(lc_fetch(sig_src->detecttime),
zero_longclock) != 0) {
cl_log(LOG_ERR, "%s: detecttime already set?",
__FUNCTION__);
return TRUE;
}
/* Otherwise, this is when it was first detected */
now = times(&dummy_tms_struct);
diff = now - sig_src->sh_detecttime; /* How long since
signal occurred? */
- sig_src->detecttime
- = sub_longclock(time_longclock(), (longclock_t)diff);
+ lc_store(
+ sig_src->detecttime,
+ sub_longclock(time_longclock(), (longclock_t)diff)
+ );
return TRUE;
}
return FALSE;
@@ -1049,14 +1076,16 @@
static struct tms dummy_tms_struct;
clock_t now;
clock_t diff;
- if (cmp_longclock(sig_src->detecttime, zero_longclock) != 0){
+ if (cmp_longclock(lc_fetch(sig_src->detecttime),
zero_longclock) != 0){
return TRUE;
}
/* Otherwise, this is when it was first detected */
now = times(&dummy_tms_struct);
diff = now - sig_src->sh_detecttime;
- sig_src->detecttime
- = sub_longclock(time_longclock(), (longclock_t)diff);
+ lc_store(
+ sig_src->detecttime,
+ sub_longclock(time_longclock(), (longclock_t)diff)
+ );
return TRUE;
}
return FALSE;
@@ -1265,7 +1294,7 @@
trig_src->dispatch = dispatch;
trig_src->udata = userdata;
trig_src->dnotify = notify;
- trig_src->detecttime = zero_longclock;
+ lc_store((trig_src->detecttime), zero_longclock);
trig_src->manual_trigger = FALSE;
@@ -1303,7 +1332,7 @@
g_assert(IS_TRIGSOURCE(trig_src));
trig_src->manual_trigger = TRUE;
- trig_src->detecttime = time_longclock();
+ lc_store((trig_src->detecttime), time_longclock());
}
@@ -1335,8 +1364,8 @@
if (trig_src->manual_trigger
- && cmp_longclock(trig_src->detecttime, zero_longclock) == 0) {
- trig_src->detecttime = time_longclock();
+ && cmp_longclock(lc_fetch(trig_src->detecttime), zero_longclock)
== 0) {
+ lc_store((trig_src->detecttime), time_longclock());
}
return trig_src->manual_trigger;
}
@@ -1353,8 +1382,8 @@
g_assert(IS_TRIGSOURCE(trig_src));
if (trig_src->manual_trigger
- && cmp_longclock(trig_src->detecttime, zero_longclock) == 0) {
- trig_src->detecttime = time_longclock();
+ && cmp_longclock(lc_fetch(trig_src->detecttime), zero_longclock)
== 0) {
+ lc_store((trig_src->detecttime), time_longclock());
}
return trig_src->manual_trigger;
}
@@ -1383,7 +1412,7 @@
}
CHECK_DISPATCH_TIME(trig_src);
}
- trig_src->detecttime = zero_longclock;
+ lc_store((trig_src->detecttime), zero_longclock);
return TRUE;
}
@@ -1468,7 +1497,7 @@
append->maxdispatchms = DEFAULT_MAXDISPATCH;
append->maxdispatchdelayms = DEFAULT_MAXDELAY;
append->description = "(timeout)";
- append->detecttime = zero_longclock;
+ lc_store((append->detecttime), zero_longclock);
append->udata = NULL;
append->nexttime = add_longclock(time_longclock()
@@ -1549,7 +1578,7 @@
gboolean ret;
g_assert(IS_TIMEOUTSRC(append));
- append->detecttime = append->nexttime;
+ lc_store(append->detecttime, append->nexttime);
CHECK_DISPATCH_DELAY(append);
_______________________________________________________
Linux-HA-Dev: [email protected]
http://lists.linux-ha.org/mailman/listinfo/linux-ha-dev
Home Page: http://linux-ha.org/
