Page MenuHomeFreeBSD

Allow default mount options for ZFS
AcceptedPublic

Authored by sef on Feb 16 2020, 10:03 PM.

Details

Reviewers
allanjude
mmacy
delphij
Group Reviewers
ZFS
Summary

As a follow-on to review D21458 (from last summer), here is a way to use it. Specifically, this adds a new sysctl, "vfs.zfs.mount_options", which will be prepended to mount options; when I set it to "nocover,emptydir" then it won't import a pool if the specified mount directory is already a mount point,

Note that this does require the zfs module be loaded first.

I have not (yet) run this by the ZoLoF people yet. I also haven't made a man page change for it yet; I'm not sure which man page would be?

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

sef created this revision.Feb 16 2020, 10:03 PM
delphij requested changes to this revision.Feb 17 2020, 1:16 AM

Some minor nits.

cddl/contrib/opensolaris/cmd/zfs/zfs_main.c
6979

this if..else block should be enclosed in a if (tmp != NULL) condition because tmp can be NULL.

cddl/contrib/opensolaris/lib/libzfs/common/libzfs_mount.c
382

I think it would be safer to do:

if (sysctlbyname("vfs.zfs.mount_options", NULL, &len, NULL, 0) != 0)
    return NULL;

here.

386

retval should be checked before proceeding. Assuming malloc failed and returned NULL, we are essentially doing the same sysctlbyname above here, and would get 0, then write to the reserved block.

455

this if..else block should be enclosed in a if (tmp != NULL) condition because tmp can be NULL.

This revision now requires changes to proceed.Feb 17 2020, 1:16 AM
sef marked 3 inline comments as done.Feb 17 2020, 1:36 AM

New version coming now. Thanks!

cddl/contrib/opensolaris/lib/libzfs/common/libzfs_mount.c
386

I changed up this code a bit. I'd actually had two implementations of the function at one point, and deleted the wrong one -- correct one uses strlcat and strlcpy, and more checks.

sef updated this revision to Diff 68434.Feb 17 2020, 1:37 AM
delphij added inline comments.Feb 17 2020, 4:58 AM
cddl/contrib/opensolaris/cmd/zfs/zfs_main.c
6985

Took a closer look at the code. It looks like mntopts is only used when the file system is not mount=legacy (this codepath). It seems that you can move this whole code block above (line 6917), keeping only:

char *default_mount_opts = freebsd_default_mount_options(zhp);

if (default_mount_opts != NULL) {
	strlcpy(mntopts, default_mount_opts, sizeof(mntopts));
}
free(default_mount_opts);

and in line 6922, change the code to:

	if (mntopts[0] != '\0') {
		strlcat(mntopts, ",", sizeof(mntopts));
	}
	strlcat(mntopts, optarg, sizeof(mntopts));

?

(Note that this changes -o semantics slightly, previously specifying it multiple times, the last one wins, now every time it appends new options; the alternative would be to save the optarg pointer and perform the appending after getopt() loop.)

sef added a reviewer: ZFS.Feb 24 2020, 7:13 AM

Ping anyone?

cddl/contrib/opensolaris/cmd/zfs/zfs_main.c
6985

Or just keep the code as I have it; this means that the new code isn't invoked except in the legacy case, and doesn't change the semantics of the -o option...

delphij accepted this revision.Feb 25 2020, 2:06 AM
This revision is now accepted and ready to land.Feb 25 2020, 2:06 AM
sef added a comment.Mar 8 2020, 12:49 AM

I have not forgotten about this; I'm working on another aspect of this (dataset properties), and am holding off on committing this a bit.