Improve the handling of mountpoint, sharesmb and sharenfs properties

cmd/zfs/zfs_main.c

@@ -4207,8 +4207,9 @@ static int
 set_callback(zfs_handle_t *zhp, void *data)
 {
 	nvlist_t *props = data;
+	int ret = zfs_prop_set_list(zhp, props);
 
-	if (zfs_prop_set_list(zhp, props) != 0) {
+	if (ret != 0 || libzfs_errno(g_zfs) != EZFS_SUCCESS) {
 		switch (libzfs_errno(g_zfs)) {
 		case EZFS_MOUNTFAILED:
 			(void) fprintf(stderr, gettext("property may be set "
@@ -4219,9 +4220,8 @@ set_callback(zfs_handle_t *zhp, void *data)
 			    "but unable to reshare filesystem\n"));
 			break;
 		}
-		return (1);
 	}
-	return (0);
+	return (ret);
 }
 
 static int

lib/libshare/os/freebsd/nfs.c

@@ -161,7 +161,8 @@ nfs_is_shared(sa_share_impl_t impl_share)
 static int
 nfs_validate_shareopts(const char *shareopts)
 {
-	(void) shareopts;
+	if (strlen(shareopts) == 0)
+		return (SA_SYNTAX_ERR);
 	return (SA_OK);
 }
 

lib/libshare/os/linux/nfs.c

@@ -319,12 +319,49 @@ get_linux_shareopts_cb(const char *key, const char *value, void *cookie)
 	    "wdelay" };
 
 	char **plinux_opts = (char **)cookie;
+	char *host, *val_dup, *literal, *next;
 
-	/* host-specific options, these are taken care of elsewhere */
-	if (strcmp(key, "ro") == 0 || strcmp(key, "rw") == 0 ||
-	    strcmp(key, "sec") == 0)
+	if (strcmp(key, "sec") == 0)
 		return (SA_OK);
 
+	if (strcmp(key, "ro") == 0 || strcmp(key, "rw") == 0) {
+		if (value == NULL || strlen(value) == 0)
+			return (SA_OK);
+		val_dup = strdup(value);
+		host = val_dup;
+		if (host == NULL)
+			return (SA_NO_MEMORY);
+		do {
+			if (*host == '[') {
+				host++;
+				literal = strchr(host, ']');
+				if (literal == NULL) {
+					free(val_dup);
+					return (SA_SYNTAX_ERR);
+				}
+				if (literal[1] == '\0')
+					next = NULL;
+				else if (literal[1] == '/') {
+					next = strchr(literal + 2, ':');
+					if (next != NULL)
+						++next;
+				} else if (literal[1] == ':')
+					next = literal + 2;
+				else {
+					free(val_dup);
+					return (SA_SYNTAX_ERR);
+				}
+			} else {
+				next = strchr(host, ':');
+				if (next != NULL)
+					++next;
+			}
+			host = next;
+		} while (host != NULL);
+		free(val_dup);
+		return (SA_OK);
+	}
+
 	if (strcmp(key, "anon") == 0)
 		key = "anonuid";
 
@@ -472,6 +509,10 @@ static int
 nfs_validate_shareopts(const char *shareopts)
 {
 	char *linux_opts = NULL;
+
+	if (strlen(shareopts) == 0)
+		return (SA_SYNTAX_ERR);
+
 	int error = get_linux_shareopts(shareopts, &linux_opts);
 	if (error != SA_OK)
 		return (error);

lib/libzfs/libzfs_changelist.c

@@ -174,7 +174,6 @@ changelist_postfix(prop_changelist_t *clp)
 	prop_changenode_t *cn;
 	uu_avl_walk_t *walk;
 	char shareopts[ZFS_MAXPROPLEN];
-	int errors = 0;
 	boolean_t commit_smb_shares = B_FALSE;
 	boolean_t commit_nfs_shares = B_FALSE;
 
@@ -244,13 +243,13 @@ changelist_postfix(prop_changelist_t *clp)
 		    zfs_is_mounted(cn->cn_handle, NULL);
 
 		if (!mounted && !needs_key && (cn->cn_mounted ||
-		    ((sharenfs || sharesmb || clp->cl_waslegacy) &&
+		    (((clp->cl_prop == ZFS_PROP_MOUNTPOINT &&
+		    clp->cl_prop == clp->cl_realprop) ||
+		    sharenfs || sharesmb || clp->cl_waslegacy) &&
 		    (zfs_prop_get_int(cn->cn_handle,
 		    ZFS_PROP_CANMOUNT) == ZFS_CANMOUNT_ON)))) {
 
-			if (zfs_mount(cn->cn_handle, NULL, 0) != 0)
-				errors++;
-			else
+			if (zfs_mount(cn->cn_handle, NULL, 0) == 0)
 				mounted = TRUE;
 		}
 
@@ -262,19 +261,19 @@ changelist_postfix(prop_changelist_t *clp)
 		const enum sa_protocol nfs[] =
 		    {SA_PROTOCOL_NFS, SA_NO_PROTOCOL};
 		if (sharenfs && mounted) {
-			errors += zfs_share(cn->cn_handle, nfs);
+			zfs_share(cn->cn_handle, nfs);
 			commit_nfs_shares = B_TRUE;
 		} else if (cn->cn_shared || clp->cl_waslegacy) {
-			errors += zfs_unshare(cn->cn_handle, NULL, nfs);
+			zfs_unshare(cn->cn_handle, NULL, nfs);
 			commit_nfs_shares = B_TRUE;
 		}
 		const enum sa_protocol smb[] =
 		    {SA_PROTOCOL_SMB, SA_NO_PROTOCOL};
 		if (sharesmb && mounted) {
-			errors += zfs_share(cn->cn_handle, smb);
+			zfs_share(cn->cn_handle, smb);
 			commit_smb_shares = B_TRUE;
 		} else if (cn->cn_shared || clp->cl_waslegacy) {
-			errors += zfs_unshare(cn->cn_handle, NULL, smb);
+			zfs_unshare(cn->cn_handle, NULL, smb);
 			commit_smb_shares = B_TRUE;
 		}
 	}
@@ -288,7 +287,7 @@ changelist_postfix(prop_changelist_t *clp)
 	zfs_commit_shares(proto);
 	uu_avl_walk_end(walk);
 
-	return (errors ? -1 : 0);
+	return (0);
 }
 
 /*

tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_006_pos.ksh

@@ -94,7 +94,7 @@ while (( depth < MAXDEPTH )); do
 done
 
 log_must zfs set mountpoint=$mtpt $TESTPOOL/$TESTFS
-log_must zfs $mountcmd $TESTPOOL/$TESTFS
+log_must ismounted $TESTPOOL/$TESTFS
 
 log_must zfs set overlay=off $TESTPOOL/$TESTFS
 if ! is_illumos; then

tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_008_pos.ksh

@@ -71,7 +71,7 @@ log_must mkfile 1M $testfile $testfile1
 
 log_must zfs unmount $fs1
 log_must zfs set mountpoint=$mntpnt $fs1
-log_must zfs mount $fs1
+log_must ismounted $fs1
 log_must zfs unmount $fs1
 log_must zfs mount -O $fs1
 
@@ -85,7 +85,7 @@ log_must ls $mntpnt/$TESTFILE1 $mntpnt/$TESTFILE2
 # Verify $TESTFILE2 was created in $fs1, rather than $fs
 log_must zfs unmount $fs1
 log_must zfs set mountpoint=$mntpnt1 $fs1
-log_must zfs mount $fs1
+log_must ismounted $fs1
 log_must ls $testfile1 $mntpnt1/$TESTFILE2
 
 # Verify $TESTFILE2 was not created in $fs, and $fs is accessible again.

tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_012_pos.ksh

@@ -25,13 +25,12 @@
 # STRATEGY:
 # 1. Unmount the dataset
 # 2. Create a new empty directory
-# 3. Set the dataset's mountpoint
-# 4. Attempt to mount the dataset
-# 5. Verify the mount succeeds
-# 6. Unmount the dataset
-# 7. Create a file in the directory created in step 2
-# 8. Attempt to mount the dataset
-# 9. Verify the mount succeeds
+# 3. Set the dataset's mountpoint, this should mount the dataset
+# 4. Verify the mount succeeds
+# 5. Unmount the dataset
+# 6. Create a file in the directory created in step 2
+# 7. Attempt to mount the dataset
+# 8. Verify the mount succeeds
 #
 
 verify_runnable "both"
@@ -43,7 +42,7 @@ fs=$TESTPOOL/$TESTFS
 log_must zfs umount $fs
 log_must mkdir -p $TESTDIR
 log_must zfs set mountpoint=$TESTDIR $fs
-log_must zfs mount $fs
+log_must ismounted $fs
 log_must zfs umount $fs
 log_must touch $TESTDIR/testfile.$$
 log_must zfs mount $fs

tests/zfs-tests/tests/functional/cli_root/zfs_set/mountpoint_002_pos.ksh

@@ -35,7 +35,9 @@
 #
 # DESCRIPTION:
 #	If ZFS is currently managing the file system but it is currently unmounted,
-#	and the mountpoint property is changed, the file system remains unmounted.
+#	and the mountpoint property is changed, the file system should be mounted
+#	if it is a valid mountpoint and canmount allows to mount, otherwise it
+#	should not be mounted.
 #
 # STRATEGY:
 # 1. Setup a pool and create fs, ctr within it.
@@ -62,7 +64,7 @@ function cleanup
 }
 
 log_assert "Setting a valid mountpoint for an unmounted file system, \
-	it remains unmounted."
+	it gets mounted."
 log_onexit cleanup
 
 old_fs_mpt=$(get_prop mountpoint $TESTPOOL/$TESTFS)
@@ -83,7 +85,11 @@ while (( i < ${#dataset[@]} )); do
 	while (( j < ${#values[@]} )); do
 		set_n_check_prop "${values[j]}" "mountpoint" \
 			"${dataset[i]}"
-		log_mustnot ismounted ${dataset[i]}
+		if [ "${dataset[i]}" = "$TESTPOOL/$TESTFS" ]; then
+			log_must ismounted ${dataset[i]}
+		else
+			log_mustnot ismounted ${dataset[i]}
+		fi
 		(( j += 1 ))
 	done
 	cleanup

tests/zfs-tests/tests/functional/cli_root/zfs_set/zfs_set_003_neg.ksh

@@ -33,7 +33,9 @@
 
 #
 # DESCRIPTION:
-# 'zfs set mountpoint/sharenfs' should fail when the mountpoint is invalid
+# 'zfs set mountpoint/sharenfs' should set the property when mountpoint
+#  is invalid. Setting the property should be successful, but dataset
+# should not be mounted, as mountpoint is invalid.
 #
 # STRATEGY:
 # 1. Create invalid scenarios
@@ -62,10 +64,12 @@ longpath=$(gen_dataset_name 1030 "abcdefg")
 log_must zfs create -o mountpoint=legacy $TESTPOOL/foo
 
 # Do the negative testing about "property may be set but unable to remount filesystem"
-log_mustnot eval "zfs set mountpoint=$badpath $TESTPOOL/foo >/dev/null 2>&1"
+set_n_check_prop "$badpath" "mountpoint" "$TESTPOOL/foo"
+log_mustnot ismounted $TESTPOOL/foo
 
 # Do the negative testing about "property may be set but unable to reshare filesystem"
-log_mustnot eval "zfs set sharenfs=on $TESTPOOL/foo >/dev/null 2>&1"
+set_n_check_prop "on" "sharenfs" "$TESTPOOL/foo"
+log_mustnot ismounted $TESTPOOL/foo
 
 # Do the negative testing about "sharenfs property can not be set to null"
 log_mustnot eval "zfs set sharenfs= $TESTPOOL/foo >/dev/null 2>&1"