- Notifications
You must be signed in to change notification settings - Fork1.9k
FreeBSD: Add zfs jail property#17768
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
under illumos based platform we have |
Thanks for taking this moment into consideration. Indeed, the cross-platform term in the codebase is |
man/man7/zfsprops.7 Outdated
| This read-only property reports the name of the jail that mounted the jailed | ||
| dataset. | ||
| The "0" name is used for datasets that are not mounted or not jailed. | ||
| If a jail is renamed, the property will still report its old name from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We should mention the reasoning for "0". What do you think of?:
The "0" name is used for datasets that are not mounted or not jailed.+ This differs from the normal ZFS convention to print dash ('-') for unset values,+ since '-' can be a valid jail name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is a great idea. Fixed, thanks.
"jail": {"value":"0","source": {"type":"LOCAL","data":"-" } } I would have expected
|
module/os/freebsd/zfs/zfs_vfsops.c Outdated
| zfsvfs->z_os->os_dsl_dataset->ds_jailname= | ||
| kmem_zalloc(strlen(pr->pr_name)+1,KM_SLEEP); | ||
| strcpy(zfsvfs->z_os->os_dsl_dataset->ds_jailname, | ||
| pr->pr_name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You can usekmem_strdup(pr->pr_name); to simplify this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Indeed. Fixed.
module/zfs/dsl_prop.c Outdated
| (ds->ds_jailname&&INGLOBALZONE(curproc)) ? | ||
| ds->ds_jailname :"0")); | ||
| VERIFY0(nvlist_add_string(propval,ZPROP_SOURCE,setpoint)); | ||
| VERIFY0(nvlist_add_nvlist(*nvp,"jail",propval)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thefnvlist_* wrappers should be used here, or better yet add the required error handling even if they're almost certain never to fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sure. It seems I followed the examples fromdsl_prop_get_all_impl() itself. Migrated tofnvlist_*, thanks.
A read-only property to report name of the jail that mounted thedataset.Sponsored-by: SkunkWerks, GmbHSigned-off-by: Igor Ostapenko <pm@igoro.pro>
Fixed.
Sure, especially that existing test provides a working baseline. I've added a separate test not to mix multiple concerns into a single one. |
| If the jail is renamed, the property will still report its old name from | ||
| the time the dataset was mounted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It makes me wonder if reporting jail ID would be more predictable.
| chards_snapname[ZFS_MAX_DATASET_NAME_LEN]; | ||
| #ifdef__FreeBSD__ | ||
| char*ds_jailname; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I am not familiar with zones, but wouldn't this functionality apply there too? Would someds_zonename without theifdef be more universal?
| structprison*pr=curthread->td_ucred->cr_prison; | ||
| if (pr!=&prison0) { | ||
| zfsvfs->z_os->os_dsl_dataset->ds_jailname= | ||
| kmem_strdup(pr->pr_name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Again, I am not closely familiar with the zone/jail code in ZFS, but I am not sure the delegation should necessarily be tied to a mounting. I suppose some dataset may not be mounted, but be controlled by the jail. Am I wrong?
Sponsored-by: SkunkWerks, GmbH
Motivation and Context
It targets#15710 feature request to add a read-only property to report name of the jail that mounted the dataset.
Description
The decisions made:
0name is used for datasets that are not mounted or not jailed. The default zfs style is to use-for a missing value, but it's allowed to have a jail named like that. At the same time there is no way to name a jail as0:jail: name cannot be numeric (unless it is the jid).allprison_lock) to lookup jail name by jid does not feel good for the cases with many jailed datasets and/or jails.0. Such way it covers the requirement not to reveal jail name to the jail itself.zfsprops.7man page was extended to outline the property. It is mentioned within the section of tunables even though it is a read-only one, it seems to be better to keep it closer to thejailedproperty.Hence, the mechanism is simple:
zfs_domount()cachespr_namezfs_umount()clears the nameThe proposed implementation covers usual cases without extra complexity and property retrieval performance impact. The opportunities for improvements are left for the future.
How Has This Been Tested?
Originally the patch was created months ago for 15-CURRENT, it was re-tested for the latest 16-CURRENT. The UI/UX demonstration:
Types of changes
Checklist:
Signed-off-by.