Page MenuHomeFreeBSD
Differential D59572

jail: define well-known parameter names in sys/jail.h
ClosedPublic
Actions

Authored by seuros on Thu, Sep 10, 2:36 PM.
Tags
None
Referenced Files
F171620875: D59572.id186386.diff
Sat, Sep 12, 5:57 AM
F171589111: D59572.diff
Fri, Sep 11, 11:05 PM
F171588876: D59572.diff
Fri, Sep 11, 11:02 PM
F171578788: D59572.id.diff
Fri, Sep 11, 9:17 PM
F171555270: D59572.id186408.diff
Fri, Sep 11, 5:57 PM
F171523162: D59572.id186406.diff
Fri, Sep 11, 2:15 PM
F171494839: D59572.id186405.diff
Fri, Sep 11, 11:25 AM
Unknown Object (File)
Fri, Sep 11, 7:45 AM
View All 13 Files
Subscribers
imp
kevans

Details

Reviewers
jamie
kib
markj
olce
adrian
Group Reviewers
Jails
Commits
rG1256447cbc21: jail: define well-known parameter names in sys/jail.h
Summary

Userland jail managers hardcode jail parameter name strings; nothing
in the headers has ever named them, even when sys/jail.h already
has JAIL_META_PRIVATE/JAIL_META_SHARED for the metadata pair.

Added JAIL_PARAM_* string constants for the static parameters registered
by the base kernel.

No functional change.

MFC After: 1 week

Diff Detail

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

Event Timeline

seuros created this revision.Thu, Sep 10, 2:36 PM
Herald added a subscriber: imp. · View Herald TranscriptThu, Sep 10, 2:36 PM
seuros requested review of this revision.Thu, Sep 10, 2:36 PM
Harbormaster completed remote builds in B76749: Diff 186386.Thu, Sep 10, 2:36 PM
adrian accepted this revision.Thu, Sep 10, 3:19 PM

i like this; it makes it much easier to write code that works by correctly using defines rather than hoping the strings match.

I'll approve it but let me first double check with phk or some other jails-y person before landing it.

This revision is now accepted and ready to land.Thu, Sep 10, 3:19 PM
adrian added a comment.Thu, Sep 10, 3:20 PM

(and subsequent commits should use these defines instead of hard-coded strings..)

seuros added a reviewer: Jails.Thu, Sep 10, 3:35 PM
jamie added a comment.Thu, Sep 10, 6:23 PM

It would make sense to broaden their use, with bare string literals also being used in vfs_copyopt and vfs_setopt calls.

seuros updated this revision to Diff 186405.Thu, Sep 10, 6:28 PM

Broaden constant use to vfs_copyopt/vfs_setopt call sites and the jail(2) compat iovecs, per jamie. Adds JAIL_PARAM_DESC/LASTJID/NODYING for the pseudo-params those paths use.

This revision now requires review to proceed.Thu, Sep 10, 6:28 PM
Harbormaster completed remote builds in B76753: Diff 186405.Thu, Sep 10, 6:28 PM
adrian added inline comments.Thu, Sep 10, 6:34 PM
sys/sys/param.h
77 ↗(On Diff #186405)

Why do we need to bump version here?

jamie added a comment.Thu, Sep 10, 6:35 PM

Now that I think about it, jail(8) and jls(8) code is also rife with these strings.

adrian added a comment.Thu, Sep 10, 6:38 PM
In D59572#1366963, @jamie wrote:

Now that I think about it, jail(8) and jls(8) code is also rife with these strings.

Right; we can fix those but after this commit. This commit doesn't change their values, it just adds a level of indirection to them.

seuros updated this revision to Diff 186406.Thu, Sep 10, 6:50 PM
seuros marked an inline comment as done.

refactor userland

Harbormaster completed remote builds in B76754: Diff 186406.Thu, Sep 10, 6:50 PM
adrian added a comment.Thu, Sep 10, 6:52 PM

nooooo please do the userland refactor as a follow-up commit. The kernel refactor doesnt change the string contents at all.

seuros added a comment.Thu, Sep 10, 6:54 PM
In D59572#1366964, @adrian wrote:

Right; we can fix those but after this commit. This commit doesn't change their values, it just adds a level of indirection to them.

I was testing the world , that why i pushed the version bump. I reverted the version bump.

The headers only didn't need it, but now we have the full refactoring.

This change also can be backported to 15.

seuros added a comment.Thu, Sep 10, 6:54 PM
In D59572#1366984, @adrian wrote:

nooooo please do the userland refactor as a follow-up commit. The kernel refactor doesnt change the string contents at all.

Ok.

seuros updated this revision to Diff 186407.Thu, Sep 10, 6:57 PM

revert userland

Harbormaster completed remote builds in B76755: Diff 186407.Thu, Sep 10, 6:57 PM
jamie accepted this revision.Thu, Sep 10, 6:59 PM
This revision is now accepted and ready to land.Thu, Sep 10, 6:59 PM
adrian accepted this revision.Thu, Sep 10, 7:01 PM

thanks! lets clean up userland after this.

(the only reason we'd bump version is if we started to write ports/external software that uses these..)

Closed by commit rG1256447cbc21: jail: define well-known parameter names in sys/jail.h (authored by seuros). · Explain WhyThu, Sep 10, 7:04 PM
This revision was automatically updated to reflect the committed changes.
seuros added a commit: rG1256447cbc21: jail: define well-known parameter names in sys/jail.h.
kevans added a subscriber: kevans.Thu, Sep 10, 11:49 PM
In D59572#1366987, @seuros wrote:
In D59572#1366964, @adrian wrote:

Right; we can fix those but after this commit. This commit doesn't change their values, it just adds a level of indirection to them.

I was testing the world , that why i pushed the version bump. I reverted the version bump.

Can you expand on this a little more? You shouldn't have needed a version bump for buildworld, for instance, because clang should have emitted the header as a dependency that bmake acknowledges would made the including object files out of date (even without metamode or anything fancy)?

seuros added a comment.Fri, Sep 11, 8:06 AM

Can you expand on this a little more? You shouldn't have needed a version bump for buildworld, for instance, because clang should have emitted the header as a dependency that bmake acknowledges would made the including object files out of date (even without metamode or anything fancy)?

right, but the bump for my workspace, i had other unrelated commit prior that needed invalidation.
So when i was asked to push a fix, i committed that extra local file by mistake.

Revision Contents
Changeset List

PathSize
sys/
kern/
67 lines
sys/
80 lines

Diff 186386

View Options

sys/kern/kern_jail.c

Loading...
View Options

sys/sys/jail.h

Loading...