In D50244#1156230, @bz wrote:

I do understand the problem. I just think the name is a bit futuristic and not very descriptive for what it is doing.

I'm calling this "done" but I'm still open to a name change.

remove NG_WORMHOLE_WARP from ng_wormhole.h as its not needed and in fact can't be referred to this way.

Marking changes addressed done. Just realized I still need to get "warp" and its define moved to C file.

Just using VIMAGE in the Makefile didn't work but I noticed sys/modules/Makefile had an example that does work for wtap. I also stole the assert from sys/dev/wtap/if_wtap.c.

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.

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 ...]]

Document that you must leave the jail empty to indicate the jail where you run ngportal.

Allow a jail to be specified numerically to the "open" command.

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.

In D50244#1153881, @imp wrote:

What is the use case for this?

Fixed man page to be mandoc -Tlint compliant.
Saw a style issue in Usage() and fixed it.
parse_spec() was giving errors about "component[x]" which has no meaning at all to users of ngportal and was changed to display the component with an error which should be much more helpful.

I saw that other man pages used the simplified license so I changed this to do the same.

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.

Fix source file license per @ziaee (comment was in D50241).

Fix source file license per @ziaee (comment was in D50241).

marking changes done.

Address comments so far from @kevans and @zlei
The man page ordering was -f then -j but I had to refactor for correct behaviour as @zlei pointed out and so I added to manpage in the same way as sysctl so user knows file is not in jail even with -j option.

@imp has historically said we should avoid changing it for existing files without signoff from copyright holders.

In D50241#1146027, @ziaee wrote:

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.

As author I don't think this provides enough new utility to make it worth pursuing.

I think if I had seen this post (1st reply by PMc) showing the "echo trick" I wouldn't have put this up for review.

I vastly under appreciated that folks rely upon the ng_eiface not moving with the struct ifnet. Probably because I've been using them in jails for over a decade and only recently noticed myself.

I think I'll just accept my understanding is flawed now to save time and withdraw this. Thank you.

add asserts as suggested.

I removed the "black magic" I added of relying upon alloc_unr_specific to return -1 when a user requested uplink0.
What was there before is more obvious and reads almost like english:

if (linkNum == 0 && isUplink)
        return (EINVAL);

That says exactly why we return EINVAL. Also I still get benefit of initializing unr(9) to start at 1 in the code. No longer have to grab a number and make sure its at least 1 for uplink.

I'm not sure how to clear the comments as they are often on wrong line numbers now. Sorry about that.

I have incorporated all the suggestions and uplink has a separate unr(9) now. Note the bhyve change is removed entirely as it is not necessary, you can still use peerhook=link with bhyve though for it to auto assign.

I have to clean up my patch and figure out how to update (I'm new to this). I will be removing bhyve from this so apologies to all the bhyve folks that got tagged on this.

Actually before I go move things to newhook can I ask do we have consensus that I should just have a separate unr(9) for uplink?
Also, the bhyve works without having it "figure out" it has an ng_bridge(4). You can pass "link" to it and it does the right thing. Given that should I pull bhyve change out so the patch is more focused on ng_bridge(4) which is my main concern?

In D44615#1017240, @glebius wrote:

Now more fundamental question, that you probably have answer for, cause you did it the way you did: why can't we alloc_unr() and possible return EEXIST in the (almost) synchronous newhook method? Why do we postpone that to later connect method?

In D44615#1017227, @afedorov wrote:

My main objection is that backward compatibility is broken. We can no longer have uplink[N] and link[N] at the same time. This may break existing software.

I had the same concern and point this out in the bug report. I am still not sure why uplink0 was never allowed, but kept that restriction. uplink is for when you attach a real network interface, an ng_ether(4) and although netgraph makes just about anything possible, I wonder if anybody does have more than one uplink on an ng_bridge(4)?