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.
> [...] 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.
> 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. It will enlarge the client code that
currently prefers zero.
> 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. It may be good enough (we'd just have to
document it), and would solve the missing-[d]nan() libc problems you
say exist on some platforms.
> 3. pmclient_fg (binary and python variants) lacks QA, man page, and
> there are a few problems in the pmclient makefile now.
> [ I've fixed these, see separate mail ]
Thanks.
> - 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.
- FChE
|