On 06/05/15 05:24, Martins Innus wrote:
Hi,
Here is this new error code as discussed previously:
https://github.com/ubccr/pcp/tree/hotproc_cleanups
Meant to be used when pmstore gets input it doesn't expect. Updated the
appropriate pmdas and I think the correct qa tests. I think only 2 were
affected, but I have a bunch of "notruns" so this needs another look by
somebody else.
While doing this, I found a few existing error codes that hadn't made
their way to the python and perl side, so I added those as well.
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]
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.
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;
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 */
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;
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.
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.
|