kldinfo utility parses ELF header of a given KLD module file, then searches for
available Kernel Loadable objects and shows their dependency and version
information. If path to file is given the utility tries to open it directly.
It quits if open fails. Otherwise, it searches for .ko in directories set by
kern.module_path sysctl variable. Can be used to show builtin modules in kernel.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped - Build Status
Buildable 78226 Build 75109: arc lint + arc unit
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
The kldstat works with modules loaded into kernel memory, it finds module by its name and prints what module_stat has (id, refs, and a pointer to modspecific_t). It does not print version information and dependencies.
The proposed kldinfo works with files: it parses ELF, searches for KObjs, extracts their dependency and version info and fine-prints what it finds. Note, that one module file may have many KObjs those also may have deps on other modules. Extracting such info from module is not easy, one has to write a script that juggles other tools like nm, objdump, hexdump. Thus, kldinfo is that script written in pure C that does it on its own. ;) No sorting is currently implemented.
To illustrate:
% kldinfo amdgpio MODULE: amdgpio_acpi DEPENDS: kernel VERSION: 1501000 1501000 1501000 MODULE: amdgpio DEPENDS: acpi VERSION: 1 1 1 MODULE: amdgpio DEPENDS: gpiobus VERSION: 1 1 1 Found 3 dependencies in file /boot/kernel/amdgpio.ko
Calling on kernel file will print all modules built-in into that particular kernel:
% kldinfo /boot/kernel/kernel MODULE: xpt DEPENDS: kernel VERSION: 1501000 1501000 1599999 MODULE: xpt DEPENDS: cam VERSION: 1 1 1 MODULE: cam DEPENDS: kernel VERSION: 1501000 1501000 1599999 MODULE: aprobe DEPENDS: kernel VERSION: 1501000 1501000 1599999 MODULE: aprobe DEPENDS: cam VERSION: 1 1 1 ... MODULE: ixl DEPENDS: iflib VERSION: 1 1 1 MODULE: smartpqi_pci DEPENDS: kernel VERSION: 1501000 1501000 1599999 MODULE: smartpqi DEPENDS: pci VERSION: 1 1 1 MODULE: x86bios DEPENDS: kernel VERSION: 1501000 1501000 1599999 MODULE: ioapic_pci DEPENDS: kernel VERSION: 1501000 1501000 1599999 MODULE: apic_nexus DEPENDS: kernel VERSION: 1501000 1501000 1599999 Found 926 dependencies in file /boot/kernel/kernel
Comment Actions
Also, FreeBSD module has three version values: min, preferred and max. All three are printed.
Comment Actions
Generally, I like this.
If there's demand, we can add pnp info as well, but that's harder to format and should be separate form this commit.
Comment Actions
I'll investigate what it takes to extract and fine-print MODULE_PNP_INFO. Will provide a separate patch if something comes out of my keyboard.
Comment Actions
I only looked at the single function.
Please ensure that the sources are in KNF, see style(9). Then I will read it more.
| sbin/kldinfo/kldinfo.c | ||
|---|---|---|
| 47 | Why this var is not bool? | |
| 60 | Indent is wrong. Previous line can be filled more. | |
| 64 | There must be a blank line between var declarations and statements. | |
| 65 | Why is this not just str(n)cpy? | |
| 67 | Why there is so many blank lines in the bodies? They do not serve any purpose. | |
| 69 | Why new line is only printed for verbose mode? | |
Comment Actions
Thank you for finding time to review my code and also for pointing to style(9), it's been quite a while since I looked into that man page last time. Before uploading this patch I checked it with checkstyle9.pl which I wrongly thought was sufficient, sorry about that. Will re-style code according to KNF and update revision.
Some of the blank lines are put intentionally for clarity, some are rudiments left from removed debug info I have had plenty of when parsing ELF. ;)
strncat() is used because it always adds null-teminator, while strncpy() does not. strncpy() needs more love, i.e. more code, which makes things less readable.
Comment Actions
Code style rework
- Reworked code style according to style(9).
- Added more comments, some others reworded.
- Skip sysctl__version objects as irrelevant.
| sbin/kldinfo/kldinfo.c | ||
|---|---|---|
| 68 | Please use size_t for sizes. | |
| 73 | First of all, use strlcpy(3), not strncat(3). Second, what is the point of copying into a buffer which is only used for the subsequent printf(3)? Just use %*s instead. | |
| 167 | This function would benefit from being broken up. You can start by moving everything from elf_begin() to elf_end() into a separate function. | |
| 169–175 | Please order your variable definitions correctly (first by descending size, then by name). | |
| 178 | I know ch is commonly used for the return value from getopt(3) in old BSD code, but I suggest opt instead. | |
| 186 | This line serves no purpose. | |
| 194 | What if argc > 1? | |
| 197 | please use err(3) or errx(3) for errors. | |
| 306–311 | This would be more readable if you used a temporary variable for the base address. | |
| 346 | Why else here and not continue as above? | |
| 347 | I would check print_ver first. | |
| 358 | Why is this going to stderr and not stdout? | |
| sbin/kldinfo/Makefile | ||
|---|---|---|
| 2–29 | There is nothing copyrightable in this file. | |
| sbin/kldinfo/Makefile | ||
|---|---|---|
| 2–29 | I think I just copied Makefile from some other tool (kldload), it has this copyright and license message. Ok, will remove it. | |
| sbin/kldinfo/kldinfo.c | ||
| 68 | Will do, where it is possible. Note that printf("%.*s") wants string size to be int. | |
| 73 | I hope you mean a lesser known "%.*s" feature of printf(3) ? Ok, will definitely use it. | |
| 169–175 | Ok. | |
| 178 | Did not know that. Ok, will use opt. | |
| 186 | Agreed. | |
| 194 | Will set limit to one (file) exactly. | |
| 197 | Ok, will also use warnx() for messages. | |
| 347 | Makes sense, yes. | |
| 358 | We might want to pass kldinfo output to some other command (sort?), this stat message will be a pain. | |
Comment Actions
More coding style rework
- Order variables
- Use err(3) for errors and warnx(3) for messages
- Eliminate use of strncat(3) in favour of printf("%.*s)
- Remove unnecessary copyright message from Makefile
- Rename some vars, use temp var for base address
- Accept only one file in args
- Misc.
Comment Actions
Even more code style rework, add support for arbitrary num of files
- Reorder local vars by occupied amount of mem.
- Add support for arbitrary files, reflect feature in man page.
- Split main(), move all processing logic to process_module_file().
- Fix usage().
| sbin/kldinfo/kldinfo.c | ||
|---|---|---|
| 53–56 | none of these initializations are needed | |
| 108–109 | please use warnx() | |
| 112 | no cast is needed here | |
| 113–114 | please use warnx() | |
| 119–120 | please use warnx() | |
| 127 | size_t sorts after pointers | |
| 129 | structs go first | |
| 132 | you can avoid this if by normalizing the name upstream | |
| 133 | ...and you should check the return value | |
| 135 | ...and you should check the return value | |
| 163 | size_t sorts after pointers | |
| 327 | not really needed | |
| 346 | this is a nit but if you are treating argv here as an iterator rather than an array, then *argv is more appropriate than argv[0]. | |
| 351 | ||
| sbin/kldinfo/kldinfo.c | ||
|---|---|---|
| 53–56 | Agree. Will remove. | |
| 108–109 | Will do. I completely forgot about that. Sorry. | |
| 112 | Did not know that void* is compatible with any pointer in C. Thanks. | |
| 132 | Normalizing name upstream will make it impossible to use arbitrary file as input, i.e. /boot/kernel/kernel. I'll take that "if" out and put before the loop, so it will be called once. | |
| 327 | indeed. | |
| 346 | Ok, will use *argv. | |
Comment Actions
Yet some more code style rework
- Remove unnecessary initialization.
- Eliminate progname global var as it's used only once.
- Use err(3) to fail in find_module_file().
- Reorder variables (size_t after pointer).
- Take strstr() out of the loop.
- Use *argv instead of argv[0].
- Return with exit(EX_OK) on success.