Skip to content

helper: accurate failure diagnostics + coherent library version#17

Merged
fxd0h merged 2 commits into
mainfrom
diag/helper-perror-version
Jun 26, 2026
Merged

helper: accurate failure diagnostics + coherent library version#17
fxd0h merged 2 commits into
mainfrom
diag/helper-perror-version

Conversation

@fxd0h

@fxd0h fxd0h commented Jun 26, 2026

Copy link
Copy Markdown
Owner

Addresses the two code-rigor findings in #5 from the RustDesk maintainer review.

Misleading diagnostics

The helper printed perror("cap_set_proc failed") on six error paths, four of which have nothing to do with capabilities. In a setcap binary a wrong message actively misleads an operator during incident response (a seccomp or socket failure reporting a capability failure). Each path now gets an accurate message, chosen by whether the failing call actually sets errno:

path was now
cap_init cap_set_proc failed perror(cap_init failed)
cap_set_flag cap_set_proc failed perror(cap_set_flag failed)
seccomp_init cap_set_proc failed fprintf(seccomp_init failed) (does not set errno, so no perror)
seccomp_rule_add cap_set_proc failed fprintf(... strerror(-rc)) (returns -errno, does not set errno)
send cap_set_proc failed perror(send failed)
send_error cap_set_proc failed dropped (the reason is already logged; errno not meaningfully set here)

On the two cap paths, errno is now saved before cap_free() and restored before perror(), since the cleanup call may clobber it (would have reintroduced the same class of misleading message). The genuine cap_set_proc and seccomp_load failure paths now log instead of returning silently.

Coherent version

drmtap_version() returned 0.1.0 while the project is well past that, which reads as unmaintained. Set DRMTAP_VERSION_* and the meson project version to 0.4.3, matching the currently published libdrmtap-sys crate (same C sources). The .so soversion stays 0 (ABI unchanged); only the full version string moves. The version tests now derive their expected value instead of hardcoding it:

  • C tests (test_helper, test_enumerate): from DRMTAP_VERSION_*.
  • Rust sys test: from CARGO_PKG_VERSION, which also cross-checks that the C header and the crate version stay equal (fails loudly if a future release bumps one without the other).

Validation (local, x86_64 i915)

  • release+werror, debug+asan+ubsan+werror, buildtype=plain+werror (FORTIFY gate): all clean
  • meson test: 5/5 (unit + integration)
  • cppcheck (warning,portability): clean
  • cargo build/test: clean (sys + wrapper)
  • real V3 capture through the rebuilt helper: 3840x2160, EGL detile ret=0, happy-path diagnostics print correctly with no spurious cap_set_proc message

Closes #5

@coderabbitai

coderabbitai Bot commented Jun 26, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 7acea379-60e8-4514-a13c-369e63bed8db

📥 Commits

Reviewing files that changed from the base of the PR and between 94ff6ea and a7ecc33.

📒 Files selected for processing (1)
  • meson.build
📜 Recent review details
⏰ Context from checks skipped due to timeout. (4)
  • GitHub Check: Build & Test (Ubuntu 22.04)
  • GitHub Check: Build & Test (Ubuntu 24.04)
  • GitHub Check: Analyze (rust)
  • GitHub Check: Analyze (c-cpp)
🧰 Additional context used
📓 Path-based instructions (1)
meson.build

📄 CodeRabbit inference engine (AGENTS.md)

Use Meson as the build system configuration for the project.

Files:

  • meson.build
🔇 Additional comments (2)
meson.build (2)

4-4: LGTM!


14-40: LGTM!


📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes

    • Improved error handling for capability dropping and seccomp setup, with consistent -1/0 return behavior and clearer diagnostics.
    • Corrected socket send/error logging and removed misleading/duplicate messages.
    • DRM capture failures now include the original errno message by preserving it through the error path.
  • Chores

    • Bumped library/Meson version to 0.4.3 and updated public version constants.
    • Added a version coherence check in the build.
    • Updated version tests to compute expectations from the header macros.

Walkthrough

This PR updates helper error handling in both C copies, bumps the project and public version constants to 0.4.3, and changes the C and Rust version tests to compute the expected packed version from the macros.

Changes

Helper diagnostics and version sync

Layer / File(s) Summary
Helper diagnostics
helper/drmtap-helper.c, bindings/rust/libdrmtap-sys/csrc/drmtap-helper.c
Capability, seccomp, socket, and DRM capture error paths now use more specific diagnostics and consistent failure returns in both helper C copies.
Version sync
meson.build, include/drmtap.h, bindings/rust/libdrmtap-sys/csrc/drmtap.h, tests/test_enumerate.c, tests/test_helper.c, bindings/rust/libdrmtap-sys/src/lib.rs
The project version and public DRMTAP_VERSION_* values move to 0.4.3, and the version tests now compute the expected packed value from those macros.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • fxd0h/libdrmtap#13: Overlaps on helper/drmtap-helper.c, including the privileged hardening and seccomp/capability paths.

Poem

I hopped through caps and seccomp gates,
and straightened out the error states.
The version hopped to four-dot-three,
and tests now match the macros, see?
Thump-thump! The helper sings once more.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly captures the two main changes: helper error diagnostics and version coherence.
Description check ✅ Passed The description matches the changeset and explains the diagnostics and version updates in context.
Linked Issues check ✅ Passed The PR addresses the linked issue by fixing misleading helper diagnostics and updating version reporting coherently.
Out of Scope Changes check ✅ Passed The added tests and Meson version checks support the stated fixes and do not appear unrelated.
Docstring Coverage ✅ Passed Docstring coverage is 93.33% which is sufficient. The required threshold is 80.00%.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch diag/helper-perror-version

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 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 `@bindings/rust/libdrmtap-sys/csrc/drmtap-helper.c`:
- Around line 191-200: The `drmtap-helper` error handling in the `cap_set_proc`
path has the same `errno`-loss issue as the other helper: `cap_free(caps)` can
change `errno` before `perror()` runs, causing misleading diagnostics. Update
the embedded helper logic around `cap_set_proc` in `drmtap-helper.c` to preserve
`errno` before calling `cap_free(caps)`, then restore it before the
`perror("drmtap-helper: cap_set_proc failed")` call so the reported error
remains accurate.

In `@bindings/rust/libdrmtap-sys/src/lib.rs`:
- Around line 149-151: The sys version test is still hardcoding the packed
version literal, so it can drift from the C library version. Update the
assertion in libdrmtap-sys/src/lib.rs to derive the expected value from the
exposed DRMTAP_VERSION_* constants instead of using 0x000404. Keep the check
near drmtap_version() and make it mirror how the C tests compute the packed
version so this stays in sync across releases.

In `@helper/drmtap-helper.c`:
- Around line 191-200: The error reporting in drmtap-helper around cap_set_flag
and cap_set_proc can lose the original failure cause because cap_free(caps) may
overwrite errno before perror() runs. In drmtap_helper.c, preserve errno
immediately after the failing libcap call, perform cleanup, then restore or use
the saved value when calling perror() in these error paths so the logged message
reflects the real failure.
🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 793e616d-fd64-4a4e-bd88-6a049108090c

📥 Commits

Reviewing files that changed from the base of the PR and between 99fba93 and 1f9ad68.

📒 Files selected for processing (8)
  • bindings/rust/libdrmtap-sys/csrc/drmtap-helper.c
  • bindings/rust/libdrmtap-sys/csrc/drmtap.h
  • bindings/rust/libdrmtap-sys/src/lib.rs
  • helper/drmtap-helper.c
  • include/drmtap.h
  • meson.build
  • tests/test_enumerate.c
  • tests/test_helper.c
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{c,h}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{c,h}: Use 4-space indentation, never tabs, and follow the documented C style conventions.
Use snake_case for functions and variables, prefix public API functions with drmtap_, use UPPER_SNAKE_CASE for macros/constants with DRMTAP_ prefixes, and use snake_case_t for internal types and drmtap_* for public types.
Use same-line braces (1TBS style).
Keep line length to a 100-character soft limit and 120-character hard limit.
Use // for single-line comments and /* */ for multi-line comments.
Return negative errno values for errors, never abort, and always clean up resources on error paths.
Always check malloc returns and free allocated memory in cleanup paths.
Every .c and .h file must start with the standard libdrmtap copyright/SPDX header block and an @file/@brief Doxygen file header.

Files:

  • tests/test_helper.c
  • tests/test_enumerate.c
  • include/drmtap.h
  • bindings/rust/libdrmtap-sys/csrc/drmtap.h
  • helper/drmtap-helper.c
  • bindings/rust/libdrmtap-sys/csrc/drmtap-helper.c
tests/*.c

📄 CodeRabbit inference engine (AGENTS.md)

tests/*.c: Write tests as simple assert-based C programs without an external test framework.
Use DRM_DEVICE from the environment in tests and do not hardcode /dev/dri/card0.
Do not test tiling/deswizzle behavior against vkms, because vkms is LINEAR-only.
Account for the fact that bare vkms has no active planes and may require a compositor for integration tests.

Files:

  • tests/test_helper.c
  • tests/test_enumerate.c
**/*.h

📄 CodeRabbit inference engine (AGENTS.md)

Use #ifndef DRMTAP_MODULE_H / #define DRMTAP_MODULE_H style header guards.

Files:

  • include/drmtap.h
  • bindings/rust/libdrmtap-sys/csrc/drmtap.h
include/drmtap.h

📄 CodeRabbit inference engine (AGENTS.md)

Public API functions in include/drmtap.h must have Doxygen comments documenting behavior, parameters, return values, and relevant error codes.

Files:

  • include/drmtap.h
meson.build

📄 CodeRabbit inference engine (AGENTS.md)

Use Meson as the build system configuration for the project.

Files:

  • meson.build
helper/*.c

📄 CodeRabbit inference engine (AGENTS.md)

The privileged helper binary should implement the helper spawn and SCM_RIGHTS IPC path.

Files:

  • helper/drmtap-helper.c
🔇 Additional comments (7)
helper/drmtap-helper.c (1)

212-258: LGTM!

Also applies to: 273-287

bindings/rust/libdrmtap-sys/csrc/drmtap-helper.c (1)

212-258: LGTM!

Also applies to: 273-287

meson.build (1)

3-4: LGTM!

include/drmtap.h (1)

28-34: LGTM!

bindings/rust/libdrmtap-sys/csrc/drmtap.h (1)

28-34: LGTM!

tests/test_enumerate.c (1)

31-34: LGTM!

tests/test_helper.c (1)

37-40: LGTM!

Comment thread bindings/rust/libdrmtap-sys/csrc/drmtap-helper.c
Comment thread bindings/rust/libdrmtap-sys/src/lib.rs Outdated
Comment thread helper/drmtap-helper.c
@fxd0h fxd0h force-pushed the diag/helper-perror-version branch from 1f9ad68 to afccf28 Compare June 26, 2026 16:37
@fxd0h fxd0h changed the title helper: accurate failure diagnostics + bump library version to 0.4.4 helper: accurate failure diagnostics + coherent library version Jun 26, 2026
@fxd0h

fxd0h commented Jun 26, 2026

Copy link
Copy Markdown
Owner Author

Thanks, all three are addressed in the latest push.

  • errno before cap_free (both copies): saved errno right after the failing cap call and restored it before perror on the cap_set_flag and cap_set_proc paths. Good catch, that would have reintroduced exactly the misleading-message problem this PR is fixing.
  • sys test hardcoded literal: switched it to derive from CARGO_PKG_VERSION, and aligned the C library version to the crate (0.4.3 rather than 0.4.4, since the crate is frozen at 0.4.3 until the next release). So the sys test now also doubles as a coherence check between the C header and the crate version. The C tests already derive from DRMTAP_VERSION_*.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 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 `@bindings/rust/libdrmtap-sys/csrc/drmtap-helper.c`:
- Around line 191-198: The new single-line comments in drmtap-helper should use
C++-style line comments instead of block-comment syntax. Update the affected
comment sites in the helper flow around cap_set_flag, cap_set_proc, and the
other referenced spots so any added single-line notes are written with //, while
keeping any true multi-line explanations in /* */ form.
- Around line 286-290: The errno-backed grab failure paths are losing the saved
errno because send_error() only logs the reason string. Update the MODE_MAP_DUMB
failed and mmap failed call sites in drmtap-helper.c to preserve errno by
capturing it before any further calls, then either pass it into a helper or
format it alongside the reason; keep send_error() unchanged for the non-errno
callers since they do not need errno details.

In `@helper/drmtap-helper.c`:
- Around line 191-198: The added single-line comments in drmtap-helper.c are
using block-comment syntax instead of the required // style. Update the new
comments in the affected sections around cap_free(), cap_set_proc(), and the
other marked spots to use // for single-line notes, keeping /* */ only where a
true multi-line comment is needed.
- Around line 286-290: The current error reporting in send_error() only sends a
text reason, so the MODE_MAP_DUMB and mmap() failure paths lose the real
system-call failure cause. Update the call sites that report these failures to
capture errno immediately after the failing call and pass it through in the
diagnostic, or add a dedicated send_errno_error() helper that preserves and
reports errno alongside the reason. Keep the fix localized around send_error(),
the MODE_MAP_DUMB path, and the mmap() error handling so the original failure
details are not overwritten.
🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: f9ec6b23-ab23-44ba-aa7d-82cc9702f8f7

📥 Commits

Reviewing files that changed from the base of the PR and between 1f9ad68 and afccf28.

📒 Files selected for processing (8)
  • bindings/rust/libdrmtap-sys/csrc/drmtap-helper.c
  • bindings/rust/libdrmtap-sys/csrc/drmtap.h
  • bindings/rust/libdrmtap-sys/src/lib.rs
  • helper/drmtap-helper.c
  • include/drmtap.h
  • meson.build
  • tests/test_enumerate.c
  • tests/test_helper.c
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{c,h}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{c,h}: Use 4-space indentation, never tabs, and follow the documented C style conventions.
Use snake_case for functions and variables, prefix public API functions with drmtap_, use UPPER_SNAKE_CASE for macros/constants with DRMTAP_ prefixes, and use snake_case_t for internal types and drmtap_* for public types.
Use same-line braces (1TBS style).
Keep line length to a 100-character soft limit and 120-character hard limit.
Use // for single-line comments and /* */ for multi-line comments.
Return negative errno values for errors, never abort, and always clean up resources on error paths.
Always check malloc returns and free allocated memory in cleanup paths.
Every .c and .h file must start with the standard libdrmtap copyright/SPDX header block and an @file/@brief Doxygen file header.

Files:

  • tests/test_helper.c
  • tests/test_enumerate.c
  • bindings/rust/libdrmtap-sys/csrc/drmtap.h
  • include/drmtap.h
  • bindings/rust/libdrmtap-sys/csrc/drmtap-helper.c
  • helper/drmtap-helper.c
tests/*.c

📄 CodeRabbit inference engine (AGENTS.md)

tests/*.c: Write tests as simple assert-based C programs without an external test framework.
Use DRM_DEVICE from the environment in tests and do not hardcode /dev/dri/card0.
Do not test tiling/deswizzle behavior against vkms, because vkms is LINEAR-only.
Account for the fact that bare vkms has no active planes and may require a compositor for integration tests.

Files:

  • tests/test_helper.c
  • tests/test_enumerate.c
meson.build

📄 CodeRabbit inference engine (AGENTS.md)

Use Meson as the build system configuration for the project.

Files:

  • meson.build
**/*.h

📄 CodeRabbit inference engine (AGENTS.md)

Use #ifndef DRMTAP_MODULE_H / #define DRMTAP_MODULE_H style header guards.

Files:

  • bindings/rust/libdrmtap-sys/csrc/drmtap.h
  • include/drmtap.h
include/drmtap.h

📄 CodeRabbit inference engine (AGENTS.md)

Public API functions in include/drmtap.h must have Doxygen comments documenting behavior, parameters, return values, and relevant error codes.

Files:

  • include/drmtap.h
helper/*.c

📄 CodeRabbit inference engine (AGENTS.md)

The privileged helper binary should implement the helper spawn and SCM_RIGHTS IPC path.

Files:

  • helper/drmtap-helper.c
🔇 Additional comments (6)
meson.build (1)

4-4: LGTM!

include/drmtap.h (1)

28-34: LGTM!

bindings/rust/libdrmtap-sys/csrc/drmtap.h (1)

28-34: LGTM!

tests/test_enumerate.c (1)

31-34: LGTM!

tests/test_helper.c (1)

37-40: LGTM!

bindings/rust/libdrmtap-sys/src/lib.rs (1)

149-157: LGTM!

Comment on lines +191 to +198
int saved = errno; /* cap_free() may clobber errno */
cap_free(caps);
perror("cap_set_proc failed"); return -1;
errno = saved;
perror("drmtap-helper: cap_set_flag failed"); return -1;
}

int ret = cap_set_proc(caps);
int saved = errno; /* preserve before cap_free() touches errno */

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use // for added single-line comments.

These new single-line comments use block-comment syntax. As per coding guidelines, use // for single-line comments and /* */ for multi-line comments.

Also applies to: 216-217, 242-242, 256-256, 289-289

🤖 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 `@bindings/rust/libdrmtap-sys/csrc/drmtap-helper.c` around lines 191 - 198, The
new single-line comments in drmtap-helper should use C++-style line comments
instead of block-comment syntax. Update the affected comment sites in the helper
flow around cap_set_flag, cap_set_proc, and the other referenced spots so any
added single-line notes are written with //, while keeping any true multi-line
explanations in /* */ form.

Source: Coding guidelines

Comment thread bindings/rust/libdrmtap-sys/csrc/drmtap-helper.c Outdated
Comment thread helper/drmtap-helper.c
Comment on lines +191 to +198
int saved = errno; /* cap_free() may clobber errno */
cap_free(caps);
perror("cap_set_proc failed"); return -1;
errno = saved;
perror("drmtap-helper: cap_set_flag failed"); return -1;
}

int ret = cap_set_proc(caps);
int saved = errno; /* preserve before cap_free() touches errno */

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use // for added single-line comments.

These new single-line comments use block-comment syntax. As per coding guidelines, use // for single-line comments and /* */ for multi-line comments.

Also applies to: 216-217, 242-242, 256-256, 289-289

🤖 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 `@helper/drmtap-helper.c` around lines 191 - 198, The added single-line
comments in drmtap-helper.c are using block-comment syntax instead of the
required // style. Update the new comments in the affected sections around
cap_free(), cap_set_proc(), and the other marked spots to use // for single-line
notes, keeping /* */ only where a true multi-line comment is needed.

Source: Coding guidelines

Comment thread helper/drmtap-helper.c Outdated
The privileged helper printed perror("cap_set_proc failed") on six error
paths, four of which had nothing to do with capabilities (cap_init,
cap_set_flag, seccomp_init, seccomp_rule_add, send, and the generic grab
error path). In a setcap binary a wrong error message actively misleads an
operator during incident response, so give each path an accurate message,
chosen by whether the failing call actually sets errno:

  - cap_init / cap_set_flag / cap_set_proc / send: perror with the real call.
    On the cap paths errno is saved before cap_free() and restored before
    perror(), since the cleanup call may clobber it.
  - seccomp_init: plain fprintf, it does not set errno so perror would print
    a stale value.
  - seccomp_rule_add / seccomp_load: they return -errno, so report
    strerror(-rc) instead of perror.

The genuine cap_set_proc and seccomp_load failure paths now log instead of
returning silently.

The grab error path is split in two: send_error() for logical/validation
failures (no useful errno), and send_error_errno() for syscall/libdrm
failures (MODE_MAP_DUMB, mmap, drmModeGetFB2, drmModeGetPlaneResources),
which captures errno before send_all() can overwrite it and appends
strerror(). Previously every grab error printed a bogus "cap_set_proc
failed" via perror; this keeps the real cause where errno carries one and
drops the misleading string everywhere else.

drmtap_version() still reported 0.1.0 while the project is well past that,
which reads as unmaintained. Set DRMTAP_VERSION_* and the meson project
version to 0.4.3 so the C library reports the same version as the
libdrmtap-sys crate (same C sources). The version tests now derive their
expected value instead of hardcoding it: the C tests from DRMTAP_VERSION_*,
and the Rust sys test from CARGO_PKG_VERSION, which also cross-checks that
the C header and the crate version stay equal.

Closes #5
@fxd0h fxd0h force-pushed the diag/helper-perror-version branch from afccf28 to 94ff6ea Compare June 26, 2026 16:47
@fxd0h

fxd0h commented Jun 26, 2026

Copy link
Copy Markdown
Owner Author

Round 2:

  • errno lost on the syscall grab paths: agreed, that was a real regression from this PR (the old mislabeled perror at least printed the errno). Split the error reporter in two: send_error() for logical/validation failures with no useful errno (no active framebuffer, handles[0]==0, geometry too large, unknown command), and a new send_error_errno() for syscall/libdrm failures (MODE_MAP_DUMB, mmap, drmModeGetFB2, drmModeGetPlaneResources) that captures errno before send_all() and appends strerror(). Pushed.

  • comment style (// vs /* /): leaving these as / /. This file uses C-style block comments for inline and short explanatory notes throughout (/ DRM ioctls /, / data_size=0 signals error /, the / NOTE: open/openat ... / block, etc.) and reserves // for function-leading comments. The notes I added are inline/explanatory, so / */ matches the local idiom. Switching just these to // would make them the odd ones out.

Re-validated locally: release+werror, asan+ubsan, plain/FORTIFY, meson test 5/5, cppcheck clean, cargo test green, real V3 capture OK.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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 `@helper/drmtap-helper.c`:
- Around line 185-205: The capability and seccomp error paths in drmtap-helper
are still collapsing detailed failure codes into a generic -1. Update the
existing failure returns in the cap_init/cap_set_flag/cap_set_proc flow to
return the preserved negative errno value or the saved error code instead of -1,
and keep the cleanup/error logging intact. Also apply the same return-value
change to the seccomp_* paths so they propagate their existing rc/ret values,
and mirror the exact behavior in the duplicate drmtap-helper implementation
under the Rust bindings csrc.

In `@meson.build`:
- Line 4: Meson’s project version is still not tied to the exported/runtime
version values, so it can drift from DRMTAP_VERSION_* and CARGO_PKG_VERSION_*.
Update the version handling around meson.project_version() so the same source
drives the generated version macros, or add a configure/test-time assertion that
the Meson project version matches the library version symbols used by
drmtap_version(), tests/test_enumerate, tests/test_helper, and the Rust
bindings.
🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 9a75444c-32a0-428e-94a1-c6d965318771

📥 Commits

Reviewing files that changed from the base of the PR and between afccf28 and 94ff6ea.

📒 Files selected for processing (8)
  • bindings/rust/libdrmtap-sys/csrc/drmtap-helper.c
  • bindings/rust/libdrmtap-sys/csrc/drmtap.h
  • bindings/rust/libdrmtap-sys/src/lib.rs
  • helper/drmtap-helper.c
  • include/drmtap.h
  • meson.build
  • tests/test_enumerate.c
  • tests/test_helper.c
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{c,h}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{c,h}: Use 4-space indentation, never tabs, and follow the documented C style conventions.
Use snake_case for functions and variables, prefix public API functions with drmtap_, use UPPER_SNAKE_CASE for macros/constants with DRMTAP_ prefixes, and use snake_case_t for internal types and drmtap_* for public types.
Use same-line braces (1TBS style).
Keep line length to a 100-character soft limit and 120-character hard limit.
Use // for single-line comments and /* */ for multi-line comments.
Return negative errno values for errors, never abort, and always clean up resources on error paths.
Always check malloc returns and free allocated memory in cleanup paths.
Every .c and .h file must start with the standard libdrmtap copyright/SPDX header block and an @file/@brief Doxygen file header.

Files:

  • tests/test_helper.c
  • include/drmtap.h
  • tests/test_enumerate.c
  • bindings/rust/libdrmtap-sys/csrc/drmtap.h
  • helper/drmtap-helper.c
  • bindings/rust/libdrmtap-sys/csrc/drmtap-helper.c
tests/*.c

📄 CodeRabbit inference engine (AGENTS.md)

tests/*.c: Write tests as simple assert-based C programs without an external test framework.
Use DRM_DEVICE from the environment in tests and do not hardcode /dev/dri/card0.
Do not test tiling/deswizzle behavior against vkms, because vkms is LINEAR-only.
Account for the fact that bare vkms has no active planes and may require a compositor for integration tests.

Files:

  • tests/test_helper.c
  • tests/test_enumerate.c
**/*.h

📄 CodeRabbit inference engine (AGENTS.md)

Use #ifndef DRMTAP_MODULE_H / #define DRMTAP_MODULE_H style header guards.

Files:

  • include/drmtap.h
  • bindings/rust/libdrmtap-sys/csrc/drmtap.h
include/drmtap.h

📄 CodeRabbit inference engine (AGENTS.md)

Public API functions in include/drmtap.h must have Doxygen comments documenting behavior, parameters, return values, and relevant error codes.

Files:

  • include/drmtap.h
meson.build

📄 CodeRabbit inference engine (AGENTS.md)

Use Meson as the build system configuration for the project.

Files:

  • meson.build
helper/*.c

📄 CodeRabbit inference engine (AGENTS.md)

The privileged helper binary should implement the helper spawn and SCM_RIGHTS IPC path.

Files:

  • helper/drmtap-helper.c
🔇 Additional comments (2)
bindings/rust/libdrmtap-sys/csrc/drmtap-helper.c (1)

191-191: Use // for these single-line comments.

Same guideline issue as the earlier review: these one-line notes are still written as /* ... */ instead of //.

Also applies to: 198-198, 216-216, 242-242, 256-256, 288-288, 298-298

helper/drmtap-helper.c (1)

191-191: Use // for these single-line comments.

Same guideline issue as the earlier review: these one-line notes are still written as /* ... */ instead of //.

Also applies to: 198-198, 216-216, 242-242, 256-256, 288-288, 298-298

Comment thread helper/drmtap-helper.c
Comment on lines +185 to 205
perror("drmtap-helper: cap_init failed"); return -1;
}

cap_value_t keep[] = { CAP_SYS_ADMIN };
if (cap_set_flag(caps, CAP_PERMITTED, 1, keep, CAP_SET) != 0 ||
cap_set_flag(caps, CAP_EFFECTIVE, 1, keep, CAP_SET) != 0) {
int saved = errno; /* cap_free() may clobber errno */
cap_free(caps);
perror("cap_set_proc failed"); return -1;
errno = saved;
perror("drmtap-helper: cap_set_flag failed"); return -1;
}

int ret = cap_set_proc(caps);
int saved = errno; /* preserve before cap_free() touches errno */
cap_free(caps);

if (ret == 0) {
fprintf(stderr, "drmtap-helper: dropped caps, keeping CAP_SYS_ADMIN\n");
if (ret != 0) {
errno = saved;
perror("drmtap-helper: cap_set_proc failed");
return -1;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Return the specific error codes you already have.

These new paths now log the right failure, but they still collapse errno / libseccomp's -errno into -1. That drops machine-readable detail and conflicts with the C guideline to return negative errno values. Please return -errno/-saved for the capability failures and rc/ret for the seccomp_* failures, then mirror the same adjustment into bindings/rust/libdrmtap-sys/csrc/drmtap-helper.c.

Suggested shape
-        perror("drmtap-helper: cap_init failed"); return -1;
+        int saved = errno;
+        perror("drmtap-helper: cap_init failed");
+        return saved ? -saved : -1;
...
-        perror("drmtap-helper: cap_set_flag failed"); return -1;
+        perror("drmtap-helper: cap_set_flag failed");
+        return saved ? -saved : -1;
...
-        perror("drmtap-helper: cap_set_proc failed");
-        return -1;
+        perror("drmtap-helper: cap_set_proc failed");
+        return saved ? -saved : -1;
...
-            return -1;
+            return rc;
...
-        return -1;
+        return ret;
As per coding guidelines, "Return negative errno values for errors, never abort, and always clean up resources on error paths."

Also applies to: 242-259

🤖 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 `@helper/drmtap-helper.c` around lines 185 - 205, The capability and seccomp
error paths in drmtap-helper are still collapsing detailed failure codes into a
generic -1. Update the existing failure returns in the
cap_init/cap_set_flag/cap_set_proc flow to return the preserved negative errno
value or the saved error code instead of -1, and keep the cleanup/error logging
intact. Also apply the same return-value change to the seccomp_* paths so they
propagate their existing rc/ret values, and mirror the exact behavior in the
duplicate drmtap-helper implementation under the Rust bindings csrc.

Source: Coding guidelines

Comment thread meson.build
The library version lives in the meson project() version, the
DRMTAP_VERSION_* macros in include/drmtap.h, and the Rust crate version.
The version unit tests already tie the header macros to drmtap_version() and
to CARGO_PKG_VERSION; this closes the last gap by parsing the public header
at configure time and asserting it equals meson.project_version(), so a
future bump that touches one source but not the other fails the build early
with a clear message instead of drifting silently.
@fxd0h

fxd0h commented Jun 26, 2026

Copy link
Copy Markdown
Owner Author

Round 3:

  • meson version drift: good point, added a configure-time assertion (build: assert meson version matches DRMTAP_VERSION_*). meson.build now parses include/drmtap.h and asserts the macros equal meson.project_version(), so the header, the project version, and (via the existing unit tests) the crate version are all locked together. A bump that touches one source but not the others now fails meson setup early with a clear message. Verified it both passes when coherent and fails on an injected drift.

  • returning -errno instead of -1 from drop_caps/install_seccomp: leaving as -1. Both callers (main, lines ~766/774) only test the result as a boolean and then print 'refusing to run' and exit, so the specific code is never consumed. The actionable diagnostic is the stderr message inside each function, which is exactly what this PR makes accurate. Propagating an unused errno would be churn without changing any observable behavior.

Re-validated: release/asan/plain reconfigure clean, meson test 5/5.

@fxd0h fxd0h merged commit d1e3d18 into main Jun 26, 2026
9 checks passed
@fxd0h fxd0h deleted the diag/helper-perror-version branch June 26, 2026 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Helper: misleading diagnostics (6x wrong perror) + stale drmtap_version() in the privileged TCB

1 participant