Made the suggested changes to commit 1ba3f1fcc98964b
Here is a summary of the changes:
This code was imported so most of these changes are to bring the code
into the pcp family.
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).
There seems to already be one
diff --git a/qa/group b/qa/group
index 06a1f08..eca61fe 100644
--- a/qa/group
+++ b/qa/group
@@ -889,6 +889,7 @@ avahi
+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)
Moved to 752
+#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.
Included pmapi.h and impl.
+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.
Added test for pmParseTimeWindow.
+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.
Added proper main declaration
+ 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?)
I added TZ=gmt to 752 invocation of test
+ 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.
Will 'indent -linux -nce -i4'
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.
Added CFILES and HFILES
+.y.c:
+ $(YACC) $<
+ /bin/mv y.tab.c `basename $< .y`.c
See pmie makefile again - e.g. no hard-coded paths to binaries.
Remove reference y.tab.c reference
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.
getdate.y has %pure-parser which bison says:
Alternatively, you can generate a pure, reentrant parser. The Bison
declaration `%define api.pure' says that you want the parser to be
reentrant.
The staticsall seem to be static const
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.
I will check this on a RHEL release
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.
Removed from repo
+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.
The TZ= support from the parser is gone in lieu of pcp -Z support
+ tmp = localtime (&now->tv_sec);
pmLocaltime?
pmLocaltime it is
+ 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.
The TZ= support from the parser is gone in lieu of pcp -Z support
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.
_PCP_HIDDEN added. get_date.h removed
--- /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.
The TZ= support from the parser is gone in lieu of pcp -Z support
+/* 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.
SCOX removing TZ_ removed the configury
+
+/* 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.
Gone pecan
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.
These changes are a first crack at this
+#if HAVE_COMPOUND_LITERALS
We have no configure support for this.
SCOX removing TZ_ removed the configury
+/* 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.
Switched to pmLocalTime and gmtime_r, which is in POSIX
+ }
+ 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).
I'll run it through indent as mentioned above
+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?
So my understanding is pmParseTimeWindow is time zone agnostic and
the time zone activities are handled in pmTimeStateSetup. Since the TZ=
handling is now stripped out of the extended datetime support, this
should be resolved
+/* 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.
gone
+#else
+#if HAVE_TZNAME
+ {
+# ifndef tzname
+ extern char *tzname[];
+# endif
Same comments as earlier.
gone
+#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.
There is a test, one program, but it calls get_date directly, maybe I
can glean some additional tests from it
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?
As mentioned below, just use tv_usec
+# if defined CLOCK_REALTIME
Nor this.
+[...] && HAVE_CLOCK_GETTIME
Nor this.
Removing TZ_ removed these
+ 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.
Just use tv_usec
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.
Thank you, emacs automatic Copyright mechanism
* 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>
string.h include is gone
@@ -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.
Changed to use malloc and friends
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).
Moved to internal.h
@@ -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)
errMsg is passed in from the caller
+ /* 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?
It seems logically connected so I put it in braces. Not opposed
to moving dcls to the top but my personal preference is dcls near uses
+ 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).
indent
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.
Changed to return int
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.
That should be the caller's responsibility; right? Hmm I passed in an
allocated buffer in the test. Fixed that and freed on error.
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.
This is gone
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 @@
+#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)
Used gettimeofday
+int settime (struct timespec const *);
(unused - remove)
So, could just move that inline call into internal.h, and remove
the new header entirely.
Gone
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.
6t5
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.
Done Changed to use malloc and friends
|