Add -j <jail> flag to ngctl to allow ngctl to attach and run inside a
jail. This allow parent to manipulate netgraph nodes in the jail
even if ngctl is not available.
Details
Details
Motivation is to simplify testing netgraph within jails.
This also lets you use netgraph with jails that were built with either WITHOUT_NETGRAPH or WITHOUT_NETGRAPH_SUPPORT.
That is ngctl -j jid_or_name ... is able to run even when jexec jid_or_name ngctl ... may not be able to (when ngctl(8) is not present in the jail).
ngctl(8) can NOT load modules when used with -j option. This is good and no different from ifconfig(8) -j option for epair(4).
I don't want to be disingenuous, it does makes testing easier as I can use a "bare jail" and not care if ngctl(8) is available. But the reason I want it for a separate reviews: D50244 and D50245.
This is the only patch of the series that changes existing code.
Similar reviews (D40213 and D40377) asked for the jail_attach to be broken out of the getopt handling. I did not do that because the -f option already just opens a file in place.
First make sure we don't have the modules loaded:
kldstat | grep -E 'eiface|epair'
Now create a "bare jail":
jail -i -c name=demo host.hostname=demo.example.net vnet persist
And verify that neither command loads a kernel module (we already know ifconfig(8) is correct):
ifconfig -j demo epair create ifconfig: SIOCIFCREATE2 (epair): Invalid argument ngctl -j demo mkpeer eiface e ether ngctl: send msg: Operation not permitted
That is as it should be, kernel modules can't be loaded once you switch to a jail context. But if we have the necessary module loaded it works fine:
kldload ng_eiface ngctl -j demo mkpeer eiface e ether ngctl -j demo ls There are 2 total nodes: Name: ngeth0 Type: eiface ID: 00000003 Num hooks: 0 Name: ngctl13421 Type: socket ID: 00000004 Num hooks: 0
Cleanup of netgraph nodes in a jail is not necessary the system cleans them:
jail -r demo
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Comment Actions
Check out https://docs.freebsd.org/en/articles/license-guide/ or style.9 from current, there is a much shorter text you can use now. EDIT: This was for your other review, sorry.
Comment Actions
That makes sense as I'm not comfortable changing licenses for existing code. But do you mean I should change for both D50244 and D50245 ? I think I just realized I did the usual thing of copying sys/netgraph/ng_sample.[ch] for creating ng_wormhole.[ch] and it has the "WHISTLE COMMUNICATIONS" disclaimer.
Should I change both to the much shorter text?
Comment Actions
@imp has historically said we should avoid changing it for existing files without signoff from copyright holders.
Comment Actions
New code should have the shorter text. We cannot change licenses of existing code unless lots of corner circumstances.
Edit, sorry for the confusion. I was not recommending changing existing license text.
| usr.sbin/ngctl/main.c | ||
|---|---|---|
| 684 | Would it be worth changing getopt() to getopt_long() so order options are given in won't matter? I kind of struggled with that as I don't want any existing use of ngctl(8) to break because of this addition. Or do you just mean in this usage string? I think I have same order for man page so there as well? | |
Comment Actions
@imp has historically said we should avoid changing it for existing files without signoff from copyright holders.
Right, my imprecise use of language confused this. I 100% agree and that is what I meant to clarify: only change for my new files which are not in this review.
| usr.sbin/ngctl/main.c | ||
|---|---|---|
| 684 | Yeah, sorry, just in the usage string (and man page, since you brought it up =)). | |
| usr.sbin/ngctl/main.c | ||
|---|---|---|
| 156 | I guess when the -f option is given, the file should be opened prior to attaching to jail. Otherwise ngctl(8) would try to open the file within the jail's root path. Then the order of -j and -f matters and behavior varies. FYI, A similar change to sysctl(8) https://reviews.freebsd.org/D48618 . | |
| usr.sbin/ngctl/main.c | ||
|---|---|---|
| 156 | Wow. I totally missed this implication. And now that you have pointed it out I will definitely be refactoring to switch to jail outside the getopt as well as fixing order. | |
| usr.sbin/ngctl/main.c | ||
|---|---|---|
| 684 | I don't have a strong opinion, but the man page should match. The -f explanation comes after -j, just as in sysctl(8), because it explains that the file is opened before the switch to a new jail and its easier with this ordering. | |
| usr.sbin/ngctl/ngctl.8 | ||
| 44 | This can move above so -j appears before -f in synopsis. But this is only part of man page I would move just to keep in sync with Usage() function. | |
Comment Actions
Not sure if this is the time, but I also noticed that the command usage and man page synopsis differ. The man page synopsis uses [-n nodename] and the command usage uses [-n name] the usage should be changed to match the man page.
Additionally in the man page, the synopsis uses [-f filename] but the description uses -f nodeinfo. That description is confusing and wrong and should change to match synopsis.
Since these would change the command usage string and man page synopsis already, bug 262094 could also be done.
That bug is just making it clearer you only get one command altering [command ...] to [command [argument ...]].
If I need to change the [-j jail] order to be consistent I could fix those stray documentation issues too (unless they need a separate commit).
Comment Actions
Be consistent with other commands and have [-j jail] first. Clean up other issues in man page. Importantly the usage string and the man page synopsis agree now.
| usr.sbin/ngctl/main.c | ||
|---|---|---|
| 167 | I don't believe it's possible to get NULL here; if the argument is specified a required, getopt(3) won't return 'j' at all without an argument. That said, might be a good ide to check for *jail_name == '\0' here. | |
| usr.sbin/ngctl/main.c | ||
|---|---|---|
| 167 | yeah I didn't think so but I snaked that line from D48618 (sysctl adding -j). Most do an if (optarg == NULL) but I agree that it shouldn't even be possible since I have the colon after the j, I wouldn't even think we need to check for \0 to be honest. Let me go re-read man page and get back to you on this. | |
| usr.sbin/ngctl/main.c | ||
|---|---|---|
| 167 | Ahh, yes. That is a stupid wrong usage. optarg == NULL is possible only when the argument is optional (.GNU extension ). | |
| usr.sbin/ngctl/main.c | ||
|---|---|---|
| 167 | You don't technically need to check for nul, but it leads to a better error message. getopt(3) won't prevent -j '' because empty arguments are valid- we'd otherwise pass it on to jail_getid and get a complaint there. | |
Comment Actions
Remove check for NULL from -j option not required. Check for empty string before attempting to switch to jail.
New output:
root@fbsd15:~ # ngctl -j ngctl: option requires an argument -- j usage: ngctl [-j jail] [-d] [-f filename] [-n nodename] [command [argument ...]] root@fbsd15:~ # ngctl -j '' ngctl: invalid jail name usage: ngctl [-j jail] [-d] [-f filename] [-n nodename] [command [argument ...]]
Comment Actions
I didn't think the empty string would be passed along, but @kevans had it right. I moved the check to later before switching to jail.