Page MenuHomeFreeBSD
Differential D40213

Teach ifconfig to attach and run itself in a jail.
ClosedPublic
Actions

Authored by nyan_myuji.xyz on May 22 2023, 7:37 PM.
Tags
None
Referenced Files
F81889490: D40213.diff
Thu, Nov 28, 6:30 PM
Unknown Object (File)
Thu, Nov 14, 9:35 AM
Unknown Object (File)
Wed, Nov 13, 9:46 PM
Unknown Object (File)
Wed, Nov 13, 7:16 AM
Unknown Object (File)
Wed, Nov 13, 4:27 AM
Unknown Object (File)
Tue, Nov 12, 12:46 PM
Unknown Object (File)
Tue, Nov 12, 10:24 AM
Unknown Object (File)
Tue, Nov 12, 10:14 AM
View All Files
Subscribers
dfr
imp
melifaro

Details

Reviewers
emaste
lwhsu
khng
melifaro
Group Reviewers
Jails
Commits
rG3af770071046: ifconfig(8): Teach ifconfig to attach and run itself in a jail
rG7e49aa86a282: ifconfig(8): Teach ifconfig to attach and run itself in a jail
Summary

Add -j <jail> flag to ifconfig to allow ifconfig to attach and run inside a
jail. This allow parent to configure network interfaces of its children
even if ifconfig is not available in child's tree (e.g. Linux Jails)

Event: Kitchener-Waterloo Hackathon 202305

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 51607
Build 48498: arc lint + arc unit

Event Timeline

nyan_myuji.xyz created this revision.May 22 2023, 7:37 PM
Herald added a subscriber: imp. · View Herald TranscriptMay 22 2023, 7:37 PM
nyan_myuji.xyz requested review of this revision.May 22 2023, 7:37 PM
Harbormaster completed remote builds in B51607: Diff 122231.May 22 2023, 7:37 PM
nyan_myuji.xyz added reviewers: emaste, lwhsu, khng.May 22 2023, 7:39 PM
nyan_myuji.xyz updated this revision to Diff 122237.May 22 2023, 7:53 PM
  • fix rescue build
Harbormaster completed remote builds in B51609: Diff 122237.May 22 2023, 7:53 PM
nyan_myuji.xyz edited the summary of this revision. (Show Details)May 22 2023, 7:55 PM
khng added inline comments.May 22 2023, 8:05 PM
sbin/ifconfig/ifconfig.c
469

The following sounds more idiomatic to me:

jid = jail_getid(optarg);
if (jid == -1)

Perror("1");

if (jail_attach(jid) != 0)

Perror("2");
nyan_myuji.xyz added inline comments.May 22 2023, 8:10 PM
sbin/ifconfig/ifconfig.c
469

Great! I prefer that too just worry about breaking style.

nyan_myuji.xyz updated this revision to Diff 122241.May 22 2023, 8:11 PM
  • improve readability
Harbormaster completed remote builds in B51611: Diff 122241.May 22 2023, 8:11 PM
nyan_myuji.xyz marked an inline comment as done.May 22 2023, 8:11 PM
khng accepted this revision.May 22 2023, 8:13 PM

LGTM. You may want to add a manual page changes as well.

This revision is now accepted and ready to land.May 22 2023, 8:13 PM
emaste added a comment.May 22 2023, 8:26 PM

yes let's add the man page in here too

sbin/ifconfig/ifconfig.c
474

Do we want

#else
    Perror("not built with jail support")

or leave j out of options when not #ifdef JAIL?

nyan_myuji.xyz updated this revision to Diff 122254.May 22 2023, 10:23 PM
  • modify man page to refactor the new -j flag
This revision now requires review to proceed.May 22 2023, 10:23 PM
Harbormaster completed remote builds in B51616: Diff 122254.May 22 2023, 10:23 PM
lwhsu added inline comments.May 22 2023, 10:27 PM
sbin/ifconfig/ifconfig.8
31 ↗(On Diff #122254)

Bump the date. :-)

lwhsu added inline comments.May 22 2023, 10:29 PM
sbin/ifconfig/ifconfig.c
474

Maybe perror() is easier since going the other way, we also need to have usage() #ifdef'd

lwhsu added a comment.May 22 2023, 10:31 PM

If would be perfect if you want to add a simple test under tests/

nyan_myuji.xyz updated this revision to Diff 122261.May 22 2023, 10:31 PM
  • print error and exit if -j specified but ifconfig isn't built with jail support
  • bump manpage date
Harbormaster completed remote builds in B51622: Diff 122261.May 22 2023, 10:31 PM
lwhsu added a comment.May 22 2023, 10:42 PM

BTW, I'm not sure if doing jail_attach() in args_parse() is a common or good practice.

nyan_myuji.xyz updated this revision to Diff 122262.May 22 2023, 11:13 PM

Move jail_attach away from args_parse

Harbormaster completed remote builds in B51623: Diff 122262.May 22 2023, 11:13 PM
nyan_myuji.xyz updated this revision to Diff 122263.May 22 2023, 11:16 PM

cleanup

Harbormaster completed remote builds in B51624: Diff 122263.May 22 2023, 11:16 PM
nyan_myuji.xyz updated this revision to Diff 122264.May 22 2023, 11:18 PM

including the correct range

Harbormaster completed remote builds in B51625: Diff 122264.May 22 2023, 11:18 PM
khng accepted this revision.May 23 2023, 2:35 AM
This revision is now accepted and ready to land.May 23 2023, 2:35 AM
lwhsu added a reviewer: Jails.May 23 2023, 6:49 PM
melifaro accepted this revision.May 23 2023, 7:34 PM
melifaro added a subscriber: melifaro.
melifaro added inline comments.
sbin/ifconfig/ifconfig.c
444

Nit: this is not needed, args is zeroed by default.

nyan_myuji.xyz updated this revision to Diff 122337.May 23 2023, 7:43 PM
  • remove explicit initialization
This revision now requires review to proceed.May 23 2023, 7:43 PM
Harbormaster completed remote builds in B51654: Diff 122337.May 23 2023, 7:44 PM
emaste accepted this revision as: emaste.May 23 2023, 7:45 PM
This revision is now accepted and ready to land.May 23 2023, 7:45 PM
Closed by commit rG7e49aa86a282: ifconfig(8): Teach ifconfig to attach and run itself in a jail (authored by nyan_myuji.xyz, committed by lwhsu). · Explain WhyMay 23 2023, 8:57 PM
This revision was automatically updated to reflect the committed changes.
lwhsu added a commit: rG7e49aa86a282: ifconfig(8): Teach ifconfig to attach and run itself in a jail.
dfr added a subscriber: dfr.Jun 1 2023, 7:31 AM

It would be nice to have something similar for route. Setting up network state for containers with external connectivity also needs some way of setting a default route. For podman, I ended up nesting the container jail inside another jail where I can safely run ifconfig and route without requiring their presence inside the container (or trusting the container's binaries if they are present).

nyan_myuji.xyz added a comment.Jun 1 2023, 2:48 PM

@dfr A patch for route is available at D40377

dfr added a comment.Aug 13 2023, 10:15 AM

Would there be any objection to me merging this change and the related change in D40377 to stable/13? It would make setting up container networking for containerd and nerdctl much simpler. For podman, it could also make networking more efficient - we currently use two jails per container with one solely used to own the container vnet while also allowing access to host networking tools.

melifaro added a comment.Aug 13 2023, 10:48 AM
In D40213#943667, @dfr wrote:

Would there be any objection to me merging this change and the related change in D40377 to stable/13? It would make setting up container networking for containerd and nerdctl much simpler. For podman, it could also make networking more efficient - we currently use two jails per container with one solely used to own the container vnet while also allowing access to host networking tools.

No objection, the only note is that ifconfig(8) code is different in stable/13, so likely it would require some changes.

dfr added a commit: rG3af770071046: ifconfig(8): Teach ifconfig to attach and run itself in a jail.Aug 16 2023, 12:32 PM

Revision Contents
Changeset List

PathSize
sbin/
ifconfig/
26 lines

Diff 122231

View Options

sbin/ifconfig/ifconfig.c

Loading...