From 3d5ae103b319026fe1e9979f85d1263fb9f558d6 Mon Sep 17 00:00:00 2001 From: Umer Saleem Date: Tue, 5 Sep 2023 13:27:53 +0500 Subject: [PATCH 1/2] Update the behavior of mountpoint property There are some inconsistencies in the handling of mountpoint property. This commit updates the behavior and makes it consistent. If mountpoint property is set when dataset is unmounted, this would update the mountpoint property. The mountpoint could be valid or invalid in this case. Setting the mountpoint property would result in success in this case. Dataset would still be unmounted here. On the other hand, if dataset is mounted and mountpoint property is updated to something invalid where mount cannot be successful, for example, setting the mountpoint inside a readonly directory. This would unmount the dataset, set the mountpoint property to requested value and tries to mount the dataset. The mount operation returns error and this error is treated as overall failure of setting the property while the property is actually set. To make the behavior consistent in case dataset is mounted or unmounted, we should try to mount the dataset whenever mountpoint property is updated. This would result in mounting the datasets if canmount property is set to on, regardless if the dataset was previously unmounted. The failure in mount operation while setting the mountpoint property should not be treated as failure, since the property is actually set now to user requested value. Signed-off-by: Umer Saleem --- cmd/zfs/zfs_main.c | 7 ++++--- lib/libzfs/libzfs_changelist.c | 8 ++++---- .../cli_root/zfs_mount/zfs_mount_006_pos.ksh | 2 +- .../cli_root/zfs_mount/zfs_mount_008_pos.ksh | 4 ++-- .../cli_root/zfs_mount/zfs_mount_012_pos.ksh | 15 +++++++-------- .../cli_root/zfs_set/mountpoint_002_pos.ksh | 12 +++++++++--- .../cli_root/zfs_set/zfs_set_003_neg.ksh | 10 +++++++--- 7 files changed, 34 insertions(+), 24 deletions(-) diff --git a/cmd/zfs/zfs_main.c b/cmd/zfs/zfs_main.c index 629e048872bd..45cd2998d6e9 100644 --- a/cmd/zfs/zfs_main.c +++ b/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 " @@ -4217,11 +4218,11 @@ set_callback(zfs_handle_t *zhp, void *data) case EZFS_SHARENFSFAILED: (void) fprintf(stderr, gettext("property may be set " "but unable to reshare filesystem\n")); + ret = 1; break; } - return (1); } - return (0); + return (ret); } static int diff --git a/lib/libzfs/libzfs_changelist.c b/lib/libzfs/libzfs_changelist.c index dd14c570ec03..4b0f66964346 100644 --- a/lib/libzfs/libzfs_changelist.c +++ b/lib/libzfs/libzfs_changelist.c @@ -244,13 +244,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; } diff --git a/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_006_pos.ksh b/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_006_pos.ksh index 2a2466f65c02..e9ab472795eb 100755 --- a/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_006_pos.ksh +++ b/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 diff --git a/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_008_pos.ksh b/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_008_pos.ksh index 2c1029d551cf..0437c61a2c40 100755 --- a/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_008_pos.ksh +++ b/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. diff --git a/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_012_pos.ksh b/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_012_pos.ksh index 5ff094d2c479..66958f2f0884 100755 --- a/tests/zfs-tests/tests/functional/cli_root/zfs_mount/zfs_mount_012_pos.ksh +++ b/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 diff --git a/tests/zfs-tests/tests/functional/cli_root/zfs_set/mountpoint_002_pos.ksh b/tests/zfs-tests/tests/functional/cli_root/zfs_set/mountpoint_002_pos.ksh index a5785226e02e..c227a6fb8aa8 100755 --- a/tests/zfs-tests/tests/functional/cli_root/zfs_set/mountpoint_002_pos.ksh +++ b/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 diff --git a/tests/zfs-tests/tests/functional/cli_root/zfs_set/zfs_set_003_neg.ksh b/tests/zfs-tests/tests/functional/cli_root/zfs_set/zfs_set_003_neg.ksh index 3afb0eb7010e..5901ba7dc461 100755 --- a/tests/zfs-tests/tests/functional/cli_root/zfs_set/zfs_set_003_neg.ksh +++ b/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" From c3762e81621b622dfa2f1850f430b2f5b2eb99f4 Mon Sep 17 00:00:00 2001 From: Umer Saleem Date: Tue, 5 Sep 2023 13:33:58 +0500 Subject: [PATCH 2/2] Improve the handling of sharesmb,sharenfs properties For sharesmb and sharenfs properties, the status of setting the property is tied with whether we succeed to share the dataset or not. In case sharing the dataset is not successful, this is treated as overall failure of setting the property. In this case, if we check the property after the failure, it is set to on. This commit updates this behavior and the status of setting the share properties is not returned as failure, when we fail to share the dataset. For sharenfs property, if access list is provided, the syntax errors in access list/host adresses are not validated until after setting the property during postfix phase while trying to share the dataset. This is not correct, since the property has already been set when we reach there. Syntax errors in access list/host addresses are validated while validating the property list, before setting the property and failure is returned to user in this case when there are errors in access list. Signed-off-by: Umer Saleem --- cmd/zfs/zfs_main.c | 1 - lib/libshare/os/freebsd/nfs.c | 3 ++- lib/libshare/os/linux/nfs.c | 47 +++++++++++++++++++++++++++++++--- lib/libzfs/libzfs_changelist.c | 11 ++++---- 4 files changed, 51 insertions(+), 11 deletions(-) diff --git a/cmd/zfs/zfs_main.c b/cmd/zfs/zfs_main.c index 45cd2998d6e9..76c82fd53217 100644 --- a/cmd/zfs/zfs_main.c +++ b/cmd/zfs/zfs_main.c @@ -4218,7 +4218,6 @@ set_callback(zfs_handle_t *zhp, void *data) case EZFS_SHARENFSFAILED: (void) fprintf(stderr, gettext("property may be set " "but unable to reshare filesystem\n")); - ret = 1; break; } } diff --git a/lib/libshare/os/freebsd/nfs.c b/lib/libshare/os/freebsd/nfs.c index 521631c51f07..d9fc66106369 100644 --- a/lib/libshare/os/freebsd/nfs.c +++ b/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); } diff --git a/lib/libshare/os/linux/nfs.c b/lib/libshare/os/linux/nfs.c index c27e5564c1e1..004946b0cfe4 100644 --- a/lib/libshare/os/linux/nfs.c +++ b/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); diff --git a/lib/libzfs/libzfs_changelist.c b/lib/libzfs/libzfs_changelist.c index 4b0f66964346..efe1c0c06035 100644 --- a/lib/libzfs/libzfs_changelist.c +++ b/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; @@ -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); } /*