Ken,
On 5/6/15 12:38 AM, Ken McDonell wrote:
Thanks for this Martins.
I've reviewed the code, comments at the end of this mail.
The executive summary is it looks good overall, I have a minor
cosmetic patch to be applied, and then there are a couple of small
issues that are not necessarily part of the PM_ERR_BADSTORE change,
but rather problems in the code that pre-date these changes.
I propose pushing your commit and my fixup patch (if you agree) and
then I think the other issues can be resolved in subsequent commits
after some discussion.
src/perl/PMDA/PMDA.pm | 10 ++++++++--
src/perl/PMDA/cvalue.c | 4 ++++
src/python/pmapi.c | 2 ++
Suggest re-ordering to match pmerr -l output ... no functional
diff, just makes spotting these sort of omissions easier in
the future [I have a patch for this and will commit that,
if you agree]
Sounds good.
src/pmdas/linux_proc/contexts.c | 4 ++--
The proc_ctx_set_cgroups case is OK. For the
proc_ctx_set_threads case, I can't tell what the semantics
of proc.control.perclient.threads is (there is no help text
for any of the proc.control metrics which is another issue
outside the scope of this review) and in particular why
values <= 1 are acceptable and values > 1 produce an error
... so I'm not sure if PM_ERR_BADSTORE is correct here,
although it is probably OK. The comment in proc_store()
for proc.control.all.threads which I assume is related,
suggests the value should be 0 or 1, but the code does not
tightly enforce this for either of the metrics.
Seems like there is some more auditing needed here beyond
the PM_ERR_BADSTORE changes.
From my understanding, this should be a boolean type value. For this
and a couple below, I didn't touch any functionality, just replaced
error codes. But I believe you are correct.
src/pmdas/linux_proc/pmda.c | 4 ++--
See comment above about check for boolean values .. something
like
if (av.ul != 0 && av.ul != 1)
sts = PM_ERR_BADSTORE;
would seem to be more accurate than
if (av.ul > 1)
sts = PM_ERR_BADSTORE;
I believe so.
The hotproc.control.config case is OK, although as a
general note for all PMDA writers (outside the scope of
the PM_ERR_BADSTORE changes), please note that it is good
etiquette and style to add the metric names alongside the
"item switch cases", e.g.
case ITEM_HOTPROC_G_CONFIG:
is MUCH harder to read and understand than
case ITEM_HOTPROC_G_CONFIG: /* hotproc.control.config */
OK, will look at these.
src/pmdas/papi/papi.c | 2 +-
I suspect all/many of the pmExtractValue() calls might return
PM_ERR_CONV and ditto for the second refresh_metrics() call
... perhaps you need a catch all before the final return in
papi_store() like ...
if (sts == PM_ERR_CONV)
sts = PM_ERR_BADSTORE;
I hadn't though about what pmExtractValue() might return. As you say
this is more of a policy decision.
This is probably more of a policy decision ... the
difference between PM_ERR_CONV and PM_ERR_BADSTORE is small
in these cases. If this change is to become the policy,
then we probably need to audit all the PMDA store methods
... e.g. a quick check shows mmv_store() and sample_store()
(there may be many others, I only looked a few PMDAs) also
contains pmExtractValue() calls where a PM_ERR_CONV error
maybe returned directly to the client.
Right. I guess it makes the code a little cluttered and is one step
that will likely be overlooked by new pmda writers, but may return a
more descriptive error to the client, and is probably useful to provide.
qa/006.out | 2 +-
qa/982.out | 2 +-
src/include/pcp/pmapi.h | 1 +
src/libpcp/src/err.c | 2 ++
src/pmdas/logger/event.c | 2 +-
src/pmdas/logger/logger.c | 2 +-
src/pmdas/mmv/src/mmv.c | 2 +-
src/pmdas/pmcd/src/pmcd.c | 6 +++---
src/pmdas/sample/src/sample.c | 12 ++++++------
src/pmdas/simple/pmdasimple.perl | 6 +++---
src/pmdas/simple/pmdasimple.python | 6 +++---
src/pmdas/simple/simple.c | 6 +++---
src/pmdas/trace/src/trace.c | 2 +-
No issues.
Thanks for the review!
Martins
|