Page MenuHomeFreeBSD

libkldelf: add a private library for kernel/kld-related ELF parsing
ClosedPublic

Authored by khng on Sep 20 2024, 4:55 PM.
Tags
None
Referenced Files
F106436721: D46719.diff
Mon, Dec 30, 6:00 PM
Unknown Object (File)
Fri, Dec 27, 5:21 AM
Unknown Object (File)
Wed, Dec 11, 1:02 AM
Unknown Object (File)
Nov 30 2024, 12:19 AM
Unknown Object (File)
Nov 28 2024, 11:14 PM
Unknown Object (File)
Nov 17 2024, 5:05 AM
Unknown Object (File)
Nov 14 2024, 9:10 PM
Unknown Object (File)
Nov 14 2024, 3:45 PM

Details

Summary

The libkldelf library was originally a part of kldxref(8). It exposed
ELF parsing helpers specialized in parsing KLDs and the kernel
executable. The library can be used to read metadata such as linker_set,
mod_depend, mod_version and PNP match info, and raw data from the ELF.

To promote the reuse of the facilities the ELF parsing code is separated
from kldxref(8) into a new private library.

kldxref(8) is modified to link against the libkldelf library.

Sponsored by: Juniper Networks, Inc.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 59630
Build 56517: arc lint + arc unit

Event Timeline

khng requested review of this revision.Sep 20 2024, 4:55 PM
khng added a child revision: D46720: kldxref: use libkldxref.

I think it would be easier to review if you moved the files out of kldxref and changed kldxref to use the library as part of this commit. Git might be able to follow history in that case, but with simple copies it is not coping it seems.

I think I have to make it PRIVATELIB to be more useful.

khng retitled this revision from libkldelf: add an internal library for kernel/kld-related ELF parsing to libkldelf: add a private library for kernel/kld-related ELF parsing.Sep 23 2024, 6:07 PM

Updated to be a private library.

Do we need ef_mips.c still ?

In D46719#1066014, @imp wrote:

Do we need ef_mips.c still ?

At least not for the upstream FreeBSD I presume.

Ka Ho

Fix indentiation issue in mtree files

Update Makefile.depend as well.

This is missing some description of libkldelf's intended functionality, either in the commit message or in a man page (maybe a man page is not required for private libraries). It's hard to tell at a glance what this library does, at least for me who is not familiar with kldxref internals.

I don't have any objection to the change itself, it seems fine and will be useful for debuggers and related tools. libdtrace might be able to take advantage of it as well, replacing some existing code.

lib/libkldelf/Makefile
15

Does this mean that the header gets installed? Is that appropriate for a private library?

This is missing some description of libkldelf's intended functionality, either in the commit message or in a man page (maybe a man page is not required for private libraries). It's hard to tell at a glance what this library does, at least for me who is not familiar with kldxref internals.

I don't have any objection to the change itself, it seems fine and will be useful for debuggers and related tools. libdtrace might be able to take advantage of it as well, replacing some existing code.

Will update the commit message accordingly.

Update commit message. If it is not reflected on Phabricator I will do it one more time.

This seems ok to me. Please make sure you test MacOS and Linux builds as well, e.g., with a PR on github.

It would be nice to see the patches for intended consumers (libkvm?) before this lands, though.

lib/libkldelf/Makefile
15

Still wondering about this.

Remove include headers installation

khng marked an inline comment as done.Sep 27 2024, 5:20 PM
khng added inline comments.
lib/libkldelf/Makefile
15

Removed. The header is seemingly useless to be installed into usr/include for now.

lib/libkldelf/ef_amd64.c
108

Should this case be an error instead?

khng marked an inline comment as done.Oct 2 2024, 12:58 AM
khng added inline comments.
lib/libkldelf/ef_amd64.c
108

I could be wrong, but if we return error here kmods with R_X86_64_IRELATIVE might be an issue.

I think @jhb should get a chance to take another look before committing this.

lib/libkldelf/ef_amd64.c
108

Indeed, but it might be good to handle that one separately somehow, e.g., return EOPNOTSUPP and modify callers to handle it.

This revision is now accepted and ready to land.Oct 2 2024, 7:17 AM

Overall LGTM

lib/libkldelf/Makefile
7

while here, might as well make these one per line also

lib/libkldelf/ef_amd64.c
108

Agreed, silently ignoring unhandled relocations seems like a bad idea. This can be a followup change though.

khng marked an inline comment as done.

Bootstrap it everytime, and remove the __FreeBSD_version bump as well given this library is private.

This revision now requires review to proceed.Oct 2 2024, 5:52 PM
This revision was not accepted when it landed; it landed in state Needs Review.Oct 8 2024, 4:29 AM
This revision was automatically updated to reflect the committed changes.