pcp
[Top] [All Lists]

Re: [pcp] datetime enhancements

To: Stan Cox <scox@xxxxxxxxxx>
Subject: Re: [pcp] datetime enhancements
From: Nathan Scott <nathans@xxxxxxxxxx>
Date: Mon, 24 Feb 2014 01:51:43 -0500 (EST)
Cc: PCP <pcp@xxxxxxxxxxx>
Delivered-to: pcp@xxxxxxxxxxx
In-reply-to: <53042BF5.5070704@xxxxxxxxxx>
References: <53042BF5.5070704@xxxxxxxxxx>
Reply-to: Nathan Scott <nathans@xxxxxxxxxx>
Thread-index: d8g0Cw74PDn2oOK8KPOZ+3ut5w+Sug==
Thread-topic: datetime enhancements

----- Original Message -----
> commit 7ef03c90217b adds support for the gnulib get_date module to
> pcplib/rtime.c::__pmParseTime.  First it tries the existing mechanism.
> If that fails it tries gnulib get_date.   Here is an example from the
> testcase which shows example datetimes and how the start/end times are
> adjusted.   No doc changes yet but the date command uses the gnulib
> get_date support and documents it in
>   info date 'date input'
> 

Apologies for the delay in review Stan.  BTW, that fpaste on the man
page change hasn't been committed yet - your approach of referencing
the "info" page looked OK-ish to me.  Maybe not ideal though ... it'd
be better to have it in man format like everything else - I read thru
the info pages, and the content did not seem particularly lengthy...?
If you do add in the new __pmParseTime (impl.h/exports), as discussed
below - and I think you probably should now - a new pmparsetime.3 man
page would be a welcome addition (see pmparsectime.3 for a template).

Anyway, here's the rest of the review...


diff --git a/qa/751 b/qa/751
new file mode 100755
index 0000000..a239f71
--- /dev/null
+++ b/qa/751
@@ -0,0 +1,19 @@
+#!/bin/sh
+# PCP QA Test No. 751
+# Test supported datetime strings

test $PCP_VER -ge 3901 || _notrun "No support for new extended datetime strings"

diff --git a/qa/group b/qa/group
index 06a1f08..eca61fe 100644
--- a/qa/group
+++ b/qa/group
@@ -889,6 +889,7 @@ avahi
 748 pmlogrewrite pmda.mysql local oss
 749 pmcd local oss
 750 pmda.rpm local oss
+751 libpcp local oss

(there is a "git merge" conflict here with other recent test additions
- just FYI, in case you do a git pull to update to current pcp dev code)

 755 pmda.apache pmda.install local oss
 768 pmlogextract local oss
 775 sanity pmfind local oss
diff --git a/qa/src/GNUlocaldefs b/qa/src/GNUlocaldefs
index e3a5793..2933f9a 100644
--- a/qa/src/GNUlocaldefs
+++ b/qa/src/GNUlocaldefs
@@ -74,7 +74,7 @@ CFILES = disk_test.c exercise.c context_test.c chkoptfetch.c \
        pmdacache.c check_import.c unpack.c aggrstore.c atomstr.c \
        grind_conv.c getconfig.c err.c torture_logmeta.c keycache.c \
        keycache2.c pmdaqueue.c drain-server.c template.c anon-sa.c \
-       username.c
+       username.c rtimetest.c

Awesome, testing!  Thanks Stan!
 
 ifeq ($(shell test -f ../localconfig && echo 1), 1)
 include ../localconfig
diff --git a/qa/src/rtimetest.c b/qa/src/rtimetest.c
new file mode 100644
index 0000000..3b3222f
--- /dev/null
+++ b/qa/src/rtimetest.c
@@ -0,0 +1,123 @@

Missing copyright annotation.

+#include <time.h>
+#include <sys/time.h>
+#include <time.h>

Guess we *really* want the time headers!  :)  To be sure to be sure.

+#include <stdio.h>
+#include <limits.h>
+#include <string.h>

It will make life easier to remove all those headers and just include
pmapi.h (and probably impl.h too, depending on other questions below).
pmapi.h indirectly gets config.h which has platform-specific details.

+int __pmParseTime(
+    const char     *string,    /* string to be parsed */
+    struct timeval  *logStart, /* start of log or current time */
+    struct timeval  *logEnd,   /* end of log or tv_sec == INT_MAX */
+                               /* assumes sizeof(t_time) == sizeof(int) */
+    struct timeval  *rslt,     /* if parsed ok, result filled in */
+    char           **errMsg);  /* error message, please free */

Er, wha?  That's not going to fly; this symbol would need to be added
to the exports file in libpcp.  Do we want this directly callable via
client tools?  (maybe?  dunno)  If so, updates needed to exports and
to impl.h -- if not, this test needs to call the API that calls this
internal API.

We do need to update exports file for other things planned for this
next release, so its probably for the best to simply add it there I
guess, then testing is simpler too.

That does make me realise however, that there is no test coverage of
the non-double-underscore API interfaces that will usually be used
to access this functionality.  A second test program (or perhaps a
command line option for this one) could exercise that.  That should
check that examples of the existing syntax continue to be parsed in
addition to the extended syntax.  I suppose the "existing parsing is
still working" is already tested, but we need to make sure the new
parsing is indeed working from the non-double-underscore API - that
latter part is all we really need still.

+int
+main ()
+{

IIRC, this main() definition may cause compiler warnings on some
platforms.  Adding some getopts handling for the above issue will
of course resolve this.

+  set_tm (NULL, &tmtmp, &tmstart, 0, 19, 11, 45);
+  tmtmp_str = asctime(&tmtmp);
+  char *tmtmp_c = strchr (tmtmp_str, '\n');
+  *tmtmp_c = ' ';
+  __pmParseTime (tmtmp_str, &tvstart, &tvend, &tvrslt, &errmsg);
+  localtime_r (&tvrslt.tv_sec, &tmrslt); // time_t => tm

Hmm, is this not timezone sensitive?  (will this test pass no matter
where it is run?)

+  int sfx;
+  for (sfx = 0; sfx < (sizeof(strftime_fmt) / sizeof(void*)); sfx++) {
+    int len = strftime (buffer, sizeof (buffer), strftime_fmt[sfx], &tmtmp);
+    if (len != 0)

Inconsistency with braces?  (on the "for" vs "if" lines - pick one and
preferably the same as rest of pcp).  Actually, the entire source file
code style differs to all other qa/src programs.

diff --git a/src/libpcp/src/GNUmakefile b/src/libpcp/src/GNUmakefile
index 176079b..4a19e4b 100644
--- a/src/libpcp/src/GNUmakefile
+++ b/src/libpcp/src/GNUmakefile
@@ -58,6 +58,11 @@ LSRCFILES += accounts.c
 LLDLIBS        += -lpsapi
 endif
 
+CFILES += gettime.c xmalloc.c getdate.c

Add directly into the original CFILES list.  We've also added new headers
here, so HFILES needs updating for the new headers.  YFILES needs to be
extended to include getdate.y - see pmie makefile for example.

+.y.c:
+       $(YACC) $<
+       /bin/mv y.tab.c `basename $< .y`.c

See pmie makefile again - e.g. no hard-coded paths to binaries.

diff --git a/src/libpcp/src/check-statics b/src/libpcp/src/check-statics
index bc30905..050c9bd 100755
--- a/src/libpcp/src/check-statics
+++ b/src/libpcp/src/check-statics
@@ -181,6 +181,26 @@ fetchlocal.o
     b splitmax.[0-9]*          # single-threaded PM_SCOPE_DSO_PMDA
 fetch.o
 freeresult.o
+getdate.o
+    d dst_table
+    d meridian_table
+    d military_table
+    d month_and_day_table
+    b RELATIVE_TIME_0
+    d relative_time_table
+    d time_units_table
+    d time_zone_table
+    d universal_time_zone_table
+    r yycheck
+    r yydefact
+    r yydefgoto
+    r yypact
+    r yypgoto
+    r yyr1
+    r yyr2
+    r yytable
+    r yytranslate
+gettime.o
 hash.o

Comments need to be added to each of these explaining why/how they are safe
in a multi-threaded environment.  Everything ending in _table looks like it
is a "const static" (readonly & hence safe).  However all the yacc symbols
are highly suspect and need comments and probably code audit to add missing
locking.

I also found the build failed for me in the check-statics phase - I had some
unexpected symbols - yyval_default.[0-9]* in getdate.o and also some strange
C.[0-9][0-9].[0-9]* symbol that I could not trace.  Do you know what that
might be?  A regex as wide-open as C.* in check-statics would not be ideal,
so I'd really least like to find out what this symbol is.

diff --git a/src/libpcp/src/getdate.c b/src/libpcp/src/getdate.c
new file mode 100644
index 0000000..448657e
--- /dev/null
+++ b/src/libpcp/src/getdate.c

This file is generated and should not be committed to the git repo.  Remove
from CFILES in Makefile too - check out pmie makefile for an example.

+static char *
+get_tz (char tzbuf[TZBUFSIZE])
+  char *tz = getenv ("TZ");

Ah - this doesn't take into account the current timezone, in the stack
that libpcp provides.  See pmNewZone and pmUseZone.

+  tmp = localtime (&now->tv_sec);

pmLocaltime?

+  if (strncmp (p, "TZ=\"", 4) == 0)

Hmm, suspiscious for above reasons.

+#if HAVE_STRUCT_TM_TM_ZONE

We have no configure support for this.

+#if HAVE_TZNAME

We have no configure support for this.

+# ifndef tzname
+    extern char *tzname[];
+# endif

We unconditionally access this over in tz.c so the ifdef appears
unnecessary for all platforms PCP has ever supported.

+    for (i = 0; i < 2; i++)
+      {
+       pc.local_time_zone_table[i].name = tzname[i];

Oh we keep pointers to it - can tzname ever change?  I would punt that it
can (see tz.c, which calls tzset - hmm, why is there no tzset in this new
code??)

+             if (setenv ("TZ", tz1buf, 1) != 0)
+               goto fail;

This is unlikely to play well with the existing timezone stacking in tz.c.

+#ifdef HAVE_TM_GMTOFF

We have no configure support for this.

+ done:
+  if (tz_was_altered)
+    ok &= (tz0 ? setenv ("TZ", tz0, 1) : unsetenv ("TZ")) == 0;

Hmmm.  Interesting, maybe this can be made to play nice after all.
We do still need to take into account the existing zone setting -
a QA test exercising use of pmNewZone & pmUseZone in conjunction
with this new (internal) API seems warranted.

diff --git a/src/libpcp/src/getdate.h b/src/libpcp/src/getdate.h
new file mode 100644
index 0000000..142231e
--- /dev/null
+++ b/src/libpcp/src/getdate.h
@@ -0,0 +1,23 @@
+/* Parse a string into an internal time stamp.
+
+   Copyright (C) 1995, 1997, 1998, 2003, 2004, 2007 Free Software
+   Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 2, or (at your option)
+   any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program; if not, write to the Free Software Foundation,
+   Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.  */
+
+#include <stdbool.h>
+#include <time.h>
+
+bool get_date (struct timespec *, char const *, struct timespec const *);

get_date function needs _PCP_HIDDEN annotation.  This header should
probably be rolled into internal.h.  The use of bool as an error
handling mechanism is different to the way the rest of libpcp works,
so I'd advise not keeping that aspect either.

--- /dev/null
+++ b/src/libpcp/src/getdate.y
@@ -0,0 +1,1520 @@
[...]
+   nanosecond-resolution time stamps, and in October 2004 to support
+   TZ strings in dates.  */

We should verify if this latter feature functions with (or breaks)
timezone stacking in libpcp.

+/* FIXME: Check for arithmetic overflow in all cases, not just
+   some of them.  */

Eek.

+// #include <config.h>

Mhmm.  We need to sort out configuration by auditing the code, and
adding to our configure checks, not just commenting the header out.

+
+/* Since the code of getdate.y is not included in the Emacs executable
+   itself, there is no need to #define static in this file.  Even if
+   the code were included in the Emacs executable, it probably
+   wouldn't do any harm to #undef it here; this will only cause
+   problems if we try to write to a static variable, which I don't
+   think this code needs to do.  */
+#ifdef emacs
+# undef static
+#endif

The above can certainly go.

This entire file has a different coding style to the rest of libpcp,
not surprisingly.  We need to decide whether to tradeoff consistency
with the ability to make direct comparisons to new versions of the
source code -- how often is this code updated?  (upstream, I mean)

I suspect we should embrace the existing code and make it our own -
the header comments suggest few updates.

+#if HAVE_COMPOUND_LITERALS

We have no configure support for this.

+/* We want a reentrant parser, even if the TZ manipulation and the calls to

(Ah, good to hear!)

+   localtime and gmtime are not reentrant.  */

Rest of libpcp uses the pcp lock to provide these guarantees - this
code will need to be audited to check interactions there, and whether
it is circumventing the existing locking.  At first glance, it appears
to be broken in this regard.

+             }
+           else
+             {

This coding style is really jarring when reading it, to me at least -
should be made consistent with the rest of the PCP code base.  The use
of whitespace inconsistently also is problematic (different whitespace
in conditionals, function calls, etc).

+static table const *
+lookup_zone (parser_control const *pc, char const *name)
+{
+  table const *tp;
+
+  for (tp = universal_time_zone_table; tp->name; tp++)
+    if (strcmp (name, tp->name) == 0)
+      return tp;
+
+  /* Try local zone abbreviations before those in time_zone_table, as
+     the local ones are more likely to be right.  */
+  for (tp = pc->local_time_zone_table; tp->name; tp++)
+    if (strcmp (name, tp->name) == 0)
+      return tp;
+
+  for (tp = time_zone_table; tp->name; tp++)
+    if (strcmp (name, tp->name) == 0)
+      return tp;
+
+  return NULL;
+}

The above timezone table continues to make me nervous.  :)  Surely
this needs to interact with the timezone code in tz.c in some way?

+/* A reasonable upper bound for the size of ordinary TZ strings.
+   Use heap allocation if TZ's length exceeds this.  */
+enum { TZBUFSIZE = 100 };
+
+/* Return a copy of TZ, stored in TZBUF if it fits, and heap-allocated
+   otherwise.  */
+static char *
+get_tz (char tzbuf[TZBUFSIZE])
+{
+  char *tz = getenv ("TZ");
+  if (tz)
+    {
+      size_t tzsize = strlen (tz) + 1;
+      tz = (tzsize <= TZBUFSIZE
+           ? memcpy (tzbuf, tz, tzsize)
+           : xmemdup (tz, tzsize));
+    }
+  return tz;
+}

Same comments as above.

+#else
+#if HAVE_TZNAME
+  {
+# ifndef tzname
+    extern char *tzname[];
+# endif

Same comments as earlier.

+#if TEST
+
+int
+main (int ac, char **av)
+{
+  char buff[BUFSIZ];
+
+  printf ("Enter date, or blank line to exit.\n\t> ");
+  fflush (stdout);
+
+  buff[BUFSIZ - 1] = '\0';
+  while (fgets (buff, BUFSIZ - 1, stdin) && buff[0])
+    {
+      struct timespec d;
+      struct tm const *tm;
+      if (! get_date (&d, buff, NULL))
+       printf ("Bad format - couldn't convert.\n");
+      else if (! (tm = localtime (&d.tv_sec)))
+       {
+         long int sec = d.tv_sec;
+         printf ("localtime (%ld) failed\n", sec);
+       }
+      else
+       {
+         int ns = d.tv_nsec;
+         printf ("%04ld-%02d-%02d %02d:%02d:%02d.%09d\n",
+                 tm->tm_year + 1900L, tm->tm_mon + 1, tm->tm_mday,
+                 tm->tm_hour, tm->tm_min, tm->tm_sec, ns);
+       }
+      printf ("\t> ");
+      fflush (stdout);
+    }
+  return 0;
+}
+#endif /* TEST */

Is there actual test data that the authors of this code used with
this test program?  It would be good to pull that into our tests
as well, in the same way we have pulled in the code.

diff --git a/src/libpcp/src/gettime.c b/src/libpcp/src/gettime.c
new file mode 100644
index 0000000..70bb5de
--- /dev/null
+++ b/src/libpcp/src/gettime.c
@@ -0,0 +1,49 @@
[...]
+// #include <config.h>

(same issue discussed earlier)

+/* Get the system time into *TS.  */
+
+void
+gettime (struct timespec *ts)
+{
+#if HAVE_NANOTIME
+  nanotime (ts);
+#else

We have no configure check for this.  nanotime doesn't appear to
exist in Linux headers?

+# if defined CLOCK_REALTIME

Nor this.

+[...] && HAVE_CLOCK_GETTIME

Nor this.

+    struct timeval tv;
+    gettimeofday (&tv, NULL);
+    ts->tv_sec = tv.tv_sec;
+    ts->tv_nsec = tv.tv_usec * 1000;

The accuracy of gettimeofday tends to be accepted as plenty for the
sorts of problems PCP is used to tackle.

diff --git a/src/libpcp/src/rtime.c b/src/libpcp/src/rtime.c
index 086981b..26f8ccb 100644
--- a/src/libpcp/src/rtime.c
+++ b/src/libpcp/src/rtime.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1995 Silicon Graphics, Inc.  All Rights Reserved.
+ * Copyright (c) 1995, 2014 Silicon Graphics, Inc.  All Rights Reserved.

*cough* - heh.

  * This library is free software; you can redistribute it and/or modify it
  * under the terms of the GNU Lesser General Public License as published
@@ -14,6 +14,7 @@
 
 #include <limits.h>
 #include <ctype.h>
+#include <string.h>
 #include "pmapi.h"
 #include "impl.h"

[ Not needed? ... ]
$ grep string.h /usr/include/pcp/*h
/usr/include/pcp/pmapi.h:#include <string.h>
 
@@ -519,6 +520,74 @@ __pmConvertTime(
+
+/*
+ * Used by xmalloc/xrealloc/xcalloc which are called by get_date, the datetime 
parser
+ */
+
+void
+xalloc_die (void)
+{
+    //  error (exit_failure, 0, "%s", _("memory exhausted"));
+    abort ();
+}
+

Not really appropriate error handling for a library.  A __pmNoMem call would
be more appropriate than xalloc_die too - but will need more context - I'd
imagine date string parsing should never cause program exit - a graphical
tool like pmchart might be calling it with an interactively-user-supplied
string, for example.

 int    /* 0 -> ok, -1 -> error */
 __pmParseTime(
     const char     *string,    /* string to be parsed */
@@ -533,7 +602,7 @@ __pmParseTime(
     struct timeval  start;
     struct timeval  end;
     struct timeval  tval;
-
+    int get_date (struct timespec *, char const *, struct timespec const *);

No inline extern function definitions.  This is already in a header, use
that instead (should be internal.h by the time we're done).

@@ -542,33 +611,62 @@ __pmParseTime(
 
     /* ctime string */
     if (parseChar(&scan, '@')) {
-       if (__pmParseCtime(scan, &tm, errMsg) < 0)
-           return -1;
-       tm.tm_wday = NO_OFFSET;
-       __pmConvertTime(&tm, &start, rslt);
+       if (__pmParseCtime(scan, &tm, errMsg) >= 0) {
+           tm.tm_wday = NO_OFFSET;
+           __pmConvertTime(&tm, &start, rslt);
+           return 0;
+       }
     }
 
(is there a memory leak on errMsg here)


     /* relative to end of archive */
     else if (end.tv_sec < INT_MAX && parseChar(&scan, '-')) {
-       if (pmParseInterval(scan, &tval, errMsg) < 0)
-           return -1;
-       tm.tm_wday = NEG_OFFSET;
-       tm.tm_sec = (int)tval.tv_sec;
-       tm.tm_yday = (int)tval.tv_usec;
-       __pmConvertTime(&tm, &end, rslt);
+       if (pmParseInterval(scan, &tval, errMsg) >= 0) {
+           tm.tm_wday = NEG_OFFSET;
+           tm.tm_sec = (int)tval.tv_sec;
+           tm.tm_yday = (int)tval.tv_usec;
+           __pmConvertTime(&tm, &end, rslt);
+           return 0;
+       }
     }

 
(is there another memory leak on errMsg here)


     /* relative to start of archive or current time */
     else {
        parseChar(&scan, '+');
-       if (pmParseInterval(scan, &tval, errMsg) < 0)
-           return -1;
-       tm.tm_wday = PLUS_OFFSET;
-       tm.tm_sec = (int)tval.tv_sec;
-       tm.tm_yday = (int)tval.tv_usec;
-       __pmConvertTime(&tm,  &start, rslt);
+       if (pmParseInterval(scan, &tval, errMsg) >= 0) {
+           tm.tm_wday = PLUS_OFFSET;
+           tm.tm_sec = (int)tval.tv_sec;
+           tm.tm_yday = (int)tval.tv_usec;
+           __pmConvertTime(&tm,  &start, rslt);
+           return 0;
+       }
     }


(is there another memory leak on errMsg here)

 
+    /* datetime is not a pcp defined one, so drop down into the glib get_date 
case */
+    int status  = -1;
+    int rel_type = have_relative_date (scan);
+    struct timespec tsrslt;
+    struct timespec *tsrsltp = &tsrslt;

IIRC, this (locals declared in the middle of a function) was reported as
not working under some compiler versions recently - perhaps take this to
a separate function?

+    if (parseChar(&scan, '@'))
+       rel_type = NO_OFFSET;
+
+    if (rel_type == NO_OFFSET)
+       status = get_date (tsrsltp, scan, NULL);
+    else if (rel_type == NEG_OFFSET && end.tv_sec < INT_MAX) {
+       struct timespec tsend;
+       struct timespec *tsendp = &tsend;
+       TIMEVAL_TO_TIMESPEC (&end, tsendp);
+       status = get_date (tsrsltp, scan, &tsend);

We're mixing up different code styles now - PCP style and glib style
(4 space indents vs extra whitespace).

get_date returns a bool doesn't it?  This is the sort of bug I was half
wondering might happen.  Remove the use of bool would be best I think.

Oh, whooah - are we guaranteed that an errMsg string will be correctly
initialised for all failing cases?  Might be OK, on reflection - if the
strategy is just that we provide more passing cases (which AIUI is what
we're doing), and pass out the earlier error string.  Yeah, that's OK.
I think.  :)

We definitely leak errMsg though - in all newly added passing cases,
we leak the earlier error messages.  An additional free right near the
end, before "return 0;" should do the trick.

diff --git a/src/libpcp/src/setenv.h b/src/libpcp/src/setenv.h
new file mode 100644
index 0000000..92e7bba
--- /dev/null
+++ b/src/libpcp/src/setenv.h
@@ -0,0 +1,54 @@
+#if HAVE_SETENV || HAVE_UNSETENV

Its luck of the draw as to whether these will be defined correctly,
if we don't pull in our config.h first here.  (success would depend
on the order of #includes at any/all places including this header)

That said, everywhere else that modifies the environment (see tz.c,
in particular) in libpcp uses putenv and provides locking.

diff --git a/src/libpcp/src/timespec.h b/src/libpcp/src/timespec.h
new file mode 100644
index 0000000..cce2d66
--- /dev/null
+++ b/src/libpcp/src/timespec.h
@@ -0,0 +1,37 @@
+/* timespec -- System time interface
+
+   Copyright (C) 2000, 2002, 2004, 2005, 2007 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 2, or (at your option)
+   any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program; if not, write to the Free Software Foundation,
+   Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.  */
+
+#if ! defined TIMESPEC_H
+# define TIMESPEC_H
+
+# include <time.h>
+
+/* Return negative, zero, positive if A < B, A == B, A > B, respectively.
+   Assume the nanosecond components are in range, or close to it.  */
+static inline int
+timespec_cmp (struct timespec a, struct timespec b)
+{
+  return (a.tv_sec < b.tv_sec ? -1
+         : a.tv_sec > b.tv_sec ? 1
+         : a.tv_nsec - b.tv_nsec);
+}

+void gettime (struct timespec *);

(Open code use of gettimeofday like everywhere else in libpcp,
remove gettime use - or, make everywhere else use it if it has
value to us?  I recommend the former option)

+int settime (struct timespec const *);

(unused - remove)

So, could just move that inline call into internal.h, and remove
the new header entirely.


diff --git a/src/libpcp/src/xalloc.h b/src/libpcp/src/xalloc.h
new file mode 100644
index 0000000..0c6d8dc
--- /dev/null
+++ b/src/libpcp/src/xalloc.h
@@ -0,0 +1,271 @@
+/* xalloc.h -- malloc with out-of-memory checking

This entire header needs to be removed.  Its not appropriate to
abort() on memory allocation error in a library, nor to introduce
special, different handling of memory allocation just for this
new code.

Fortunately, there's almost nothing using these interfaces, so
conversion to usual libpcp memory allocation failure handling
should be straightforward.  In fact, most of the routines and
defines in this new header are completely unused anyway.

diff --git a/src/libpcp/src/xmalloc.c b/src/libpcp/src/xmalloc.c
--- /dev/null
+++ b/src/libpcp/src/xmalloc.c
@@ -0,0 +1,123 @@

Likewise, remove this file entirely please.


cheers.

--
Nathan

<Prev in Thread] Current Thread [Next in Thread>