----- Original Message -----
> Hi -
>
> > Mark is away for the week, and next week is QA week for 3.11.0, so I'll
> > attempt to fill his shoes and get this merge over the line before then.
>
> Thank you for your review & patches.
>
As Frank found, the actual code wasn't in the branch I'd indicated, it
is here now though: git://git.pcp.io/nathans/pcp.git fetchgroup
>
> > [...] 1. The pmExtendFetchGroup_event API cannot support highres
> > timers as-is. Either we need a second API (for highres events), or
> > we need to have one API with struct timespec for the "out_times"
> > parameter (and do a lower-res timers usec -> nsec conversion
> > within). [...]
>
> How about for now only changing the pmfg_event api to return
> nsec-timespec records, but not yet actually accepting the
> PM_TYPE_EVENT_HIGHRES type? This would make the API future-proof for
> when/if a real pmda appears that actually generates HIGHRES events,
> but in the mean time we don't have to bloat the code with it.
Well, its not "future" (for proofing), its long since been supported,
so new code needs to work with it (not all PMDAs live in-tree).
Good news is, the harder work is done in supporting events - highres is
exactly the same as lowres, just with different embedded timestamps.
The sample PMDA supports sample.event.highres_records so QA can use that
alongside the sample.event.records its already using.
Let's just get it done and not waste time arguing about small stuff like
this, thanks.
> > 2. Sentinel values
> >
> > I'm not so much against sentinels in the way Mark seemed to be, as
> > it looks kinda handy, but the choice of zero as a sentinel integer
> > value strikes me as questionable. [...prefer -1...]
>
> OK, will look into that. [...]
Perhaps some convenience macros for testing for sentinels in client code
might help here too? Just a thought, not an overly important addition.
> > I recommend a "memset(&atom, -1, sizeof(pmAtomValue));" for integer
> > PM_TYPE_* variants, as that's very uncommon (i.e. all bits set). It
> > turns out that also gives a convenient NaN (see below, may help with
> > that other issue too).
>
> Ah, it did not occur to me to investigate that coincidence. A
> quiet-NaN is preferable (for use as a sentinel without triggering
> SIGFPE), but fill-with-0xff does not seem like a completely portable
> way of generating that.
The pmie approach is this FWIW, also quite acceptable as its self-evidently
portable to all platforms (been there for many, many years) ...
dstruct.c: double zero = 0.0;
dstruct.c: mynan = zero / zero;
(and pmie doesn't do any special floating point exception handling)
Either way is fine, and yep, document it if the memset trick is used.
> > - The save/restore of the context around every fetchgroup call seems
> > excessive (extra libpcp lock/unlock x2 for every call) and makes for
> > many "goto" statements in the code that would otherwise collapse to
> > much simpler error handling. This all seems a bit unnecessary - what
> > is the aim there? [...]
>
> The intent was to let a client treat the fetchgroups as non-intrusive
> to any other contexts that the client may be using. If we do not save
> the incoming context, we still have to pmUse* ours, and thus the
> application may lose theirs. If we do not restore, then the pmfg
> private context would leak, and subsequent PMAPI operations would
> affect it. Put those together, and a client could accidentally start
> manipulating a pmfg context instead of their own.
>
Right, but this is such an unlikely situation (that an application would
choose to use both pmFG contexts *and* non-pmFG PMAPI contexts *and* that
it is buggy in its management of its non-pmFG contexts), that it is not
worth such complications in every entry/exit from all fetchgroup APIs.
Sounding like this code can go - but lets come back to that later after
focusing on the more critical highres timer & sentinel aspects.
cheers.
--
Nathan
|