----- Original Message -----
> 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.
Thanks Stan - sounds like its progressed nicely ...
> 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
Ah, I misunderstood what was happening here; I thought __pmParseTime was new
with these changes, but thats not the case. So, all comments re exports and
impl.h were not correct, and that part was fine as-is - sorry 'bout that!
> +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.
Aha - it was probably the above that caused me to misfire - there was not
ever a need for this function prototype to be added to a .c file, as it
was already in impl.h (and exports - of course, in hindsight, else build
would've failed - silly of me to not realise).
> 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
(Ah, so this is a mystery - RHEL 6.3 here FWIW, I can check others too
if its not reproducible on current Fedora).
> + 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
>
POSIX sadly no good on Windows FWIW ... would recommend finding somewhere in
libpcp doing something similar (hopefully something exists, not sure off the
top of my head though), and do it that way. Nothing appears to use gmtime_r
currently, I'd punt it wont work on Win32. /me looks - maybe __pmSquashTZ()
is doing similar things...?
> ...
> So my understanding is pmParseTimeWindow is time zone agnostic and
> the time zone activities are handled in pmTimeStateSetup. Since the TZ=
The pmTime* APIs are for clients interacting with pmtime(1) and unrelated
to these changes. The timezone APIs to look into are pmNewContextZone,
pmNewZone, pmWhichZone and pmUseZone. Oh and pmLocaltime, pmCtime, etc -
replacing localtime & ctime with PCP-tz-option aware variants.
> handling is now stripped out of the extended datetime support, this
> should be resolved
*nod*, plus use of the PCP-tz-option aware APIs above.
> 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.
Yes and no, its tricky - if everything fails, then its the definitely the
callers responsibility. But, if the first call fails (+ mallocs errmsg),
and then the new code succeeds, we return success AND we malloc'd errmsg.
In this case, we need to free the initial errmsg, don't we?
cheers.
--
Nathan
|