Modinfo rework

[evelikov]

Here's a series addressing most of the formatting issues raised in #434. In particular the formatting code is unified and beaten into shape to use consistent formatting. With the outstanding bits being:

• the module description string is free-form and can have misc whitespace characters - \t, \n et al
• the signature value is still has multi-line/wrapped/tabbed formatting

In all fairness, I'm inclined to leave those as-is and let other handle them in a future PR.

The first 3 commits are something we'd probably want for the stable 34.x branch assuming there will be another release. Although, in that case we should probably check the other PRs for similar non-invasive fixes.

For the PR itself - it mostly small prep and fixup patches with the most sizable change being modinfo: rework parm handling.

Resolves: #434

[codecov]

Codecov Report

❌ Patch coverage is 73.01587% with 34 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
tools/modinfo.c	73.73%	13 Missing and 13 partials ⚠️
testsuite/test-shared.c	71.42%	1 Missing and 5 partials ⚠️
shared/strbuf.c	66.66%	0 Missing and 2 partials ⚠️

Files with missing lines	Coverage Δ
shared/macro.h	100.00% <ø> (ø)
shared/strbuf.h	75.00% <ø> (ø)
shared/strbuf.c	80.39% <66.66%> (+3.03%)	⬆️
testsuite/test-shared.c	76.42% <71.42%> (ø)
tools/modinfo.c	52.80% <73.73%> (+2.99%)	⬆️

... and 17 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[evelikov]

The latest revision has all the yaks shaved on my end ;-)

v2:

• drop calloc commit
• enum names p_parm{desc,type} -> parm_{desc,type]
• find matching or empty entry in a single loop (add_param)
• match up-to UINT_MAX parm/parmtype entries
• drop unneeded INT_MAX checks for strlen(value)
• don't return success if we fail to [cm]alloc
• kill off struct param::{desc,type}len
• rework parm string length validation

Edit: this time for real - aka rebase + clang-format nitpick

[evelikov]

NOTE: this effectively prints the "parm" entries in inverse order... AFAICT that should not be an issue, since the output order was never meant to be deterministic.

[lucasdemarchi]

I think we need to be careful here... we may be running modinfo() into a module that is builtin for the current kernel, but we are actually doing modinfo for something else.

Example: ext4 builtin on my distro, but I'm doing modinfo ./ext4.ko for a kernel I'm developing for another system. Previously it would notice that mod got constructed from a module and use that as filename. Here you'd wrongly report that it is builtin.

[evelikov]

Valid point - dropped for now.

As a whole, a few functions rely on parsing /sys/modules and friends even when a different kernel version is explicitly provided to kmod_ctx_new. The current behaviour seems suboptimal/buggy.

IMHO we should probably error out, although I could be missing something?

[evelikov]

Somewhat related:

It looks like we have different filename (path really) in database vs filesystem... without ever flagging it as an issue. Observed this while copying/adding more modinfo tests. To make it more interesting, setup-rootfs.sh manually changes the path in .dep files but not in .dep.bin and co.

One possible (perhaps overly comprehensive) solution is:

• have the proper depmod artefacts checked-in
• on test prep, re-generate ^^ into a throwaway location and error on miss-match
• ensure correct path is used across .dep/.dep.bin/etc - might need a patch to libkmod/depmod/
• on test closure, flag files with unchanged atime - ignoring the depmod regen/check

What do you think?

[evelikov]

v3:

• scope variable declaration

• code dedup in parm/parmtype paths

• use correct counter

• avoid allocations on 0 parm(s)

• remove param::{desc,type}len

• move parm* len validation further up the call stack

• make strbuf_reserve_extra public again + reintroduce tests

• fix overflow in ^^

• use ALIGN in ^^

• remove strbuf_init -

[lucasdemarchi]

and we had strbuf_steal() for this kind of situation that got removed. IMO let's not drop these helpers if they may be used in future?

[evelikov]

Looking at the PR that removed it:

• there is one overflow bug (fixed in this PR)
• the function comment/description didn't get updated
• the above workaround was pre-existing

Can reintroduce and fix ^^ as a follow up PR.

Would you be OK with dropping "strbuf_init` or prefer to keep it - I don't mind either way.

[lucasdemarchi]

Can reintroduce and fix ^^ as a follow up PR.

sounds good

Would you be OK with dropping "strbuf_init` or prefer to keep it - I don't mind either way.

we can drop the callers from the codebase, but let's keep it in the more-or-less-self-contained strbuf implementation. I'll push an update here before merging.

As a followup I think it's also reasonable to move strbuf_to_vector() to be in strbuf.c so the re-init happens inside the strbuf implementation. Then we document that sbuf becomes invalid afte the call.

[lucasdemarchi]

Also getting this:

=================================================================
==195428==ERROR: AddressSanitizer: attempting double-free on 0x6e5cad5e03c0 in thread T0:
    #0 0x712caf32a3ff in free ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:51
    #1 0x64011a76b0f4 in modinfo_do ../tools/modinfo.c:109

Since it's now using DECLARE_STRBUF_WITH_STACK(), we need to remove the strbuf_release() at the end.

Added 2 fixup commits to handle the comments.

[evelikov]

Added 2 fixup commits to handle the comments.

The two extra fixups look good to me. Thanks o/

[lucasdemarchi]

double checking this commit:

• modinfo: omit param::desc == NULL case

"printf("%.s", 0, NULL) is a perfectly valid use."

it actually isn't. Passing NULL here is undefined behavior regardless of the precision specifier. In the end this code was converted to strbuf so we don't need it. I will simply drop the commit and solve the conflicts.

[evelikov]

double checking this commit:

* modinfo: omit param::desc == NULL case

"printf("%.s", 0, NULL) is a perfectly valid use."

it actually isn't. Passing NULL here is undefined behavior regardless of the precision specifier. In the end this code was converted to strbuf so we don't need it. I will simply drop the commit and solve the conflicts.

May I ask where did you spot that?

The man page suggests that up-to precision specifier characters of the string are accessed/printed. Plus the undefined sanitizer included in gcc (16.1.1 20260725) doesn't complain either.

Either way, thanks o/

[lucasdemarchi]

There's some discussion here about that part of the standard: https://stackoverflow.com/questions/70499706/is-it-guaranteed-by-the-c-standard-to-be-safe-to-do-printf-s-0-null

[lucasdemarchi]

Applied, thanks

[evelikov]

It seems like v2 got merged, including the UB printf("%.s", 0, NULL) piece.

I'm assuming that's by mistake - will send an update PR tomorrow/day after, with all of your comments addressed.

[lucasdemarchi]

ugh... sorry. That's what I get for working in 2 different computers :-/.

[lucasdemarchi]

I removed the commits from the master branch and will re-add once I get my other computer back. Sorry about that.

[lucasdemarchi]

Ok, now it's hopefully the right branch pushed here. @evelikov can you double check?

[evelikov]

I removed the commits from the master branch and will re-add once I get my other computer back. Sorry about that.

No problem - sh*t happens. Would prefer if we don't rewrite the branch history but revert/fixup instead. The former erodes the distro maintainers' trust, et al.

The updated branch looks great. Thanks