Minor pcp-atop updates#2653
Conversation
Add rawwrite_init_labels() which writes an {"atop": true} creator
label then copies all context-level labels from the live PMAPI context
(hostname, domain, machineid, etc.) into the PMI archive via
pmiPutLabel.
Updated qa/1089 to verify atop creator label in recorded archive.
Fixed pmiPutLabel to pass through already-valid JSON values (such
as quoted strings, numbers, true/false/null) verbatim rather than
double-quoting them. Previously numbers became strings and quoted
strings got escaped quotes.
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
The glob "$base".[0-9]* matched already-compressed .N.zst files (e.g., .0.zst matches because [0-9]* covers "0.zst"). These were renamed to .0.zst.tmp and re-compressed to .0.zst.zst. Skip files with .zst or .xz suffixes in the rename loop so only raw uncompressed volumes are staged for compression. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR expands PMI label encoding, adds raw-write archive label initialization, standardizes background compression, skips already-compressed volumes, and validates the Changespcp-atop archive updates
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/pcp/atop/various.c (3)
1925-1957: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUnchecked context-switch return values.
pmiUseContext,pmUseContext,pmWhichContext, andpmGetContextLabelsreturn values are not checked. If a context switch silently fails partway through, labels may be dropped without any diagnostic, and it becomes hard to tell copied-label failures apart from "no labels present". Consider checkingsaved >= 0before restoring, and logging (e.g. viapmDebugOptions.appl1) whenpmiUseContext/pmUseContextreturn an error.🤖 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/pcp/atop/various.c` around lines 1925 - 1957, Validate the return values of pmWhichContext, pmiUseContext, pmUseContext, and pmGetContextLabels in the label-copying flow. Log context-switch failures through the existing pmDebugOptions.appl1 mechanism, only restore contexts when saved is valid, and distinguish retrieval errors from the legitimate zero-label case before freeing label sets.
1760-1787: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSilent failure paths in
run_background.fork()/execv() failures (outer fork returning -1, inner fork returning -1, or execv failing) all fall through to
_exit(0)/ a plain return with no diagnostic. Background compression can silently stop working with no trace in logs.🛠️ Suggested diagnostics
pid = fork(); + if (pid < 0) + { + fprintf(stderr, "%s: run_background: fork failed: %s\n", + pmGetProgname(), osstrerror()); + return; + } if (pid == 0) { - if (fork() == 0) + pid_t gc = fork(); + if (gc == 0) { alarm(0); execv(abspath, argv); + fprintf(stderr, "%s: run_background: execv(%s) failed: %s\n", + pmGetProgname(), abspath, osstrerror()); } _exit(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/pcp/atop/various.c` around lines 1760 - 1787, Update run_background to report failures from the outer fork, inner fork, and execv instead of silently exiting or returning. Add diagnostics for each failed operation, ensure the child exits with an appropriate failure status after execv fails, and preserve the existing successful background execution and wait behavior.
1794-1802: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHardcoded
/usr/bin/zstdpath may not exist on all supported platforms.
execvrequires an absolute path and performs no PATH lookup; ifzstdis installed elsewhere on a given distro/platform, compression silently fails (compounded by the silent-failure issue inrun_background). Consider resolving the path via PATH search (e.g.execvp-style) or a build/config-time detected location, similar to howatop-daily's path is resolved viapmGetConfig("PCP_BINADM_DIR")elsewhere in this file.🤖 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/pcp/atop/various.c` around lines 1794 - 1802, Update rawarchive_compress_volume to avoid hardcoding /usr/bin/zstd; resolve the zstd executable through the configured PCP binary directory or an execvp-style PATH lookup before calling run_background. Preserve the existing compression arguments and ensure the resolved executable is used on platforms where zstd is installed outside /usr/bin.src/libpcp_import/src/import.c (1)
1178-1180: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPlain-string branch doesn't escape special characters.
Line 1180 wraps
value(andname) in quotes without escaping embedded",\, or control characters. If either contains a double quote (e.g. a value copied from an arbitrary context label), the resulting JSON fragment is malformed, and__pmAddLabelswill most likely reject it. This path is now exercised more sincerawwrite_init_labels()(various.c) copies arbitrary host context label values throughpmiPutLabel.🤖 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 1178 - 1180, The plain-string branch in the label JSON construction must escape JSON-special characters in both name and value before wrapping them in quotes. Update the code around pmsprintf to use the existing JSON-escaping utility if available, or otherwise add equivalent escaping for quotes, backslashes, and control characters, while preserving the unquoted value branch.
🤖 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/pcp/atop/various.c`:
- Around line 1922-1949: In the label reconstruction loop within the pmi context
setup, validate lp->namelen and lp->valuelen before formatting into nbuf and
vbuf. Skip the label when either length cannot fit its buffer, and emit a debug
warning identifying the rejected label; do not pass truncated strings to
pmiPutLabel.
---
Nitpick comments:
In `@src/libpcp_import/src/import.c`:
- Around line 1178-1180: The plain-string branch in the label JSON construction
must escape JSON-special characters in both name and value before wrapping them
in quotes. Update the code around pmsprintf to use the existing JSON-escaping
utility if available, or otherwise add equivalent escaping for quotes,
backslashes, and control characters, while preserving the unquoted value branch.
In `@src/pcp/atop/various.c`:
- Around line 1925-1957: Validate the return values of pmWhichContext,
pmiUseContext, pmUseContext, and pmGetContextLabels in the label-copying flow.
Log context-switch failures through the existing pmDebugOptions.appl1 mechanism,
only restore contexts when saved is valid, and distinguish retrieval errors from
the legitimate zero-label case before freeing label sets.
- Around line 1760-1787: Update run_background to report failures from the outer
fork, inner fork, and execv instead of silently exiting or returning. Add
diagnostics for each failed operation, ensure the child exits with an
appropriate failure status after execv fails, and preserve the existing
successful background execution and wait behavior.
- Around line 1794-1802: Update rawarchive_compress_volume to avoid hardcoding
/usr/bin/zstd; resolve the zstd executable through the configured PCP binary
directory or an execvp-style PATH lookup before calling run_background. Preserve
the existing compression arguments and ensure the resolved executable is used on
platforms where zstd is installed outside /usr/bin.
🪄 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: ddca2357-6e25-4f9e-bbe5-dc52b9b83f6f
⛔ Files ignored due to path filters (1)
qa/1089.outis excluded by!**/*.out
📒 Files selected for processing (6)
qa/1089src/libpcp_import/src/import.csrc/pcp/atop/atop-daily.shsrc/pcp/atop/atop.csrc/pcp/atop/atop.hsrc/pcp/atop/various.c
If a label name or value exceeds the local buffer size, pmsprintf would truncate it — potentially stripping a closing quote from a JSON string value and producing malformed JSON in the archive. Skip such labels rather than writing corrupt data. Addresses coderabbit finding on PR performancecopilot#2653. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
1 participant
Add archive labels from the context, fix double compression of daily logs.