add extra units to pmUnits#2598
Conversation
This first batch of changes ... - pmapi.h: change pmUnits to introduce extraUnit and extraScale fields and associated macros for temperature, voltage, current and power - pmda.h: rewrite the PMDA_PMUNITS() macro to make it endian-safe and avoid trashing the new fields (they will be explicitly 0 when this macro is used, like in all the legacy code) - pmda.h: introduce a new PMDA_EXTRAUNITS() macro to set all 8 fields of pmUnits in an endian-safe manner - libpcp/extraunits.c: new data-driven routines to handle extra units; currently working for pmUnitsStr*() and pmParseUnitsStr() - libpcp/internal.h: extract __pmUnits (no bit fields) from units.c so it can be used there and in extraunits.c - libpcp/units.c: minimal changes to support extra units, mostly calling routines in extraunits.c to do the work; changes currently done for pmUnitsStr*() and __pmParseUnitsStrPart() (the routine below pmParseUnitsStr()) - libpcp/units.c: add -Dpmapi diagnostics for pmUnitsStr_r() - libpcp/logmeta.c: extend test in __pmLogAddDesc() to check extraUnit and extraScale fields when checking for changes in pmUnits - sample PMDA: add metrics for all supported values of extraUnit and extraScale - qa fixups for all of the above
Adding extraunits.c (for pmUnit changes) over in libpcp needs manual GNUmakefile updates for these two derivative libraries.
Replace initialization of .pad field with initialization of .extraUnit
and .extraScale fields.
modified: src/perl/MMV/MMV.xs
modified: src/perl/PMDA/PMDA.xs
Note, this fixes a compilation issue, but we're not planning on
exposing the "extra units" features in the Perl interface layer
(pending a demonstrable need).
- rewrite the MMV_UNITS() macro to make it endian-safe and avoid trashing the new fields (they will be explicitly 0 when this macro is used, like in all the legacy code) - introduce a new MMV_EXTRAUNITS() macro to set all 8 fields of pmUnits in an endian-safe manner
- __pmCheckDesc() checks the integrity of a pmDesc, including the pmUnits part ... this will be used to replace the similar checks in pmlogcheck and I expect to use it in the derived metrics and pmie changes for extended units, but for the moment ... - pmDesc checks in pminfo with -v have been replaced by a call to __pmCheckDesc() and a new -e flag is the same as -v, just not reporting metrics that return no values (so the metadata issues are more obvious) - __pmCheckDesc() is defined in libpcp.h and exposed via exports.in but is not part of the PMAPI - shell completion and man page updates for pminfo -e - also an extra units fix for pmlogextract snuck into this commit.
- src/pmdas/statsd/src/pmda-callbacks.c: endian-safe initialization of pmUnits and fix bogus metadata setting in create_pcp_metric() - src/pmdas/statsd/src/pmdastatsd.c: endian-safe initialization of pmUnits - qa/statsd/src/cases/07.py: rework check to use pminfo -fed directly to demonstrate metadata problem and verify fix - qa/713: tweak filter for hex indom number (now) from pminfo output
- PMDA_PMUNITS() => PMDA_UNITS() for symmetry with MMV_UNITS() - PMDA_PMUNITS() preserved for backwards compatibility
No support in the Perl APIs at this stage, but this change is needed to ensure the extra fields in the pmUnits are correctly set to zero.
Need to check the additional fields when testing for a change in the metadata.
previous change to allow checking of markdown files introduced a typo that broke the script for all troff source man pages.
- update pmdesc.c to ...
+ only emit initializers for dimension and scale when dimension
is non-zero
+ emit extraUnit and extrScale initializers if required
- regenerate metrics.c (makefile updates to re-enable rule) which
has briefer pmUnits and some additional metrics (the PMNS has
moved on since Mark last generated metrics.c)
- new EXTRAUNITS option in METRIC clause that accepts units as an 8-tuple (not a 6-tuple as for the UNITS option) - change sample.temperature.centigrade => sample.temperature.celsius to match international agreement in 1948. - man page - assorted qa changes and extensions Please ente `r the commit message for your changes. Lines starting
- PM_UNITS_... renamed PM_UNIT_... so we're using the singular form for macros, pmUnits field names ... library routines tend to use ExtraUnits in the routine names because the "Units" routines already existed and they typically relate to operations on a pmUnits struct (where the name is pluralised) - fix the 4:4 to 5:3 bit field split for .extraUnit and .extraScale in the ! HAVE_BITFIELDS_LTOR alternate block (the sizes were round the wrong way) - upgrade the Str and Parse routines to accept extra units in the dividend (.extraUnit > 0) or the divisor (.extraUnit < 0) which is starting to make use of the extra bits and signedness of .extraUnit; also extend qa/670 to exercise the related new code - remade the qa/extra archive after fixing the change from a 4:4 to 5:3 bit field split for .extraUnit and .extraScale - pmConvScale() now understands extra units and this required a new table-driven __pmConvExtraScale() in src/libpcp/src/extraunits.c to hide the "extra" units semantics from pmConvScale() and avoid the nested switch mess that bidirectional N x M conversion would otherwise require - document pmiExtraUnits() - update the pmLookupDesc(3) man page to provide the definitive description of extra units ... this man page is cross-referenced from many other man pages dealing the pmUnits - additional man page changes for all of the above - add pmunits "group" to qa/group and update all entries for QA tests that dink with pmUnits - extend qa/src/grind_conv.c to accept both a 6-tuple (classical) and 8-tuple (classical + extra units) specification for both input and output pmUnits
Conflicts:
qa/group
pmie plays loose and fast with the metadata, so there is very little checking of pmUnits compatibility in expressions => not much needs to be changed here (in fact main change is removing some dead code).
- pmie propagation of pmUnits in expression trees was seriously broken, and had not been noticed because pmie does not check pmUnits for compatibility of operands in expressions (another good reason to prefer derived metrics over pmie expressions) ... the fixes here address the obvious issues and in particular safe handling of extra units and correct metadata for the default metrics exported from the summary PMDA (which runs pmie in "secret agent" mode) - revise default expressions for the summary PMDA metrics summary.netif.packets and summary.netif.busy to consistently consider only active network interfaces, and tweak the help text to better match the semantics of the expressions used - new qa/1665 and qa/1666 to exercise pmUnits handling for pmie and the summary PMDA - fix incorrect pmUnits fiddling for QA's broken PMDA - fix incorrect pmUnits fiddling in qa/src/pducheck.c - fix leftover fallout from pminfo -e and sample PMDA pmns changes with some QA remakes - qa/1426: rework to use modern network.tcp "drops" metrics rather than _notrun when the Irix metrics are not available - make _triage_wait_point() look for $here/$seq.wait by default, so it works when a QA test has cd'd away from $here ... update COOKBOOK.md to match - assorted extra unit QA remakes and fixups, some from earlier changes that had not been exposed to a full QA run with all the pmUnits changes to date
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds pmUnits.extraUnit/extraScale support and wires parsing, formatting, conversion, validation, APIs, tooling, PMDAs/sample, bindings, completions, docs, and QA; also introduces pminfo -e metadata-verification mode. ChangesExtra units and metadata verification
Sequence Diagram(s)sequenceDiagram
participant UserCLI as pminfo (-e)
participant Libpcp as libpcp::__pmCheckDesc
participant ExtraUnits as extraunits.c::__pmConvExtraScale
participant Rewrite as pmlogrewrite::check_metadata
UserCLI->>Libpcp: request verify metadata (__pmCheckDesc)
Libpcp->>ExtraUnits: validate/convert extraUnit/extraScale (if present)
Libpcp-->>UserCLI: return metadata status + errmsg (if any)
Rewrite->>Libpcp: pre-validate new metric descriptors via __pmCheckDesc()
Rewrite-->>UserCLI: emit warnings/errors during config processing
Possibly related issues
Poem
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
qa/1665 (1)
50-51:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the extra trailing blank line at EOF.
Line [51] indicates an extra blank line after
exit; keep exactly one trailing newline at file end.Suggested fix
-exit - +exitAs per coding guidelines "Ensure all text and source files end with a single trailing newline character".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@qa/1665` around lines 50 - 51, The file ends with an extra blank line after the final token "exit"; remove the extra trailing blank line so the file ends with exactly one newline character following "exit" (i.e., ensure there is a single trailing newline at EOF and no additional empty lines).qa/1667 (1)
94-95:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the extra trailing blank line at EOF.
Line [95] adds an extra trailing blank line after
exit; keep a single trailing newline only.Suggested fix
-exit - +exitAs per coding guidelines "Ensure all text and source files end with a single trailing newline character".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@qa/1667` around lines 94 - 95, The file ends with an extra blank line after the final token "exit"; remove the additional empty line so the file terminates with exactly one trailing newline character. Locate the final line containing "exit" and ensure there is only a single newline immediately after it (no extra blank lines).qa/1666 (1)
62-63:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the extra trailing blank line at EOF.
Line [63] leaves an extra blank line after
exit; this file should end with a single trailing newline.Suggested fix
-exit - +exitAs per coding guidelines "Ensure all text and source files end with a single trailing newline character".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@qa/1666` around lines 62 - 63, The file currently has an extra blank line after the final "exit" line; update the file so it ends immediately after the "exit" line with exactly one trailing newline (remove the extra blank line at EOF).qa/new (1)
63-83:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
-Rno longer works with numbers created by-r.
-rcreates the skeletal$idand$id.outfiles, but this new-Rpath rejects the reserved number as soon as either file exists. That makes the documented reserve → unreserve flow fail before the group entry is updated, and the later skeleton-generation path still assumes the files do not exist.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@qa/new` around lines 63 - 83, The unreserve (-R) branch incorrectly rejects an ID if $id or $id.out exist even when the group file shows the ID is reserved (breaking the documented reserve->unreserve flow); update the logic in the unreserve handling (the block using unreserve=true, id="$OPTARG", grep "^$id:reserved" group and the subsequent file checks) to allow proceeding when the group entry shows reserved—even if $id or $id.out files exist (these may have been created by -r); keep the check that forbids using -R after -r by preserving the if $reserve ... error check, but remove or relax the immediate file-existence exit-on-error so the group file can be updated to unreserved.src/libpcp/src/derive_parser.y.in (1)
1329-1334:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve right-hand extra units in multiplication.
When only the right operand carries an extra unit and the left operand is a non-constant dimensionless metric,
np->descis copied from the left side and this branch never restoresextraUnit/extraScale. The result then loses its extra-unit metadata even though the multiplication is otherwise allowed.Suggested fix
if (np->type == N_STAR) { np->desc.units.dimCount = left->units.dimCount + right->units.dimCount; np->desc.units.dimTime = left->units.dimTime + right->units.dimTime; np->desc.units.dimSpace = left->units.dimSpace + right->units.dimSpace; - /* prior checks ensure no extra units for multiplication */ + if (left->units.extraUnit == 0 && right->units.extraUnit != 0) { + np->desc.units.extraUnit = right->units.extraUnit; + np->desc.units.extraScale = right->units.extraScale; + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libpcp/src/derive_parser.y.in` around lines 1329 - 1334, The multiplication branch for N_STAR updates dims but doesn't preserve extra units/scales when np->desc was copied from left; modify the N_STAR handling (around np, left, right) to set np->desc.extraUnit and np->desc.extraScale from right when right->desc.extraUnit is present (and left has none), i.e. after updating dimCount/dimTime/dimSpace assign extraUnit/extraScale from right->desc into np->desc so the resulting node retains the right-hand extra unit metadata.src/libpcp_import/src/import.c (1)
172-185:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid shared function-local static
pmUnitsinpmiUnits/pmiExtraUnits(shared mutable state + race risk)
Both helpers declarestatic pmUnits units;, overwrite its fields on each call, and return it by value—introducing shared mutable state and possible data races under concurrent callers.Suggested fix
pmUnits pmiUnits(int dimSpace, int dimTime, int dimCount, unsigned int scaleSpace, unsigned int scaleTime, unsigned int scaleCount) { - static pmUnits units; + pmUnits units; units.dimSpace = dimSpace; units.dimTime = dimTime; units.dimCount = dimCount; units.scaleSpace = scaleSpace; units.scaleTime = scaleTime; units.scaleCount = scaleCount; units.extraUnit = 0; units.extraScale = 0; return units; } pmUnits pmiExtraUnits(int dimSpace, int dimTime, int dimCount, unsigned int scaleSpace, unsigned int scaleTime, unsigned int scaleCount, int extraUnit, unsigned int extraScale) { - static pmUnits units; + pmUnits units; units.dimSpace = dimSpace; units.dimTime = dimTime; units.dimCount = dimCount; units.scaleSpace = scaleSpace; units.scaleTime = scaleTime; units.scaleCount = scaleCount; units.extraUnit = extraUnit; units.extraScale = extraScale; return units; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libpcp_import/src/import.c` around lines 172 - 185, The helper uses a shared function-local static pmUnits (in pmiUnits and similarly in pmiExtraUnits) causing shared mutable state and data-race risk; change the declaration to a non-static local pmUnits variable (stack-allocated) and initialize its fields (or use a compound initializer) before returning so each call returns an independent struct instance instead of reusing a static one.src/pmdas/statsd/src/pmda-callbacks.c (1)
927-995:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix missing
duration_statdeclaration instatsd_resolve_dynamic_metric_fetch()
duration_statis assigned/used starting at line 940, but there is no declaration in the function scope before that use insrc/pmdas/statsd/src/pmda-callbacks.c, so this won’t compile. Declare it asenum DURATION_INSTANCE duration_statalongside the other local variables.Suggested fix
static int statsd_resolve_dynamic_metric_fetch(pmdaMetric* mdesc, unsigned int instance, pmAtomValue** atom) { + enum DURATION_INSTANCE duration_stat; struct pmda_metric_helper* helper = (struct pmda_metric_helper*) mdesc->m_user;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pmdas/statsd/src/pmda-callbacks.c` around lines 927 - 995, The function statsd_resolve_dynamic_metric_fetch() uses duration_stat but never declares it; add a local declaration "enum DURATION_INSTANCE duration_stat;" near the other locals (e.g., alongside helper, data, config, result, serial, is_default_domain, status) so duration_stat is in scope before its first assignment in the METRIC_TYPE_DURATION branches; ensure the enum type DURATION_INSTANCE is included via existing headers or forward declarations already used by functions like map_to_duration_instance and get_duration_instance.
🟠 Major comments (20)
qa/common.check-2230-2231 (1)
2230-2231:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSuppress
systemctlstderr in scheduler teardown path.At Line 2230 and Line 2231, only stdout is redirected. Missing units or stop errors can print to stderr and introduce nondeterministic noise in QA runs.
Suggested fix
- $__rc_sudo systemctl is-active $__i >/dev/null || continue - $__rc_sudo systemctl stop $__i >/dev/null + $__rc_sudo systemctl is-active $__i >/dev/null 2>&1 || continue + $__rc_sudo systemctl stop $__i >/dev/null 2>&1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@qa/common.check` around lines 2230 - 2231, The scheduler teardown currently only redirects stdout for the systemctl checks and stops, so error messages still go to stderr and create nondeterministic QA noise; update the two invocations $__rc_sudo systemctl is-active $__i and $__rc_sudo systemctl stop $__i to also redirect stderr (append 2>/dev/null) so both stdout and stderr are suppressed during teardown.qa/common.check-396-396 (1)
396-396:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix redirection order for verify capture.
At Line 396, stderr is duplicated before stdout is redirected, so stderr bypasses
$tmp.verifyand leaks to terminal output.Suggested fix
- pminfo -v $__agent 2>&1 >$tmp.verify + pminfo -v $__agent >$tmp.verify 2>&1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@qa/common.check` at line 396, The current pminfo invocation redirects stderr to the original stdout before stdout is redirected, causing stderr to leak; change the redirection order on the pminfo command (the line invoking pminfo -v $__agent) so stdout is first redirected into $tmp.verify and then stderr is redirected to that stream (use >$tmp.verify 2>&1) to capture both stdout and stderr into $tmp.verify.qa/new-397-399 (1)
397-399:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid
sed -ifor portability
sed -iis not POSIX-specified, and GNU vs BSD/macOSseddiffer in how-iis invoked, sosed -e "/^$id:reserved/d" -i groupmay be incompatible and fail on non-GNU hosts.Proposed portable change
- sed -e "/^$id:reserved/d" -i group + sed -e "/^$id:reserved/d" group >"$tmp.group" + mv "$tmp.group" group🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@qa/new` around lines 397 - 399, Replace the in-place sed invocation that deletes the ":reserved" line with a portable approach: run the sed expression (the "/^$id:reserved/d" operation referencing the id variable) and write the output to a temporary file (created with mktemp), then atomically move/rename the temp file over the original "group" file and clean up the temp on error; ensure you handle errors (non-zero exit) before overwriting and use a trap to remove the temp on failure so the replace is safe on both GNU and BSD/macOS systems.qa/src/units-parse.c-45-58 (1)
45-58:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid reading
reversed/reversed_multiplierwhenpmParseUnitsStrfails
qa/src/units-parse.creadsreversed(and prints it) even whensts != 0:
pmunits_roundtrip():pmUnitsStr_r(&reversed, convertedt, ...)and theprint_pfprintfusereversed/reversed_multiplierbefore checkingsts.pmunits_parse():pmUnitsStr_r(&reversed, ...)and thefprintfusereversed/reversed_multiplierunconditionally, before anystscheck.Guard the
pmUnitsStr_rcall(s) and printing so they only run whensts == 0(and avoid usingreversed/reversed_multiplierotherwise).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@qa/src/units-parse.c` around lines 45 - 58, The test prints use of reversed/reversed_multiplier even when pmParseUnitsStr (sts) failed; modify pmunits_roundtrip and pmunits_parse to only call pmUnitsStr_r(&reversed, ...) and only include reversed/reversed_multiplier in the fprintf when sts == 0 (i.e., after confirming pmParseUnitsStr returned success). In practice, wrap the pmUnitsStr_r invocation and the portion of the fprintf that references reversed/reversed_multiplier with an if (sts == 0) guard (or split the fprintf into two branches) so reversed and reversed_multiplier are never read or printed when pmParseUnitsStr failed.src/collectl2pcp/GNUmakefile-37-40 (1)
37-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
pmdescas an explicit prerequisite formetrics.cgeneration.
metrics.cinvokes./pmdescbut does not depend onpmdesc. In parallel builds this can race and fail intermittently.Suggested fix
-metrics.c: metrics.h pmdesc.c +metrics.c: metrics.h pmdesc.c pmdesc PCP_DERIVED_CONFIG='' pminfo hinv kernel mem swap network disk filesys \ swapdev rpc nfs nfs3 nfs4 xfs pmda ipc vfs quota tmpfs sysfs proc \ | ./pmdesc > metrics.c🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/collectl2pcp/GNUmakefile` around lines 37 - 40, The make rule that generates metrics.c must explicitly depend on the pmdesc executable to avoid races: update the rule for the target metrics.c (currently listing metrics.h and pmdesc.c) to also depend on the pmdesc binary (add "pmdesc" as a prerequisite) because the recipe runs "./pmdesc"; ensure the prerequisite is the built executable (not just pmdesc.c) so make will build pmdesc before running the command that produces metrics.c.src/libpcp/src/derive_parser.y.in-1711-1721 (1)
1711-1721:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't reject same extra units just because their scales differ.
This hard equality check makes expressions like
V + mV,A > mA, orW / mWfail even though the new extra-unit model explicitly adds scale support. Existing count/time/space units are normalized before arithmetic; extra units need the same treatment instead of requiring identicalextraScalevalues up front.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libpcp/src/derive_parser.y.in` around lines 1711 - 1721, The current check in the derive parser rejects operands when left->units.extraUnit == right->units.extraUnit but left->units.extraScale != right->units.extraScale; instead, implement scale normalization for extra units so differing extraScale values are allowed when they represent the same physical unit at different scales. Update the block that references left->units.extraUnit, right->units.extraUnit, left->units.extraScale, right->units.extraScale and PM_TPD(derive_errmsg) to: when both extraUnit fields are non-zero, first verify the extraUnit identity (unit id matches), then normalize one operand’s extraScale to the other (or compute a common scale) before proceeding; only set PM_TPD(derive_errmsg) and goto bad if the unit identities differ or normalization fails (e.g., incompatible scales), not merely because extraScale values differ.src/libpcp/src/extraunits.c-609-609 (1)
609-609:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse double precision in PM_TYPE_DOUBLE conversion.
PM_TYPE_DOUBLEcurrently casts viafloat, which truncates precision for all double conversions.Proposed fix
case PM_TYPE_DOUBLE: - val->d = (float)(cp->a + cp->b * (val->d + cp->c) / cp->d); + val->d = (double)(cp->a + cp->b * (val->d + cp->c) / cp->d); break;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libpcp/src/extraunits.c` at line 609, The PM_TYPE_DOUBLE conversion currently forces a float cast which loses precision; update the conversion expression by removing the (float) cast and using double precision so the computation of val->d uses full double precision (reference symbols: PM_TYPE_DOUBLE, val->d, cp->a, cp->b, cp->c, cp->d). Ensure the result and any intermediate values remain in double precision (not float) so double-to-double conversion is preserved.src/libpcp/src/units.c-1584-1591 (1)
1584-1591:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject unit strings that place extra units on both sides of
/.This block silently prefers the dividend’s extra unit and discards the divisor’s when both are present, producing an incorrect parse result.
Proposed fix
/* * extra units come from either dividend or divisor, but not both */ + if (dividend.extraUnit != 0 && divisor.extraUnit != 0) { + *errMsg = strdup("extra units cannot appear in both dividend and divisor"); + sts = PM_ERR_CONV; + goto out; + } if (dividend.extraUnit != 0) { out->extraUnit = dividend.extraUnit; out->extraScale = dividend.extraScale; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libpcp/src/units.c` around lines 1584 - 1591, Detect the case where both dividend.extraUnit and divisor.extraUnit are non-zero and treat it as a parse error instead of choosing one side; in the block that currently assigns out->extraUnit/out->extraScale, add a check for (dividend.extraUnit != 0 && divisor.extraUnit != 0) and return a failure (or set the function's error state) so the parser rejects unit strings with extra units on both sides of '/'; do not silently prefer dividend—emit a clear error (or error code) and ensure callers handle the failure.src/libpcp/src/units.c-175-192 (1)
175-192:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
buflenbefore writing tobuf.
buf[0] = '\0'is executed before the size check, so a zero-length buffer can be written out of bounds.Proposed fix
- buf[0] = '\0'; - - if (pu->extraUnit) { - __pmExtraUnitsStr(pu, xbuf, sizeof(xbuf)); - extrastr = xbuf; - } - - /* - * must be at least 60 bytes in buf[], then we don't need to - * pollute the code below with a check every time we call - * pmsprintf() or increment p - */ if (buflen < 60) { if (pmDebugOptions.pmapi) { fprintf(stderr, ":> returns NULL\n"); } return NULL; } + buf[0] = '\0'; + + if (pu->extraUnit) { + __pmExtraUnitsStr(pu, xbuf, sizeof(xbuf)); + extrastr = xbuf; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libpcp/src/units.c` around lines 175 - 192, The code writes to buf[0] before validating buflen, which can overflow for a zero-length buffer; update the function to validate buflen first (ensure buflen >= 60 or at least buflen > 0) before assigning buf[0] = '\0' or move the buf[0] = '\0' line to after the buflen checks; keep the existing logic that uses buf, extrastr, xbuf and the __pmExtraUnitsStr(pu, xbuf, sizeof(xbuf)) call but only perform any write into buf after the size guard passes.src/libpcp_web/src/query.c-4718-4726 (1)
4718-4726:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate extra-unit operator constraints before arithmetic loops.
These checks run after per-sample/per-instance mutation. For invalid combinations, data has already been modified before returning
-EPROTO.Move the validation to just after
series_calculate_binary_check(...)and before iterating samples.Also applies to: 4791-4803
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libpcp_web/src/query.c` around lines 4718 - 4726, The extra-unit validation (checks of l_units.extraUnit and r_units.extraUnit that return -EPROTO) must be moved earlier so it runs before any per-sample or per-instance mutation: relocate the block that inspects l_units.extraUnit and r_units.extraUnit to immediately after the call to series_calculate_binary_check(...) and before any loops/iteration over samples/instances in the function (and do the same relocation for the duplicate check around lines 4791-4803). Ensure the check still logs via infofmt/batoninfo and sets baton->error = -EPROTO and returns without modifying data.src/libpcp_web/src/query.c-4566-4570 (1)
4566-4570:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate
extraScalewithextraUnitfor + and -.Result units copy
extraUnitbut notextraScale, so metadata can be rewritten with an incorrect extra scale (default zero).Suggested fix
large_units.dimCount = l_units.dimCount; large_units.dimSpace = l_units.dimSpace; large_units.dimTime = l_units.dimTime; large_units.extraUnit = l_units.extraUnit; + large_units.extraScale = l_units.extraScale;Apply the same change in both
series_calculate_plusandseries_calculate_minus.Also applies to: 4619-4623
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libpcp_web/src/query.c` around lines 4566 - 4570, The code copies large_units.extraUnit from l_units.extraUnit but fails to copy l_units.extraScale, causing incorrect metadata; update both series_calculate_plus and series_calculate_minus to also set large_units.extraScale = l_units.extraScale wherever large_units.extraUnit is assigned (use the same spot where large_units.dimCount/dimSpace/dimTime/extraUnit are set) so result units propagate both extraUnit and extraScale consistently.src/pmdas/sample/pmns-384-387 (1)
384-387:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRename
farenheittofahrenheitbefore the PMNS name is finalized.The misspelled metric leaf becomes part of the public namespace/API. It’s much cheaper to correct now than after users/scripts depend on it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pmdas/sample/pmns` around lines 384 - 387, In the PMNS definition under the sample.temperature node, rename the misspelled leaf "farenheit" to "fahrenheit" so the public metric name is correct; update the PMNS entry in sample.temperature (replace the "farenheit" label with "fahrenheit" and keep the same identifier SAMPLE:0:173), and then search for and update any references, tests, docs or consumers that refer to sample.temperature.farenheit to use sample.temperature.fahrenheit before the namespace is finalized.src/pmdas/summary/summary.pmie-37-40 (1)
37-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard against divide-by-zero when there are no active interfaces.
Both expressions divide by
(count_inst $netio > 0), which can be zero and cause evaluation errors.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pmdas/summary/summary.pmie` around lines 37 - 40, Both expressions can divide by zero when count_inst $netio equals 0; update summary.netif.packets and summary.netif.busy to guard using the instance count before dividing — e.g., compute a local denominator from count_inst $netio and use a conditional (if count_inst $netio > 0 then sum_inst ($netio) / count_inst $netio else 0) for summary.netif.packets and similarly for summary.netif.busy (use the same conditional pattern or a ternary-like expression) so you never perform division when count_inst $netio is zero.src/pmdas/sample/src/sample.c-2753-2755 (1)
2753-2755:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKelvin conversion constant is wrong.
Celsius→Kelvin should add
273.15, not237.15.Suggested fix
- atom.f = 237.15 + temperature; + atom.f = 273.15 + temperature;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pmdas/sample/src/sample.c` around lines 2753 - 2755, The Kelvin conversion in the sample.temperature.kelvin case is using the wrong constant; update the expression in the switch case handling case 174 (symbol: atom.f = 237.15 + temperature) to use 273.15 instead of 237.15 so Celsius→Kelvin uses 273.15 (i.e., change the literal in the case 174 branch).src/pmdas/sample/src/sample.c-2789-2795 (1)
2789-2795:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMilliwatt fetch uses micro-watt scaling.
sample.power.milliwattshould multiply watts by1000, not1000000.Suggested fix
- atom.f = power * 1000000; + atom.f = power * 1000;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pmdas/sample/src/sample.c` around lines 2789 - 2795, The milliwatt case is using the wrong scale: in the switch handling (cases for sample.power.milliwatt and sample.power.microwatt) the code sets atom.f = power * 1000000 for both; change the multiplier for sample.power.milliwatt (case 183) to 1000 (i.e. atom.f = power * 1000) while leaving sample.power.microwatt (case 184) at 1000000, referencing the case labels and the atom.f assignment that use the power variable.src/perl/PMDA/test.pl-92-109 (1)
92-109:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate TAP plan to include the new fifth test.
This block emits
ok 5/not ok 5, but the plan still declares four tests (Line 19). That causes a TAP plan mismatch.Suggested fix
-BEGIN { $| = 1; print "1..4\n"; } +BEGIN { $| = 1; print "1..5\n"; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/perl/PMDA/test.pl` around lines 92 - 109, The TAP output in test.pl now runs a fifth test (prints "ok 5"/"not ok 5") but the TAP plan still declares four tests; update the TAP plan declaration from 1..4 to 1..5 so the plan matches the actual tests. Locate the TAP plan string near the top of the test script (where the "1..4" plan is emitted) and change it to "1..5" so pmda_extraunits test results align with the declared plan.src/pmdas/sample/src/sample.c-3401-3436 (1)
3401-3436:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStore-side unit conversions are inverted in multiple cases.
Several assignments convert in the wrong direction (e.g., mV/µV/mA/µA multiply instead of divide; kW divides instead of multiplies; mW/µW multiply instead of divide), and Kelvin uses the wrong offset/sign.
Suggested fix
- case 174: /* sample.temperature.kelvin */ - temperature = av.f + 237.15; + case 174: /* sample.temperature.kelvin */ + temperature = av.f - 273.15; break; @@ - case 176: /* sample.voltage.millivolt */ - voltage = av.f * 1000; + case 176: /* sample.voltage.millivolt */ + voltage = av.f / 1000; break; - case 177: /* sample.voltage.microvolt */ - voltage = av.f * 1000000; + case 177: /* sample.voltage.microvolt */ + voltage = av.f / 1000000; break; @@ - case 179: /* sample.current.milliamp */ - current = av.f * 1000; + case 179: /* sample.current.milliamp */ + current = av.f / 1000; break; - case 180: /* sample.current.microamp */ - current = av.f * 1000000; + case 180: /* sample.current.microamp */ + current = av.f / 1000000; break; @@ - case 181: /* sample.power.kilowatt */ - power = av.f / 1000; + case 181: /* sample.power.kilowatt */ + power = av.f * 1000; break; @@ - case 183: /* sample.power.milliwatt */ - power = av.f * 1000; + case 183: /* sample.power.milliwatt */ + power = av.f / 1000; break; - case 184: /* sample.power.microwatt */ - power = av.f * 1000000; + case 184: /* sample.power.microwatt */ + power = av.f / 1000000; break;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pmdas/sample/src/sample.c` around lines 3401 - 3436, The unit conversions in sample.c are inverted: fix the formulas so values in av.f (which represent the incoming unit) are converted to the base unit (SI) before storing into temperature/voltage/current/power. Specifically: in case 173 keep Fahrenheit->C as (av.f - 32) / 1.8 (verify it's intended); in case 174 set Kelvin conversion to av.f + 273.15; for voltage change case 176 (sample.voltage.millivolt) to voltage = av.f / 1000 and case 177 (sample.voltage.microvolt) to voltage = av.f / 1000000; for current change case 179 (milliamp) to current = av.f / 1000 and case 180 (microamp) to current = av.f / 1000000; for power change case 181 (kilowatt) to power = av.f * 1000, keep case 182 as power = av.f, change case 183 (milliwatt) to power = av.f / 1000 and case 184 (microwatt) to power = av.f / 1000000 so all stored values are in base units.src/pmie/src/dstruct.c-859-860 (1)
859-860:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
CND_RATEstops applying/timefor zero-time-dimension inputsThis guard changes unit propagation semantics:
rate()should still decrementdimTimefrom0to-1(e.g., count → count/sec). With the new condition, those paths keep incorrect units.Suggested fix
- if (x->units.dimTime != 0) - x->units.dimTime--; + x->units.dimTime--; + if (x->units.dimTime == 0) + x->units.scaleTime = 0;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pmie/src/dstruct.c` around lines 859 - 860, The change prevents rate() from decrementing time dimension when dimTime is 0, breaking unit propagation for zero-time-dimension inputs; restore the original semantics by removing the conditional guard so that in the CND_RATE/rate handling code you always decrement x->units.dimTime (i.e., unconditionally do x->units.dimTime-- for the node handling rate/CND_RATE) so values with dimTime==0 become -1 (count→count/sec).src/pmlogrewrite/lex.l-138-138 (1)
138-138:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset
in_extraand return tometricstate after the second extra macro.After
extraunits, the lexer can remain inunitsmode (in_extrais never reset, and<extra>always doesBEGIN(units)), sorescaleand subsequent metric clauses are tokenized incorrectly.Proposed fix
-<metric>"units" { BEGIN(units); comma_count = 0; return TOK_UNITS; } +<metric>"units" { BEGIN(units); comma_count = 0; in_extra = 0; return TOK_UNITS; } <metric>"extraunits" { BEGIN(units); comma_count = 0; in_extra = 1; return TOK_EXTRAUNITS; } -<extra>[A-Z][A-Z_]* { yylval.str = dupstr(yytext, 0); BEGIN(units); return TOK_MACRO; } +<extra>[A-Z][A-Z_]* { + yylval.str = dupstr(yytext, 0); + if (comma_count == 7) { + in_extra = 0; + BEGIN(metric); + } + else { + BEGIN(units); + } + return TOK_MACRO; + }Also applies to: 206-209, 232-250
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pmlogrewrite/lex.l` at line 138, The lexer rule for <metric> "extraunits" (which currently does BEGIN(units); comma_count = 0; in_extra = 1; return TOK_EXTRAUNITS;) leaves the lexer stuck in units mode because in_extra is never cleared; update this rule and the related <extra> handling (also at the other occurrences around the blocks noted) so that after the second/closing extra macro you set in_extra = 0 and call BEGIN(metric) to return to the metric state (i.e., ensure the closing/second extra macro rule clears in_extra and switches back from units to metric rather than leaving BEGIN(units) active).src/python/pcp/pmconfig.py-952-954 (1)
952-954:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not abort on valid extra-unit metrics.
This unconditional
sys.exit(1)makes tools fail for metrics that legitimately use extra units. Prefer continuing with native units (or route through existing incompatible-metric handling) instead of hard-exiting.Suggested minimal fix
- if is_extra(unit): - sys.stderr.write("TODO extra units: %s.\n" % (self.util.metrics[metric][2])) - sys.exit(1)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/python/pcp/pmconfig.py` around lines 952 - 954, The code unconditionally exits when encountering an extra-unit metric; instead, remove the sys.exit(1) and just emit a warning and continue—i.e., in the block where is_extra(unit) is true (referencing is_extra(unit), self.util.metrics[metric][2], metric, unit), replace the sys.exit call with logic that logs the message to stderr (or via the existing logger) and falls back to using the native units or routes the metric through the existing incompatible-metric handling path rather than aborting the process.
🟡 Minor comments (11)
man/man3/pmlookupdesc.3-160-161 (1)
160-161:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix typo: "farenheit" should be "fahrenheit".
Line 252 contains a spelling error in the comment for
PM_TEMPERATURE_Fwhere "farenheit" should be "fahrenheit".📝 Proposed fix
`#define` PM_TEMPERATURE_C 1 /* celsius */ -#define PM_TEMPERATURE_F 2 /* farenheit */ +#define PM_TEMPERATURE_F 2 /* fahrenheit */ `#define` PM_TEMPERATURE_K 3 /* kelvin */Also applies to: 198-214, 239-263
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@man/man3/pmlookupdesc.3` around lines 160 - 161, Fix the spelling of "farenheit" to "fahrenheit" in the documentation comments for PM_TEMPERATURE_F and any other occurrences in this manpage; update the comment text around the PM_TEMPERATURE_F symbol (and other similar temperature-entry comments in the same document) to use the correct spelling "fahrenheit" everywhere.man/man3/pmconvscale.3-77-87 (1)
77-87:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix typo: "mast" should be "must".
Line 84 contains a spelling error where "mast" appears instead of "must".
📝 Proposed fix
.I extraUnit -mast have the same value in both +must have the same value in both .I iunit🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@man/man3/pmconvscale.3` around lines 77 - 87, The documentation contains a typo: the word "mast" should be corrected to "must" in the sentence describing dimensionality equality; update the text in man/man3/pmconvscale.3 where it lists dimSpace, dimTime, dimCount and extraUnit to read "...must have the same value in both iunit and ounit." ensuring the reference names (dimSpace, dimTime, dimCount, extraUnit, iunit, ounit) remain unchanged.qa/src/badUnitsStr_r.c-28-30 (1)
28-30:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard both
mallocresults before writing intobuffer.Each path dereferences
bufferimmediately, so an allocation failure turns this QA helper into a null-pointer crash instead of a useful test failure.Proposed fix
- buffer = (char *)malloc(MYSIZE_OK); + buffer = (char *)malloc(MYSIZE_OK); + if (buffer == NULL) { + perror("malloc"); + return 1; + } buffer[0] = '\0'; @@ - buffer = (char *)malloc(MYSIZE_BAD); + buffer = (char *)malloc(MYSIZE_BAD); + if (buffer == NULL) { + perror("malloc"); + return 1; + } buffer[0] = '\0';Also applies to: 47-49
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@qa/src/badUnitsStr_r.c` around lines 28 - 30, The code currently dereferences buffer right after malloc (buffer[0] = '\0') and then calls pmUnitsStr_r, so check the result of malloc before writing or calling pmUnitsStr_r: after buffer = (char *)malloc(MYSIZE_OK) verify buffer != NULL and handle the allocation failure (e.g., set result to a failure/skip value, log/error out, or return early) instead of dereferencing; apply the same null-check pattern for the other occurrences around lines invoking pmUnitsStr_r and using MYSIZE_OK so the test reports allocation failure instead of crashing.qa/src/pdu-gadget.c-724-726 (1)
724-726:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix wrong field in
extraUnitoverflow message.The overflow log currently prints
units.scaleCount, which makes debugging misleading whenextraUnitoverflows.Suggested fix
- fprintf(stderr, "%d: units(): extraUnit overflow %d -> %d(\n", lineno, f, units.scaleCount); + fprintf(stderr, "%d: units(): extraUnit overflow %d -> %d(\n", lineno, f, units.extraUnit);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@qa/src/pdu-gadget.c` around lines 724 - 726, The overflow log in the units handling prints the wrong field (it currently prints units.scaleCount) making extraUnit overflow diagnostics misleading; update the fprintf call in the units() handling (the branch that checks if units.extraUnit != f) to include units.extraUnit instead of units.scaleCount so the message reports the actual overflowing field (refer to the fprintf invocation that uses lineno, f, units.scaleCount and change the third printed value to units.extraUnit).qa/src/check_import.c-105-106 (1)
105-106:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd missing status check for
pmiAddMetric("my.metric.temperature", ...)
qa/src/check_import.cassignssts = pmiAddMetric("my.metric.temperature", ...)but does not callcheck(sts, "pmiAddMetric")beforestsis overwritten by the nextpmiAddInstance(...), so metric-add failures are silently dropped.Suggested fix
sts = pmiAddMetric("my.metric.float", PM_ID_NULL, PM_TYPE_FLOAT, PM_INDOM_NULL, PM_SEM_DISCRETE, pmiUnits(0,0,0,0,0,0)); check(sts, "pmiAddMetric"); sts = pmiAddMetric("my.metric.temperature", PM_ID_NULL, PM_TYPE_FLOAT, PM_INDOM_NULL, PM_SEM_INSTANT, pmiExtraUnits(0,0,0,0,0,0,PM_UNIT_TEMPERATURE,PM_TEMPERATURE_C)); + check(sts, "pmiAddMetric");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@qa/src/check_import.c` around lines 105 - 106, The call to pmiAddMetric("my.metric.temperature", ...) assigns sts but never verifies it; add a status check by calling check(sts, "pmiAddMetric") immediately after the pmiAddMetric call (before the subsequent pmiAddInstance call overwrites sts) so failures are not silently dropped; modify the sequence around pmiAddMetric, pmiAddInstance, and the sts variable to ensure check(sts, "pmiAddMetric") is executed right after pmiAddMetric returns.src/libpcp/src/extraunits.c-169-171 (1)
169-171:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix trailing-space consumption after parsing extra units.
The loop meant to consume trailing spaces currently consumes non-space characters instead, which can advance the parse pointer incorrectly.
Proposed fix
- /* and gobble any trailing space(s) */ - while (*ptr && !isspace(*ptr)) + /* and gobble any trailing space(s) */ + while (*ptr && isspace((unsigned char)*ptr)) ptr++;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libpcp/src/extraunits.c` around lines 169 - 171, The loop that should skip trailing spaces after parsing extra units currently advances over non-space characters ("while (*ptr && !isspace(*ptr)) ptr++;") which moves the pointer wrongly; change it to skip actual whitespace by using "while (*ptr && isspace((unsigned char)*ptr)) ptr++;" so ptr advances only over space characters and use the (unsigned char) cast to safely call isspace on signed chars (the ptr variable and the loop in extraunits.c should be updated).src/libpcp_qmc/src/qmc_desc.cpp-190-196 (1)
190-196:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix extra-unit sign check in denominator formatting.
The denominator branch is entered for negative exponents, but this check uses
pu->extraUnit > 0, so negative extra units are omitted from the denominator output.Suggested fix
- if (pu->extraUnit > 0) { + if (pu->extraUnit < 0) { pmsprintf(p, sizeof(buf)-(p-buf), "%s", extraString); while (*p) p++; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libpcp_qmc/src/qmc_desc.cpp` around lines 190 - 196, The denominator branch for negative exponents currently checks pu->extraUnit > 0 and thus skips negative extraUnit values; change the condition to pu->extraUnit < 0 so extraString is printed into the denominator when extraUnit is negative (keep using pmsprintf and the existing pointer arithmetic with p, div, buf to append the string).src/perl/PMDA/cvalue.c-68-83 (1)
68-83:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInitialize
forw/backbefore packing inextraunits().On Line 74,
extraunits()assigns named fields but does not clear the structs first. Any padding/reserved bits can leak into*(unsigned int *)&forw/back, making this verifier output nondeterministic.Proposed fix
void extraunits(void) { pmUnits forw; pmUnits back; + + memset(&forw, 0, sizeof(forw)); + memset(&back, 0, sizeof(back)); forw.dimSpace = back.extraScale = 1;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/perl/PMDA/cvalue.c` around lines 68 - 83, The extraunits() function assigns individual fields of the pmUnits variables forw and back but never zero-initializes them, so padding/reserved bits can leak into the packed integer prints; before setting any fields in extraunits(), zero or otherwise initialize both forw and back (e.g., memset to 0 or value-initialize) so all padding/reserved bits are deterministic, then proceed to assign the named fields and call the printf lines that reference *(unsigned int *)&forw and *(unsigned int *)&back.src/pmdas/summary/help-53-55 (1)
53-55:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove stray parenthesis in help text.
packets per second).has an extra).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pmdas/summary/help` around lines 53 - 55, The help text contains an extra closing parenthesis in the string "Fraction of the active network interfaces busy serving at least 400 packets per second)." — remove the stray ")" so the sentence reads "...400 packets per second." in the help resource (update the help text constant or string where that exact phrase appears).src/pmdas/sample/help-942-998 (1)
942-998:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix user-facing typos in new help entries.
There are multiple spelling/wording errors in this block (e.g.,
wqater,Farenheit, and themode=1. and up.sentence). Please clean these up before release.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pmdas/sample/help` around lines 942 - 998, Fix typos and stray text in the sample help entries: correct "wqater" to "water" in all sample.temperature.* descriptions, change "Farenheit" to "Fahrenheit" in the sample.temperature.farenheit heading and description, remove or fix the stray fragment "mode=1. and up." (delete the "and up." piece), and ensure phrasing is consistent (e.g., "degrees Fahrenheit/Kelvin/Celsius" and "defaults to the freezing point of water at sea level") for sample.temperature.celsius, sample.temperature.farenheit, sample.temperature.kelvin and similar entries; update any matching lines for sample.voltage.*, sample.current.*, and sample.power.* to preserve consistent wording and correct spelling.src/python/pmapi.c-929-929 (1)
929-929:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the argument-parser function label.
The parser error context string should reference
pmUnits_int, notpmSetContextOptions, otherwise Python raises misleading argument errors.Suggested fix
- "IIIIIIII:pmSetContextOptions", keyword_list, + "IIIIIIII:pmUnits_int", keyword_list,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/python/pmapi.c` at line 929, The argument-parser context label is incorrect: replace the string "IIIIIIII:pmSetContextOptions" used with keyword_list in the argument parsing call with "IIIIIIII:pmUnits_int" so the parser reports errors for the pmUnits_int function/arguments rather than pmSetContextOptions; locate the call that passes keyword_list alongside the label (the one currently using pmSetContextOptions) and change the label to pmUnits_int.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: ee7b7e3f-3884-4909-8233-8af40272220e
⛔ Files ignored due to path filters (34)
qa/029.outis excluded by!**/*.outqa/1193.outis excluded by!**/*.outqa/1231.outis excluded by!**/*.outqa/1247.outis excluded by!**/*.outqa/1291.outis excluded by!**/*.outqa/139.outis excluded by!**/*.outqa/1418.outis excluded by!**/*.outqa/1605.outis excluded by!**/*.outqa/1664.outis excluded by!**/*.outqa/1665.outis excluded by!**/*.outqa/1666.outis excluded by!**/*.outqa/1667.outis excluded by!**/*.outqa/190.outis excluded by!**/*.outqa/256.outis excluded by!**/*.outqa/273.outis excluded by!**/*.outqa/307.outis excluded by!**/*.outqa/369.outis excluded by!**/*.outqa/483.outis excluded by!**/*.outqa/486.outis excluded by!**/*.outqa/494.outis excluded by!**/*.outqa/499.outis excluded by!**/*.outqa/509.outis excluded by!**/*.outqa/514.outis excluded by!**/*.outqa/520.outis excluded by!**/*.outqa/569.outis excluded by!**/*.outqa/578.outis excluded by!**/*.outqa/628.outis excluded by!**/*.outqa/662.outis excluded by!**/*.outqa/670.outis excluded by!**/*.outqa/736.outis excluded by!**/*.outqa/746.outis excluded by!**/*.outqa/762.outis excluded by!**/*.outqa/797.outis excluded by!**/*.outqa/882.outis excluded by!**/*.out
📒 Files selected for processing (112)
man/man1/pmie.1man/man1/pminfo.1man/man1/pmlogrewrite.1man/man3/pmapi.3man/man3/pmconvscale.3man/man3/pmiaddmetric.3man/man3/pmiunits.3man/man3/pmlookupdesc.3man/man3/pmregisterderived.3qa/1067.out.1qa/1193qa/1231qa/1247qa/1267qa/1426qa/1664qa/1665qa/1666qa/1667qa/1713qa/255qa/255.out.ipv6qa/307qa/344.out.linuxqa/344.out.otherqa/367.out.beqa/367.out.leqa/483qa/510qa/628qa/670qa/736qa/COOKBOOK.mdqa/archives/extra.0qa/archives/extra.indexqa/archives/extra.metaqa/archives/mk.extraqa/common.checkqa/common.filterqa/groupqa/newqa/pdudata/descqa/pdudata/descsqa/pmdas/broken/broken_pmda.cqa/pmdas/broken/broken_pmda.v1.cqa/show-meqa/src/badUnitsStr_r.cqa/src/check_import.cqa/src/endian.cqa/src/grind_conv.cqa/src/multictx.cqa/src/pdu-gadget.cqa/src/pducheck.cqa/src/pmconvscale.cqa/src/units-parse.cqa/statsd/src/cases/07.pyscripts/man-spellsrc/bashrc/pcp_completion.shsrc/collectl2pcp/GNUmakefilesrc/collectl2pcp/metrics.csrc/collectl2pcp/pmdesc.csrc/include/pcp/import.hsrc/include/pcp/libpcp.hsrc/include/pcp/mmv_stats.hsrc/include/pcp/pmapi.hsrc/include/pcp/pmda.hsrc/libpcp/src/GNUmakefilesrc/libpcp/src/check-staticssrc/libpcp/src/derive_parser.y.insrc/libpcp/src/exports.insrc/libpcp/src/extraunits.csrc/libpcp/src/fetchgroup.csrc/libpcp/src/internal.hsrc/libpcp/src/logmeta.csrc/libpcp/src/units.csrc/libpcp_fault/src/GNUmakefilesrc/libpcp_import/src/exportssrc/libpcp_import/src/import.csrc/libpcp_qmc/src/qmc_desc.cppsrc/libpcp_static/src/GNUmakefilesrc/libpcp_web/src/query.csrc/libpcp_web/src/query_parser.y.insrc/perl/LogImport/LogImport.pmsrc/perl/LogImport/LogImport.xssrc/perl/MMV/MMV.pmsrc/perl/MMV/MMV.xssrc/perl/PMDA/PMDA.pmsrc/perl/PMDA/PMDA.xssrc/perl/PMDA/cvalue.csrc/perl/PMDA/test.plsrc/pmdas/sample/helpsrc/pmdas/sample/pmnssrc/pmdas/sample/src/sample.csrc/pmdas/statsd/src/pmda-callbacks.csrc/pmdas/statsd/src/pmdastatsd.csrc/pmdas/summary/helpsrc/pmdas/summary/summary.pmiesrc/pmdumptext/pmdumptext.cppsrc/pmie/src/dstruct.csrc/pmie/src/dstruct.hsrc/pmie/src/pragmatics.hsrc/pmie/src/syntax.csrc/pminfo/pminfo.csrc/pmlogextract/pmlogextract.csrc/pmlogger/src/checks.csrc/pmlogrewrite/gram.ysrc/pmlogrewrite/lex.lsrc/pmlogrewrite/pmlogrewrite.csrc/python/pcp/pmapi.pysrc/python/pcp/pmconfig.pysrc/python/pmapi.csrc/zshrc/_pcp
💤 Files with no reviewable changes (1)
- src/pmie/src/pragmatics.h
- qa/common.check
- src/collectl2pcp/GNUmakefile
- src/libpcp/src/extraunits.c
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/pmdas/sample/help (1)
945-958:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix typo in new temperature help text.
wqatershould bewaterin the newly added temperature descriptions.Suggested fix
- (defaults to the freezing point of wqater at sea level), but the value + (defaults to the freezing point of water at sea level), but the value🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pmdas/sample/help` around lines 945 - 958, Fix the typo "wqater" to "water" in the three newly added help descriptions for sample.temperature.celsius, sample.temperature.fahrenheit, and sample.temperature.kelvin; update the text that reads "(defaults to the freezing point of wqater at sea level)" to "(defaults to the freezing point of water at sea level)" so the help entries referencing pmstore(1) are correct and consistent.src/pmdas/sample/src/sample.c (2)
2753-2795:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix incorrect fetch conversions for Kelvin and milliwatts.
Two conversion formulas are incorrect: Kelvin offset and milliwatt scaling.
Suggested fix
case 174: /* sample.temperature.kelvin */ - atom.f = 237.15 + temperature; + atom.f = 273.15 + temperature; break; @@ case 183: /* sample.power.milliwatt */ - atom.f = power * 1000000; + atom.f = power * 1000; break;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pmdas/sample/src/sample.c` around lines 2753 - 2795, The Kelvin conversion in case 174 and the milliwatt scaling in case 183 are wrong: update the assignment in case 174 to use 273.15 + temperature (not 237.15), and change case 183 to set atom.f = power * 1000 (milliwatt = watt * 1e3); verify case 184 (microwatt) is atom.f = power * 1000000 and leave case 182 (watt) and case 181 (kilowatt) unchanged; modify the atom.f assignments for the cases referenced (case 174, case 183, and confirm case 184) accordingly.
3401-3436:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStore-path unit conversions are reversed for multiple scales.
Several inverse conversions are backwards, so pmStore writes the wrong base values for temperature/voltage/current/power.
Suggested fix
case 174: /* sample.temperature.kelvin */ - temperature = av.f + 237.15; + temperature = av.f - 273.15; break; @@ case 176: /* sample.voltage.millivolt */ - voltage = av.f * 1000; + voltage = av.f / 1000; break; case 177: /* sample.voltage.microvolt */ - voltage = av.f * 1000000; + voltage = av.f / 1000000; break; @@ case 179: /* sample.current.milliamp */ - current = av.f * 1000; + current = av.f / 1000; break; case 180: /* sample.current.microamp */ - current = av.f * 1000000; + current = av.f / 1000000; break; @@ case 181: /* sample.power.kilowatt */ - power = av.f / 1000; + power = av.f * 1000; break; @@ case 183: /* sample.power.milliwatt */ - power = av.f * 1000; + power = av.f / 1000; break; case 184: /* sample.power.microwatt */ - power = av.f * 1000000; + power = av.f / 1000000; break;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pmdas/sample/src/sample.c` around lines 3401 - 3436, The store-path conversions are inverted for several scaled units: in the switch cases 173–184 fix the conversion directions so stored base units are correct — keep case 173 (fahrenheit) as Celsius = (av.f - 32) / 1.8, change case 174 (kelvin) to temperature = av.f - 273.15, for voltage change case 176 (millivolt) to voltage = av.f / 1000 and case 177 (microvolt) to voltage = av.f / 1000000, for current change case 179 (milliamp) to current = av.f / 1000 and case 180 (microamp) to current = av.f / 1000000, and for power change case 181 (kilowatt) to power = av.f * 1000, case 183 (milliwatt) to power = av.f / 1000 and case 184 (microwatt) to power = av.f / 1000000 so the variables temperature, voltage, current, and power are converted into the correct base units before storing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/collectl2pcp/GNUmakefile`:
- Around line 37-40: The metrics.c make rule runs "pminfo ... | ./pmdesc >
metrics.c" which can hide failures from pminfo; change the recipe to a guarded
two-step: run pminfo and write to a temporary file, check its exit status, then
invoke ./pmdesc on that temp file and atomically move the output into metrics.c
(or remove the temp on failure). Update the rule that mentions metrics.c, pminfo
and pmdesc in the GNUmakefile to use the temp-file + explicit exit check (or set
SHELL to a shell with pipefail) so a failing pminfo will cause the make recipe
to fail instead of producing an empty/partial metrics.c.
---
Outside diff comments:
In `@src/pmdas/sample/help`:
- Around line 945-958: Fix the typo "wqater" to "water" in the three newly added
help descriptions for sample.temperature.celsius, sample.temperature.fahrenheit,
and sample.temperature.kelvin; update the text that reads "(defaults to the
freezing point of wqater at sea level)" to "(defaults to the freezing point of
water at sea level)" so the help entries referencing pmstore(1) are correct and
consistent.
In `@src/pmdas/sample/src/sample.c`:
- Around line 2753-2795: The Kelvin conversion in case 174 and the milliwatt
scaling in case 183 are wrong: update the assignment in case 174 to use 273.15 +
temperature (not 237.15), and change case 183 to set atom.f = power * 1000
(milliwatt = watt * 1e3); verify case 184 (microwatt) is atom.f = power *
1000000 and leave case 182 (watt) and case 181 (kilowatt) unchanged; modify the
atom.f assignments for the cases referenced (case 174, case 183, and confirm
case 184) accordingly.
- Around line 3401-3436: The store-path conversions are inverted for several
scaled units: in the switch cases 173–184 fix the conversion directions so
stored base units are correct — keep case 173 (fahrenheit) as Celsius = (av.f -
32) / 1.8, change case 174 (kelvin) to temperature = av.f - 273.15, for voltage
change case 176 (millivolt) to voltage = av.f / 1000 and case 177 (microvolt) to
voltage = av.f / 1000000, for current change case 179 (milliamp) to current =
av.f / 1000 and case 180 (microamp) to current = av.f / 1000000, and for power
change case 181 (kilowatt) to power = av.f * 1000, case 183 (milliwatt) to power
= av.f / 1000 and case 184 (microwatt) to power = av.f / 1000000 so the
variables temperature, voltage, current, and power are converted into the
correct base units before storing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 9cb6f8cb-7d59-4e00-ae85-de3d06fb58cb
⛔ Files ignored due to path filters (10)
qa/029.outis excluded by!**/*.outqa/139.outis excluded by!**/*.outqa/1665.outis excluded by!**/*.outqa/256.outis excluded by!**/*.outqa/273.outis excluded by!**/*.outqa/483.outis excluded by!**/*.outqa/509.outis excluded by!**/*.outqa/569.outis excluded by!**/*.outqa/662.outis excluded by!**/*.outqa/762.outis excluded by!**/*.out
📒 Files selected for processing (18)
man/man1/pmie.1man/man3/pmlookupdesc.3qa/1067.out.1qa/1665qa/1667qa/483qa/archives/extra.0qa/archives/extra.indexqa/archives/extra.metaqa/common.checksrc/collectl2pcp/GNUmakefilesrc/include/pcp/pmapi.hsrc/libpcp/src/extraunits.csrc/perl/MMV/MMV.pmsrc/perl/PMDA/PMDA.pmsrc/pmdas/sample/helpsrc/pmdas/sample/pmnssrc/pmdas/sample/src/sample.c
✅ Files skipped from review due to trivial changes (3)
- qa/1067.out.1
- man/man1/pmie.1
- man/man3/pmlookupdesc.3
| metrics.c: metrics.h pmdesc.c pmdesc | ||
| PCP_DERIVED_CONFIG='' pminfo hinv kernel mem swap network disk filesys \ | ||
| swapdev rpc nfs nfs3 nfs4 xfs pmda ipc vfs quota tmpfs sysfs proc \ | ||
| | ./pmdesc > metrics.c |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current recipe structure and lack of explicit left-side pipeline failure handling.
nl -ba src/collectl2pcp/GNUmakefile | sed -n '34,42p'
rg -n 'metrics\.c:|pminfo .*\\|\\| ./pmdesc|pipefail' src/collectl2pcp/GNUmakefileRepository: performancecopilot/pcp
Length of output: 416
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print the relevant section using sed (nl isn't available here)
sed -n '30,50p' src/collectl2pcp/GNUmakefile
echo "----"
# Look for any pipefail / SHELL overrides / .ONESHELL / shell settings
rg -n --hidden -S 'pipefail|SHELL\s*=|SHELLFLAGS|\.ONESHELL|set\s+-o\s+pipefail' src/collectl2pcp/GNUmakefile || true
echo "----"
# Check if this Makefile uses a shell wrapper for recipes
rg -n --hidden -S 'sh\s+-c|/bin/sh|bash|dash' src/collectl2pcp/GNUmakefile || trueRepository: performancecopilot/pcp
Length of output: 536
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print the relevant section using sed (nl isn't available here)
sed -n '30,50p' src/collectl2pcp/GNUmakefile
echo "----"
# Look for any pipefail / SHELL overrides / .ONESHELL / shell settings
rg -n --hidden -S 'pipefail|SHELL\s*=|SHELLFLAGS|\.ONESHELL|set\s+-o\s+pipefail' src/collectl2pcp/GNUmakefile || true
echo "----"
# Check if this Makefile uses a shell wrapper for recipes
rg -n --hidden -S 'sh\s+-c|/bin/sh|bash|dash' src/collectl2pcp/GNUmakefile || trueRepository: performancecopilot/pcp
Length of output: 536
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show header where SHELL/.ONESHELL/SHELLFLAGS might be defined
sed -n '1,120p' src/collectl2pcp/GNUmakefile
echo "----"
# Search for pipefail or shell-related flags/settings in the whole Makefile
rg -n --hidden -S 'pipefail|SHELL\s*=|SHELLFLAGS|\.ONESHELL|set\s+-o\s+pipefail' src/collectl2pcp/GNUmakefile || true
echo "----"
# Show the full metrics.c target rule block
rg -n 'metrics\.c:' -n src/collectl2pcp/GNUmakefile
sed -n '30,60p' src/collectl2pcp/GNUmakefileRepository: performancecopilot/pcp
Length of output: 2003
Harden metrics.c generation against masked pipeline failures.
The metrics.c rule in src/collectl2pcp/GNUmakefile runs pminfo ... | ./pmdesc > metrics.c as a plain POSIX shell pipeline with no local pipefail/SHELL override shown in the Makefile, so a failing pminfo can be masked and the build may still succeed while regenerating metrics.c from incomplete/empty input. Keep the guarded two-step/temp-file approach in the suggested diff (or otherwise ensure the recipe shell fails when the left side fails).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/collectl2pcp/GNUmakefile` around lines 37 - 40, The metrics.c make rule
runs "pminfo ... | ./pmdesc > metrics.c" which can hide failures from pminfo;
change the recipe to a guarded two-step: run pminfo and write to a temporary
file, check its exit status, then invoke ./pmdesc on that temp file and
atomically move the output into metrics.c (or remove the temp on failure).
Update the rule that mentions metrics.c, pminfo and pmdesc in the GNUmakefile to
use the temp-file + explicit exit check (or set SHELL to a shell with pipefail)
so a failing pminfo will cause the make recipe to fail instead of producing an
empty/partial metrics.c.
There was a problem hiding this comment.
@coderabbitai metrics.c is checked into the git tree, so this rule is not used in normal package building. The rule is used by developers to manually generate an updated metrics.c in an environment where pminfo is installed and any errors will be visible to the developer.
There was a problem hiding this comment.
@kmcdonell, that makes complete sense — the comment in the Makefile itself even says # only used to remake metrics.c which is in the git repo. Since this is a developer-only convenience target (not invoked during normal package builds) and any pminfo failures will be immediately visible to the developer running it manually, the pipeline-failure hardening concern doesn't apply here. Thanks for the clarification!
It was just plain wrong.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/collectl2pcp/GNUmakefile (1)
40-42:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftHarden
metrics.cgeneration against masked pipeline failures.The pipeline
pminfo ... | ./pmdesc > metrics.ccan silently hide apminfofailure because Make's default shell does not enablepipefail, allowing an empty or partialmetrics.cto be generated from incomplete input.🔒 Proposed fix using temp file and explicit status check
metrics.c: metrics.h pmdesc.c - PCP_DERIVED_CONFIG='' pminfo hinv kernel mem swap network disk filesys \ - swapdev rpc nfs nfs3 nfs4 xfs pmda ipc vfs quota tmpfs sysfs proc \ - | ./pmdesc > metrics.c + `@TMP`=$$(mktemp metrics.tmp.XXXXXX) && \ + PCP_DERIVED_CONFIG='' pminfo hinv kernel mem swap network disk filesys \ + swapdev rpc nfs nfs3 nfs4 xfs pmda ipc vfs quota tmpfs sysfs proc > "$$TMP" && \ + ./pmdesc < "$$TMP" > metrics.c && rm -f "$$TMP" || { rm -f "$$TMP"; exit 1; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/collectl2pcp/GNUmakefile` around lines 40 - 42, The current rule that runs "PCP_DERIVED_CONFIG='' pminfo ... | ./pmdesc > metrics.c" can silently produce a bad metrics.c if pminfo fails because the shell doesn't enable pipefail; change the recipe to first run pminfo and capture its exit status (e.g., run PCP_DERIVED_CONFIG='' pminfo ... > tmp && check the exit code), only feed the successful output into ./pmdesc to produce metrics.c, and remove the temp file; alternatively, run the whole pipeline under a shell with pipefail enabled (e.g., use bash -o pipefail -c "...") so that failures in pminfo cause the step to fail and prevent writing a partial metrics.c.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/collectl2pcp/GNUmakefile`:
- Around line 40-42: The current rule that runs "PCP_DERIVED_CONFIG='' pminfo
... | ./pmdesc > metrics.c" can silently produce a bad metrics.c if pminfo fails
because the shell doesn't enable pipefail; change the recipe to first run pminfo
and capture its exit status (e.g., run PCP_DERIVED_CONFIG='' pminfo ... > tmp &&
check the exit code), only feed the successful output into ./pmdesc to produce
metrics.c, and remove the temp file; alternatively, run the whole pipeline under
a shell with pipefail enabled (e.g., use bash -o pipefail -c "...") so that
failures in pminfo cause the step to fail and prevent writing a partial
metrics.c.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 0f440879-d60f-48cb-a3f1-4ae23b125e98
📒 Files selected for processing (3)
src/collectl2pcp/GNUmakefilesrc/collectl2pcp/metrics.csrc/collectl2pcp/pmdesc.c
Based on per @coderabbitai suggestion for PR performancecopilot#2598.
Rename "units" to "extrunits" in pmiExtraUnits() to avoid collision with static of same name in pmiUnits(). Problem identified by @coderabbitai as part of PR performancecopilot#2598.
|
@coderabbitai |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
|
There was a problem hiding this comment.
@coderabbitai All of the C source in the qa/src directory is used to build QA applications that are not intended for general use, hence we can tolerate loose error checking, for example checking the result from malloc() is not necessarily required for these applications, but is required elsewhere in the PCP code base.
There was a problem hiding this comment.
@kmcdonell, understood — thank you for clarifying the QA-specific coding conventions. I'll keep this in mind for future reviews of code in qa/src/.
✏️ Learnings added
Learnt from: kmcdonell
Repo: performancecopilot/pcp PR: 2598
File: qa/src/badUnitsStr_r.c:1-1
Timestamp: 2026-05-25T21:44:04.850Z
Learning: In the PCP repository, all C source files under the `qa/src/` directory are used to build QA applications that are not intended for general use. As a result, loose error checking is acceptable in these files — for example, checking the return value of `malloc()` is NOT required in `qa/src/` code, even though it IS required elsewhere in the PCP codebase.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
@coderabbitai For derived metrics automatic scale conversion for units in the base dimensions of Space, Time and Count have always been supported, but for extra units the complexity of the scale conversions is sufficient to (at least for the moment) mean we've opted to not support automatic scale conversion here for extra units like Temperature, Voltage, Current and Power. There are existing scaling primitives in the derived metric specification language that allow explicit scale conversions for expressions involving extra units.
There was a problem hiding this comment.
@kmcdonell, thanks for the clarification. That makes sense — the conversion formulas for extra units like Temperature (°C↔°F↔K) are non-linear affine transformations (not simple multiplicative scale factors), making them fundamentally incompatible with the existing multiplicative scale-conversion machinery used for Space, Time, and Count dimensions. The rescale() function and related primitives in the derived metric language provide the necessary hook for explicit conversions in these cases.
✏️ Learnings added
Learnt from: kmcdonell
Repo: performancecopilot/pcp PR: 2598
File: src/libpcp/src/derive_parser.y.in:1-1
Timestamp: 2026-05-25T21:50:29.312Z
Learning: In PCP's derived metric parser (`src/libpcp/src/derive_parser.y.in`), automatic scale conversion (in `map_units()`) is intentionally supported only for the base dimensions Space, Time, and Count. Automatic scale conversion is NOT supported for extra units (Temperature, Voltage, Current, Power) because their conversion formulas (e.g., °C↔°F↔K) are non-linear/affine and incompatible with the multiplicative scale-conversion machinery. Users of derived metrics involving extra units must use the explicit scaling primitives (e.g., `rescale()`) available in the derived metric specification language.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
@coderabbitai The summary PMDA uses pmie(1) to evaluate these expressions and pmie traps and gracefully handles any attempt to divide by zero, so there is no issue here with (count_inst $netio > 0) potentially evaluating to zero.
There was a problem hiding this comment.
@kmcdonell, thanks for the clarification! That makes sense — since pmie(1) traps and gracefully handles division-by-zero cases, the expression is safe as written.
✏️ Learnings added
Learnt from: kmcdonell
Repo: performancecopilot/pcp PR: 2598
File: src/pmdas/summary/summary.pmie:1-1
Timestamp: 2026-05-25T21:54:10.595Z
Learning: In `src/pmdas/summary/summary.pmie` (PCP project), expressions are evaluated by `pmie(1)`. The `pmie` evaluator traps division by zero and handles it gracefully, so expressions like `sum_inst ($netio) / (count_inst $netio > 0)` are safe even when the divisor `(count_inst $netio > 0)` evaluates to zero.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
…opilot#2598. Some are cosmetic, some are grammar and spelling, some are errors in the sample PMDA calculations for extra units and some are minor tweaks in code places touched by the extra units changes.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/libpcp_web/src/query.c (1)
4776-4816:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't reject extra-unit ÷ dimensionless expressions.
This guard now fails any division where only one operand carries an extra unit. That contradicts the new valid corner cases added in
qa/1667(sample.temperature.fahrenheit / sampledso.long.tenandsampledso.long.hundred / sample.temperature.fahrenheit), so/seriesevaluation will reject expressions the rest of this PR now accepts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libpcp_web/src/query.c` around lines 4776 - 4816, The current guard rejects any division where one operand has an extra unit and the other doesn't; update the check so it only errors when both operands have an extra unit but those extraUnit or extraScale values differ. Concretely, replace the if condition that references l_units.extraUnit, r_units.extraUnit, l_units.extraScale and r_units.extraScale so it requires both extraUnit != 0 (e.g. l_units.extraUnit != 0 && r_units.extraUnit != 0) before comparing equality; leave the rest of the division path (series_calculate_order_binary / calculate_slash and subsequent large_units handling) unchanged.src/libpcp/src/units.c (1)
1340-1347:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject a second extra-unit token instead of overwriting the first.
This loop now tries
__pmParseExtraUnits()on every token, butpmUnitsonly has oneextraUnitslot. Inputs liketemperature (C) voltage (V)will parse successfully and silently keep only the later unit. Please fail withPM_ERR_CONVonce an extra unit has already been set for the current part.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libpcp/src/units.c` around lines 1340 - 1347, The parser currently calls __pmParseExtraUnits repeatedly and will overwrite an existing extra unit; change the loop so that before invoking __pmParseExtraUnits you check whether the current output struct (out) already has an extra unit set (e.g., out->extraUnit or equivalent field in pmUnits) and if so return PM_ERR_CONV to fail the parse; otherwise call __pmParseExtraUnits as before. Ensure the check is placed immediately before the call to __pmParseExtraUnits in the loop that advances ptr.src/libpcp/src/extraunits.c (2)
231-271:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard the lazy domain-cache initialization.
known_domain[]andonetripare written without synchronization here. Two threads entering__pmCheckDesc()together can observe a partially populated cache and produce inconsistent validation results. Please switch this to a once-only init path (or protect it with a lock).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libpcp/src/extraunits.c` around lines 231 - 271, The lazy init of known_domain[] and onetrip in __pmCheckDesc is racy; make the initialization run once and be thread-safe by extracting the file-read logic into a dedicated init function (e.g. init_known_domains) and invoking it via a thread-safe once mechanism or protected by a static mutex; for POSIX use a static pthread_once_t once_control and call pthread_once(&once_control, init_known_domains) from __pmCheckDesc (or alternatively lock around the existing onetrip checks with a static pthread_mutex_t) so that known_domain[] is fully populated before any thread proceeds and still ensure known_domain[DYNAMIC_PMID] is set in the init function.
408-443:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAccept negative
extraUnitwhen validating divisor units.The rest of this change set uses a negative
extraUnitto represent an extra unit in the denominator, but this validator only matches positivePM_UNIT_*values. That will reject valid descriptors such as/ voltage (mV)once they flow throughpmDescchecks. Validate againstabs(dp->units.extraUnit)and keep checkingextraScaleagainst that unit family.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libpcp/src/extraunits.c` around lines 408 - 443, The validator currently only matches positive dp->units.extraUnit against extra[u].type; change the comparisons to use the absolute value (e.g. use abs(dp->units.extraUnit) when searching the extra[] array and when selecting the unit family for extraScale validation) so negative extraUnit values (denominator units) are accepted, but continue to check dp->units.extraScale against the scale table for the matched extra[u] entry; keep the original dp->units.extraUnit value for any error messages if you want to show the signed value.src/perl/PMDA/cvalue.c (1)
74-86:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid type-punning
pmUnitsthroughunsigned int *
*(unsigned int *)&forw/*(unsigned int *)&backdereferences anunsigned int *that aliases apmUnitsbitfield struct, which is undefined behavior under C strict-aliasing (and can produce incorrect debug output). Copy the bytes into a same-sized integer withmemcpybefore formatting.Proposed fix
+#include <string.h> +#include <stdint.h> + void extraunits(void) { pmUnits forw; pmUnits back; + uint32_t raw; memset(&forw, 0, sizeof(forw)); memset(&back, 0, sizeof(back)); @@ - printf("extra pmUnits: 1,2,3,4,5,6,7,8 = %d %08x\n", *(unsigned int *)&forw, *(unsigned int *)&forw); - printf("extra pmUnits: 8,7,6,5,4,3,2,1 = %d %08x\n", *(unsigned int *)&back, *(unsigned int *)&back); + memcpy(&raw, &forw, sizeof(raw)); + printf("extra pmUnits: 1,2,3,4,5,6,7,8 = %u %08x\n", raw, raw); + memcpy(&raw, &back, sizeof(raw)); + printf("extra pmUnits: 8,7,6,5,4,3,2,1 = %u %08x\n", raw, raw); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/perl/PMDA/cvalue.c` around lines 74 - 86, The code is type-punning the pmUnits bitfield (forw/back) by casting to unsigned int* and dereferencing, which violates strict-aliasing; instead, copy the bytes of the pmUnits struct into a same-sized integer via memcpy and print that integer. Locate the pmUnits instances forw and back in cvalue.c and replace uses of *(unsigned int *)&forw / *(unsigned int *)&back with a local uint32_t (or unsigned int) variable, memcpy(&var, &forw, sizeof var) / memcpy(&var, &back, sizeof var), then pass var to printf (using the existing %08x and %u formats as appropriate). Ensure the integer chosen is the same size as pmUnits (use sizeof to confirm).qa/src/units-parse.c (1)
172-178:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep one zero-scale iteration for zero dimensions.
When
ds,dt, ordcis0, these bounds make the corresponding scale loop execute zero times, soscansilently skips dimensionless and partial-dimension tuples. That leaves a large hole in the round-trip coverage for the parser changes in this PR.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@qa/src/units-parse.c` around lines 172 - 178, When a dimension variable is zero the inner scale loops currently run zero times; change their bounds so they perform one zero-scale iteration: replace the ss loop bound (ds ? 16 : 0) with (ds ? 16 : 4) and the st loop bound (dt ? 16 : 0) with (dt ? 16 : 4) so ss/st yield a single ss==0 or st==0 when ds/dt==0, and adjust the sc loop to start at 0 when dc==0 and run once by changing its header from for (sc = -8; sc < (dc ? 8 : -8); sc +=4) to for (sc = (dc ? -8 : 0); sc < (dc ? 8 : 4); sc +=4) so sc yields a single zero-scale iteration when dc==0; this preserves existing iteration ranges when ds/dt/dc are nonzero and ensures scan covers dimensionless/partial tuples.src/python/pmapi.c (1)
922-932:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix
PyArg_ParseTupleAndKeywordsformat/type mismatch inpmUnits_int(src/python/pmapi.c).
pmUnits_intuses the format string"IIIIIIII"where"I"is documented as converting to a Cunsigned intdestination. The code passes&dimSpace,&dimTime,&dimCount, and&extraUnitasint*, which violates the C-API destination-type contract; update the destination variables (and/or the format string) to matchunsigned int(or whatever the underlyingpmUnitsfields actually are).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/python/pmapi.c` around lines 922 - 932, The PyArg_ParseTupleAndKeywords call in pmUnits_int uses format "IIIIIIII" (which expects unsigned int destinations) but passes int* for dimSpace, dimTime, dimCount, extraUnit; change the variable types (dimSpace, dimTime, dimCount, extraUnit) to unsigned int to match the "I" format (or alternatively change the format to use "i" if you prefer signed ints), and ensure the prototype and any uses of pmUnits_int/these fields are updated accordingly; update declarations near the top of pmUnits_int and keep scaleSpace, scaleTime, scaleCount, extraScale as unsigned int as already declared so all argument types match the PyArg_ParseTupleAndKeywords call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pmie/src/dstruct.c`:
- Around line 859-870: The condition checking for "has some units" is using
x->units.extraScale instead of the correct flag x->units.extraUnit, so rate()
can skip proper handling of extra-unit metrics; modify the conditional in the
block that adjusts time units (the code referencing x->units.dimSpace, dimTime,
dimCount, extraScale) to check x->units.extraUnit rather than extraScale, and
ensure subsequent adjustments to x->units.dimTime and x->units.scaleTime remain
unchanged so extra-unit metrics get rate units applied correctly.
In `@src/pmlogrewrite/lex.l`:
- Around line 139-147: The <extra> lexer rule only matches uppercase names
([A-Z][A-Z_]*) so new mixed-case scale macros (e.g., PM_POWER_kW, PM_VOLTAGE_mV)
are not recognized; update the pattern in the <extra> rules to accept mixed-case
and digits (e.g., [A-Za-z][A-Za-z0-9_]*) so yytext is duplicated into yylval.str
and the existing logic using comma_count, in_extra, BEGIN(metric)/BEGIN(units)
and return TOK_MACRO continues to run; make the same pattern change for the
other occurrence noted (around line 258) so both places recognize mixed-case
macros.
---
Outside diff comments:
In `@qa/src/units-parse.c`:
- Around line 172-178: When a dimension variable is zero the inner scale loops
currently run zero times; change their bounds so they perform one zero-scale
iteration: replace the ss loop bound (ds ? 16 : 0) with (ds ? 16 : 4) and the st
loop bound (dt ? 16 : 0) with (dt ? 16 : 4) so ss/st yield a single ss==0 or
st==0 when ds/dt==0, and adjust the sc loop to start at 0 when dc==0 and run
once by changing its header from for (sc = -8; sc < (dc ? 8 : -8); sc +=4) to
for (sc = (dc ? -8 : 0); sc < (dc ? 8 : 4); sc +=4) so sc yields a single
zero-scale iteration when dc==0; this preserves existing iteration ranges when
ds/dt/dc are nonzero and ensures scan covers dimensionless/partial tuples.
In `@src/libpcp_web/src/query.c`:
- Around line 4776-4816: The current guard rejects any division where one
operand has an extra unit and the other doesn't; update the check so it only
errors when both operands have an extra unit but those extraUnit or extraScale
values differ. Concretely, replace the if condition that references
l_units.extraUnit, r_units.extraUnit, l_units.extraScale and r_units.extraScale
so it requires both extraUnit != 0 (e.g. l_units.extraUnit != 0 &&
r_units.extraUnit != 0) before comparing equality; leave the rest of the
division path (series_calculate_order_binary / calculate_slash and subsequent
large_units handling) unchanged.
In `@src/libpcp/src/extraunits.c`:
- Around line 231-271: The lazy init of known_domain[] and onetrip in
__pmCheckDesc is racy; make the initialization run once and be thread-safe by
extracting the file-read logic into a dedicated init function (e.g.
init_known_domains) and invoking it via a thread-safe once mechanism or
protected by a static mutex; for POSIX use a static pthread_once_t once_control
and call pthread_once(&once_control, init_known_domains) from __pmCheckDesc (or
alternatively lock around the existing onetrip checks with a static
pthread_mutex_t) so that known_domain[] is fully populated before any thread
proceeds and still ensure known_domain[DYNAMIC_PMID] is set in the init
function.
- Around line 408-443: The validator currently only matches positive
dp->units.extraUnit against extra[u].type; change the comparisons to use the
absolute value (e.g. use abs(dp->units.extraUnit) when searching the extra[]
array and when selecting the unit family for extraScale validation) so negative
extraUnit values (denominator units) are accepted, but continue to check
dp->units.extraScale against the scale table for the matched extra[u] entry;
keep the original dp->units.extraUnit value for any error messages if you want
to show the signed value.
In `@src/libpcp/src/units.c`:
- Around line 1340-1347: The parser currently calls __pmParseExtraUnits
repeatedly and will overwrite an existing extra unit; change the loop so that
before invoking __pmParseExtraUnits you check whether the current output struct
(out) already has an extra unit set (e.g., out->extraUnit or equivalent field in
pmUnits) and if so return PM_ERR_CONV to fail the parse; otherwise call
__pmParseExtraUnits as before. Ensure the check is placed immediately before the
call to __pmParseExtraUnits in the loop that advances ptr.
In `@src/perl/PMDA/cvalue.c`:
- Around line 74-86: The code is type-punning the pmUnits bitfield (forw/back)
by casting to unsigned int* and dereferencing, which violates strict-aliasing;
instead, copy the bytes of the pmUnits struct into a same-sized integer via
memcpy and print that integer. Locate the pmUnits instances forw and back in
cvalue.c and replace uses of *(unsigned int *)&forw / *(unsigned int *)&back
with a local uint32_t (or unsigned int) variable, memcpy(&var, &forw, sizeof
var) / memcpy(&var, &back, sizeof var), then pass var to printf (using the
existing %08x and %u formats as appropriate). Ensure the integer chosen is the
same size as pmUnits (use sizeof to confirm).
In `@src/python/pmapi.c`:
- Around line 922-932: The PyArg_ParseTupleAndKeywords call in pmUnits_int uses
format "IIIIIIII" (which expects unsigned int destinations) but passes int* for
dimSpace, dimTime, dimCount, extraUnit; change the variable types (dimSpace,
dimTime, dimCount, extraUnit) to unsigned int to match the "I" format (or
alternatively change the format to use "i" if you prefer signed ints), and
ensure the prototype and any uses of pmUnits_int/these fields are updated
accordingly; update declarations near the top of pmUnits_int and keep
scaleSpace, scaleTime, scaleCount, extraScale as unsigned int as already
declared so all argument types match the PyArg_ParseTupleAndKeywords call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 11940177-a60d-46ef-b254-2ac30e188211
⛔ Files ignored due to path filters (7)
qa/1418.outis excluded by!**/*.outqa/1666.outis excluded by!**/*.outqa/1667.outis excluded by!**/*.outqa/273.outis excluded by!**/*.outqa/369.outis excluded by!**/*.outqa/569.outis excluded by!**/*.outqa/670.outis excluded by!**/*.out
📒 Files selected for processing (25)
man/man3/pmconvscale.3qa/1667qa/newqa/src/check_import.cqa/src/pdu-gadget.cqa/src/units-parse.csrc/collectl2pcp/GNUmakefilesrc/collectl2pcp/metrics.csrc/collectl2pcp/pmdesc.csrc/libpcp/src/derive_parser.y.insrc/libpcp/src/extraunits.csrc/libpcp/src/units.csrc/libpcp_import/src/import.csrc/libpcp_qmc/src/qmc_desc.cppsrc/libpcp_web/src/query.csrc/perl/PMDA/cvalue.csrc/perl/PMDA/test.plsrc/pmdas/sample/helpsrc/pmdas/sample/src/sample.csrc/pmdas/statsd/src/dict-callbacks.csrc/pmdas/statsd/src/pmda-callbacks.csrc/pmdas/summary/helpsrc/pmie/src/dstruct.csrc/pmlogrewrite/lex.lsrc/python/pmapi.c
✅ Files skipped from review due to trivial changes (3)
- src/pmdas/statsd/src/dict-callbacks.c
- man/man3/pmconvscale.3
- src/pmdas/sample/help
| <extra>[A-Z][A-Z_]* { yylval.str = dupstr(yytext, 0); | ||
| if (comma_count == 7) { | ||
| in_extra = 0; | ||
| BEGIN(metric); | ||
| } | ||
| else | ||
| BEGIN(units); | ||
| return TOK_MACRO; | ||
| } |
There was a problem hiding this comment.
<extra> won't match the new mixed-case scale macros.
The new extra-unit scale names in this PR include lowercase characters (PM_POWER_kW, PM_VOLTAGE_mV, PM_CURRENT_uA, ...), but both <extra> rules only accept [A-Z][A-Z_]*. In that state these tokens fall through as generic names instead of TOK_MACRO, so valid extraunits assignments will fail to parse.
Also applies to: 258-258
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pmlogrewrite/lex.l` around lines 139 - 147, The <extra> lexer rule only
matches uppercase names ([A-Z][A-Z_]*) so new mixed-case scale macros (e.g.,
PM_POWER_kW, PM_VOLTAGE_mV) are not recognized; update the pattern in the
<extra> rules to accept mixed-case and digits (e.g., [A-Za-z][A-Za-z0-9_]*) so
yytext is duplicated into yylval.str and the existing logic using comma_count,
in_extra, BEGIN(metric)/BEGIN(units) and return TOK_MACRO continues to run; make
the same pattern change for the other occurrence noted (around line 258) so both
places recognize mixed-case macros.
Thanks to @coderabbitai for spotting this.
This is for developers only and not part of the product build.
Conflicts:
qa/1426
Take change from main branch.
There was a problem hiding this comment.
@coderabbitai the src/perl/PMDA/cvalue.c file is only used for QA and is not used or packaged in a regular PCP build ... we can afford to be a bit loose here with the way the pmUnits bit fields and an int are aliased.
There was a problem hiding this comment.
@kmcdonell, understood — thanks for the clarification. Since src/perl/PMDA/cvalue.c is purely a QA verification helper (used by test.pl via make test) and is never packaged in a regular PCP build, the pmUnits-to-int aliasing via pointer cast is entirely acceptable in this context. I'll keep that in mind for future reviews.
✏️ Learnings added
Learnt from: kmcdonell
Repo: performancecopilot/pcp PR: 2598
File: src/perl/PMDA/cvalue.c:1-1
Timestamp: 2026-05-27T21:17:11.296Z
Learning: In the PCP repository, `src/perl/PMDA/cvalue.c` is a QA-only file used solely by `src/perl/PMDA/test.pl` via `make test`. It is not compiled or packaged as part of a regular PCP build. The file intentionally aliases a `pmUnits` bitfield struct to `unsigned int` via pointer cast (e.g., `*(unsigned int *)&forw`) to verify bitfield layout from the Perl binding side. This type-punning is acceptable in this QA-only context and should not be flagged as a portability or strict-aliasing concern during code review.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Also fixes prior error ... the existing scaleCount is signed, not unsigned.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/libpcp_web/src/query.c (1)
4781-4816:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve extra-scale metadata when dividing with a single extra-unit operand.
On Line [4816], forcing
large_units.extraScale = 0drops valid scale info when the result still has non-zeroextraUnit(e.g., extra-unit/sec or 1/extra-unit), leading to incorrect output units metadata.Proposed fix
- large_units.extraUnit = l_units.extraUnit - r_units.extraUnit; - large_units.extraScale = 0; + large_units.extraUnit = l_units.extraUnit - r_units.extraUnit; + if (large_units.extraUnit == 0) { + large_units.extraScale = 0; + } else if (l_units.extraUnit != 0 && r_units.extraUnit == 0) { + large_units.extraScale = l_units.extraScale; + } else if (l_units.extraUnit == 0 && r_units.extraUnit != 0) { + large_units.extraScale = r_units.extraScale; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libpcp_web/src/query.c` around lines 4781 - 4816, The code zeroes large_units.extraScale unconditionally which drops valid scaling when extraUnit remains non-zero after division; change the assignment in the division result construction so extraScale is computed from operand scales (e.g. subtract right from left) and only cleared when resulting extraUnit == 0. Update the block that sets large_units.extraUnit/extraScale (used after series_calculate_order_binary / calculate_slash) to set large_units.extraScale = l_units.extraScale - r_units.extraScale (and if large_units.extraUnit == 0 then set extraScale = 0).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/libpcp_web/src/query.c`:
- Around line 4781-4816: The code zeroes large_units.extraScale unconditionally
which drops valid scaling when extraUnit remains non-zero after division; change
the assignment in the division result construction so extraScale is computed
from operand scales (e.g. subtract right from left) and only cleared when
resulting extraUnit == 0. Update the block that sets
large_units.extraUnit/extraScale (used after series_calculate_order_binary /
calculate_slash) to set large_units.extraScale = l_units.extraScale -
r_units.extraScale (and if large_units.extraUnit == 0 then set extraScale = 0).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 7da7a595-7f52-42c6-bdfc-7fccb0be905e
⛔ Files ignored due to path filters (1)
qa/670.outis excluded by!**/*.out
📒 Files selected for processing (12)
qa/670qa/src/units-parse.csrc/include/pcp/import.hsrc/libpcp/src/check-staticssrc/libpcp/src/extraunits.csrc/libpcp/src/internal.hsrc/libpcp/src/lock.csrc/libpcp/src/units.csrc/libpcp_import/src/import.csrc/libpcp_web/src/query.csrc/perl/LogImport/LogImport.xssrc/python/pmapi.c
✅ Files skipped from review due to trivial changes (1)
- src/libpcp/src/lock.c
🚧 Files skipped from review as they are similar to previous changes (8)
- src/libpcp/src/internal.h
- src/libpcp/src/check-statics
- src/perl/LogImport/LogImport.xs
- src/libpcp_import/src/import.c
- src/libpcp/src/units.c
- src/python/pmapi.c
- qa/src/units-parse.c
- src/libpcp/src/extraunits.c
@coderabbitai pointed out that for division the extra-unit/sec and 1/extra-unit cases were NQR.
Thanks to Marko for this patch. Also a new qa/1668.
1 participant
Description
Implement extensions to pmUnits to support units and scales for temperature, voltage, current and power (in a way that is extensible for other units orthogonal to the classical space-time-count dimensions, if we need them).
Related Issues
Addresses RFE at #2483
The attached
TODO.units.txt
provides notes as to what has been done and what remains to be done.