From c63aabaf1c548248c8a5081f6014a472dc7c4570 Mon Sep 17 00:00:00 2001 From: Umer Saleem Date: Tue, 5 Sep 2023 13:33:58 +0500 Subject: [PATCH] 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. Reviewed-by: Brian Behlendorf Reviewed-by: Alexander Motin Reviewed-by: Ameer Hamza Signed-off-by: Umer Saleem Closes #15240 --- 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); } /*