Page MenuHomeFreeBSD

Dynamically load .so modules to expand functionality
ClosedPublic

Authored by imp on Thu, Dec 6, 5:15 PM.

Details

Summary

o Dynamically load all the .so files found in /libexec/nvmecontrol and

/usr/local/libexec/nvmecontrol.

o Link nvmecontrol -rdynamic so that its symbols are visible to the

libraries we load.

o Create mirrored linker sets that we dynamically expand.
o Add the linked-in top and logpage linker sets to the mirrors for them

and add those sets to the mirrors when we load a new .so.

o Add some macros to help hide the names of the linker sets.
o Update the man page.

Test Plan

run through all the commands before / after the change and make sure they work.
Use my experimental builds of intel.so and wdc.so to proof the concept.
A future commit will create the modules.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

imp created this revision.Thu, Dec 6, 5:15 PM
imp edited the test plan for this revision. (Show Details)Thu, Dec 6, 5:19 PM
imp added a reviewer: cem.
cem added a comment.Thu, Dec 6, 8:03 PM

Mostly LGTM, thanks.

sbin/nvmecontrol/nvmecontrol.c
89 ↗(On Diff #51665)

style nit: separate line for static void

151–159 ↗(On Diff #51665)

Seems like this could be simplified somewhat with realloc:

m->begin = reallocarray(m->begin, cur_n + add_n, sizeof(void *));
if (m->begin == NULL)
    err(1, "expanding mirror set");
memcpy(m->begin + cur_n, bp, add_n * sizeof(void *));

(err can be used, malloc/realloc/etc set errno on failure.)

324 ↗(On Diff #51665)

extra space between NULL and ;

325 ↗(On Diff #51665)

Could use dent->d_namlen in place of strlen(dent->d_name).

327–329 ↗(On Diff #51665)

I'd err() this failure — if this tiny allocation failed, we're not going to be able to make much progress anyway.

351 ↗(On Diff #51665)

closedir(dir); ?

360–361 ↗(On Diff #51665)

I'm not sure if libexec is the right place for these. They might just be 'lib'. libexec is generally for helper programs, not modules. There is some precedence here in, e.g., /lib/geom. (Either way, it should be documented in hier.7.)

sbin/nvmecontrol/nvmecontrol.h
92 ↗(On Diff #51665)

How did you land on the name "mirror?" It's confusing to me. Something like "contiguous_set" would make more sense to me, since the goal is to contain multiple .so's set tables in a single contiguous VA for easy searching.

imp marked 7 inline comments as done.Thu, Dec 6, 9:21 PM
imp added inline comments.
sbin/nvmecontrol/nvmecontrol.c
89 ↗(On Diff #51665)

used in one place, and less gross than before, so expanded inline.

151–159 ↗(On Diff #51665)

Good idea.

324 ↗(On Diff #51665)

thanks

325 ↗(On Diff #51665)

true. Don't really need to optimize here, but it's easy.

327–329 ↗(On Diff #51665)

OK.

351 ↗(On Diff #51665)

nice catch.

360–361 ↗(On Diff #51665)

I Was all set to argue, but we have /lib/geom, /lib/casper (annd libexec/casper which just has executables) and /usr/lib/clang. So I'm sold.

imp updated this revision to Diff 51685.Thu, Dec 6, 9:27 PM
imp edited the test plan for this revision. (Show Details)

updates per cem@

cem accepted this revision.Thu, Dec 6, 10:09 PM

LGTM. I still find the 'mirror' name confusing, but maybe that's a personal problem ;-).

This revision is now accepted and ready to land.Thu, Dec 6, 10:09 PM
imp added a comment.Thu, Dec 6, 10:51 PM
In D18455#393317, @cem wrote:

LGTM. I still find the 'mirror' name confusing, but maybe that's a personal problem ;-).

I'll change it to set_concat which is less confusing.

This revision was automatically updated to reflect the committed changes.