6493853 sharemgr: inconsistent messages if parent is shared with UFS and ZFS

6499968 sharemgr: timing window can cause problems during system start if dfstab was modified
6502752 sharemgr: nfs_get_root_principal: bad host name
6510387 sharemgr: add-share prints error
6511920 sharemgr: set with -S krb5 optionset sets the group to nfs:sys
6513576 sharemgr: under some error conditions can't remove a share

4 files changed, 394 insertions, 161 deletions

diff --git a/usr/src/cmd/dfs.cmds/sharemgr/commands.c b/usr/src/cmd/dfs.cmds/sharemgr/commands.c
index 844b29a7ab..ce04870d18 100644
--- a/usr/src/cmd/dfs.cmds/sharemgr/commands.c
+++ b/usr/src/cmd/dfs.cmds/sharemgr/commands.c
@@ -1851,24 +1851,54 @@ sa_removeshare(int flags, int argc, char *argv[])
 	    } else {
 		group = NULL;
 	    }
-	    if (ret == SA_OK) {
-		if (realpath(sharepath, dir) == NULL) {
-		    ret = SA_BAD_PATH;
-		    (void) printf(gettext("Path is not valid: %s\n"),
-					sharepath);
-		} else {
-		    sharepath = dir;
-		}
-	    }
+
+		/*
+		 * Lookup the path in the internal configuration. Care
+		 * must be taken to handle the case where the
+		 * underlying path has been removed since we need to
+		 * be able to deal with that as well.
+		 */
 	    if (ret == SA_OK) {
 		if (group != NULL)
 		    share = sa_get_share(group, sharepath);
 		else
 		    share = sa_find_share(sharepath);
+		/*
+		 * If we didn't find the share with the provided path,
+		 * it may be a symlink so attempt to resolve it using
+		 * realpath and try again. Realpath will resolve any
+		 * symlinks and place them in "dir". Note that
+		 * sharepath is only used for the lookup the first
+		 * time and later for error messages. dir will be used
+		 * on the second attempt. Once a share is found, all
+		 * operations are based off of the share variable.
+		 */
+		if (share == NULL) {
+		    if (realpath(sharepath, dir) == NULL) {
+			ret = SA_BAD_PATH;
+			(void) printf(gettext("Path is not valid: %s\n"),
+						sharepath);
+		    } else {
+			if (group != NULL)
+			    share = sa_get_share(group, dir);
+			else
+			    share = sa_find_share(dir);
+		    }
+		}
+	    }
+
+		/*
+		 * If there hasn't been an error, there was likely a
+		 * path found. If not, give the appropriate error
+		 * message and set the return error. If it was found,
+		 * then disable the share and then remove it from the
+		 * configuration.
+		 */
+	    if (ret == SA_OK) {
 		if (share == NULL) {
 		    if (group != NULL)
 			(void) printf(gettext("Share not found in group %s:"
-						"%s\n"),
+						" %s\n"),
 					argv[optind], sharepath);
 		    else
 			(void) printf(gettext("Share not found: %s\n"),
@@ -1882,10 +1912,14 @@ sa_removeshare(int flags, int argc, char *argv[])
 			    ret = sa_disable_share(share, NULL);
 				/*
 				 * we don't care if it fails since it
-				 * could be disabled already.
+				 * could be disabled already. Some
+				 * unexpected errors could occur that
+				 * prevent removal, so also check for
+				 * force being set.
 				 */
 			    if (ret == SA_OK || ret == SA_NO_SUCH_PATH ||
-				ret == SA_NOT_SUPPORTED) {
+					ret == SA_NOT_SUPPORTED ||
+					ret == SA_SYSTEM_ERR || force) {
 				ret = sa_remove_share(share);
 			    }
 			    if (ret == SA_OK)
@@ -3859,23 +3893,38 @@ sa_legacy_unshare(int flags, int argc, char *argv[])
 		ret = run_legacy_command(cmd, argv);
 		return (ret);
 	    }
+		/*
+		 * Find the path in the internal configuration. If it
+		 * isn't found, attempt to resolve the path via
+		 * realpath() and try again.
+		 */
 	    sharepath = argv[optind++];
-	    if (realpath(sharepath, dir) == NULL) {
-		ret = SA_NO_SUCH_PATH;
-	    } else {
-		sharepath = dir;
-		share = sa_find_share(sharepath);
-		if (share != NULL) {
-		    ret = sa_disable_share(share, protocol);
-		    if (ret == SA_OK) {
-			if (persist == SA_SHARE_PERMANENT)
-			    ret = sa_remove_share(share);
-			ret = sa_update_config();
-		    }
+	    share = sa_find_share(sharepath);
+	    if (share == NULL) {
+		if (realpath(sharepath, dir) == NULL) {
+		    ret = SA_NO_SUCH_PATH;
 		} else {
-		    ret = SA_NOT_SHARED;
+		    share = sa_find_share(dir);
 		}
 	    }
+	    if (share != NULL) {
+		ret = sa_disable_share(share, protocol);
+		/*
+		 * Errors are ok and removal should still occur. The
+		 * legacy unshare is more forgiving of errors than the
+		 * remove-share subcommand which may need the force
+		 * flag set for some error conditions. That is, the
+		 * "unshare" command will always unshare if it can
+		 * while "remove-share" might require the force option.
+		 */
+		if (persist == SA_SHARE_PERMANENT) {
+		    ret = sa_remove_share(share);
+		    if (ret == SA_OK)
+			ret = sa_update_config();
+		}
+	    } else {
+		ret = SA_NOT_SHARED;
+	    }
 	}
 	switch (ret) {
 	default:
diff --git a/usr/src/cmd/dfs.cmds/sharemgr/plugins/libshare_nfs.c b/usr/src/cmd/dfs.cmds/sharemgr/plugins/libshare_nfs.c
index ae3a576123..224e685b81 100644
--- a/usr/src/cmd/dfs.cmds/sharemgr/plugins/libshare_nfs.c
+++ b/usr/src/cmd/dfs.cmds/sharemgr/plugins/libshare_nfs.c
@@ -151,7 +151,7 @@ struct option_defs optdefs[] = {
 };
 
 /*
- * list of propertye that are related to security flavors.
+ * list of properties that are related to security flavors.
  */
 static char *seclist[] = {
 	SHOPT_RO,
@@ -163,8 +163,8 @@ static char *seclist[] = {
 
 /* structure for list of securities */
 struct securities {
-    sa_security_t security;
-    struct securities *next;
+	sa_security_t security;
+	struct securities *next;
 };
 
 /*
@@ -318,6 +318,7 @@ make_security_list(sa_group_t group, char *securitymodes, char *proto)
 		    }
 		}
 	    }
+
 	    if (freetok) {
 		freetok = 0;
 		sa_free_attr_string(tok);
@@ -379,10 +380,22 @@ add_security_prop(struct securities *sec, char *name, char *value,
 		else
 		    value = "true";
 	    }
+
+		/*
+		 * Get the existing property, if it exists, so we can
+		 * determine what to do with it. The ro/rw/root
+		 * properties can be merged if multiple instances of
+		 * these properies are given. For example, if "rw"
+		 * exists with a value "host1" and a later token of
+		 * rw="host2" is seen, the values are merged into a
+		 * single rw="host1:host2".
+		 */
 	    prop = sa_get_property(sec->security, name);
+
 	    if (prop != NULL) {
 		char *oldvalue;
 		char *newvalue;
+
 		/*
 		 * The security options of ro/rw/root might appear
 		 * multiple times. If they do, the values need to be
@@ -391,14 +404,32 @@ add_security_prop(struct securities *sec, char *name, char *value,
 		 */
 		oldvalue = sa_get_property_attr(prop, "value");
 		if (oldvalue != NULL) {
-		    newvalue = nfs_alistcat(oldvalue, value, ':');
-		    if (newvalue != NULL)
-			value = newvalue;
-		    (void) sa_remove_property(prop);
-		    prop = sa_create_property(name, value);
-		    ret = sa_add_property(sec->security, prop);
-		    if (newvalue != NULL)
+			/*
+			 * The general case is to concatenate the new
+			 * value onto the old value for multiple
+			 * rw(ro/root) properties. A special case
+			 * exists when either the old or new is the
+			 * "all" case. In the special case, if both
+			 * are "all", then it is "all", else if one is
+			 * an access-list, that replaces the "all".
+			 */
+		    if (strcmp(oldvalue, "*") == 0) {
+			/* Replace old value with new value. */
+			newvalue = strdup(value);
+		    } else if (strcmp(value, "*") == 0) {
+			/* Keep old value and ignore the new value. */
+			newvalue = NULL;
+		    } else {
+			/* Make a new list of old plus new access-list. */
+			newvalue = nfs_alistcat(oldvalue, value, ':');
+		    }
+
+		    if (newvalue != NULL) {
+			(void) sa_remove_property(prop);
+			prop = sa_create_property(name, newvalue);
+			ret = sa_add_property(sec->security, prop);
 			free(newvalue);
+		    }
 		    if (oldvalue != NULL)
 			sa_free_attr_string(oldvalue);
 		}
@@ -548,13 +579,14 @@ nfs_parse_legacy_options(sa_group_t group, char *options)
 	 * we need to step through each option in the string and then
 	 * add either the option or the security option as needed. If
 	 * this is not a persistent share, don't commit to the
-	 * repository.
+	 * repository. If there is an error, we also want to abort the
+	 * processing and report it.
 	 */
 	persist = is_persistent(group);
 	base = dup;
 	token = dup;
 	lasts = NULL;
-	while (token != NULL) {
+	while (token != NULL && ret == SA_OK) {
 	    ret = SA_OK;
 	    token = strtok_r(base, ",", &lasts);
 	    base = NULL;
@@ -939,7 +971,7 @@ fill_security_from_secopts(struct secinfo *sp, sa_security_t secopts)
 	sa_property_t prop;
 	char *type;
 	int longform;
-	int err = 0;
+	int err = SC_NOERROR;
 
 	type = sa_get_security_attr(secopts, "sectype");
 	if (type != NULL) {
@@ -955,8 +987,10 @@ fill_security_from_secopts(struct secinfo *sp, sa_security_t secopts)
 		return (err);
 	}
 
+	err = SA_OK;
 	for (prop = sa_get_property(secopts, NULL);
-		prop != NULL; prop = sa_get_next_property(prop)) {
+		prop != NULL && err == SA_OK;
+		prop = sa_get_next_property(prop)) {
 	    char *name;
 	    char *value;
 
@@ -981,9 +1015,14 @@ fill_security_from_secopts(struct secinfo *sp, sa_security_t secopts)
 		if (sp->s_secinfo.sc_rpcnum == AUTH_UNIX)
 		    continue;
 		/* not AUTH_UNIX */
-		if (value != NULL)
+		if (value != NULL) {
 		    sp->s_rootnames = get_rootnames(&sp->s_secinfo, value,
 							&sp->s_rootcnt);
+		    if (sp->s_rootnames == NULL) {
+			err = SA_BAD_VALUE;
+			(void) fprintf(stderr, gettext("Bad root list\n"));
+		    }
+		}
 		break;
 	    case OPT_WINDOW:
 		if (value != NULL) {
@@ -1833,6 +1872,15 @@ nfs_validate_property(sa_property_t property, sa_optionset_t parent)
 		case OPT_TYPE_STRING:
 		    /* whatever is here should be ok */
 		    break;
+		case OPT_TYPE_SECURITY:
+			/*
+			 * The "sec" property isn't used in the
+			 * non-legacy parts of sharemgr. We need to
+			 * reject it here. For legacy, it is pulled
+			 * out well before we get here.
+			 */
+		    ret = SA_NO_SUCH_PROP;
+		    break;
 		default:
 		    break;
 		}
@@ -2632,7 +2680,15 @@ nfs_space_alias(char *space)
 {
 	char *name = space;
 	seconfig_t secconf;
-	if (nfs_getseconfig_default(&secconf) == 0) {
+
+	/*
+	 * Only the space named "default" is special. If it is used,
+	 * the default needs to be looked up and the real name used.
+	 * This is normally "sys" but could be changed.  We always
+	 * change defautl to the real name.
+	 */
+	if (strcmp(space, "default") == 0 &&
+	    nfs_getseconfig_default(&secconf) == 0) {
 	    if (nfs_getseconfig_bynumber(secconf.sc_nfsnum, &secconf) == 0)
 		name = secconf.sc_name;
 	}
diff --git a/usr/src/lib/libshare/common/libshare.c b/usr/src/lib/libshare/common/libshare.c
index 9b54b30e1b..e221c61cb7 100644
--- a/usr/src/lib/libshare/common/libshare.c
+++ b/usr/src/lib/libshare/common/libshare.c
@@ -34,6 +34,7 @@
 #include <ctype.h>
 #include <sys/types.h>
 #include <sys/stat.h>
+#include <fcntl.h>
 #include <unistd.h>
 #include <libxml/parser.h>
 #include <libxml/tree.h>
@@ -51,6 +52,8 @@
 #define	TSTAMP(tm)	(uint64_t)(((uint64_t)tm.tv_sec << 32) | \
 					(tm.tv_nsec & 0xffffffff))
 
+#define	DFS_LOCK_FILE	"/etc/dfs/fstypes"
+
 /*
  * internal data structures
  */
@@ -74,6 +77,8 @@ extern void update_legacy_config(void);
 extern int issubdir(char *, char *);
 extern void sa_zfs_init(void);
 extern void sa_zfs_fini(void);
+extern void sablocksigs(sigset_t *);
+extern void saunblocksigs(sigset_t *);
 
 static int sa_initialized = 0;
 
@@ -167,46 +172,6 @@ sa_errorstr(int err)
 }
 
 /*
- * get_legacy_timestamp(root, path)
- *	gets the timestamp of the last time sharemgr updated the legacy
- *	files. This is used to determine if someone has modified them by
- *	hand.
- */
-static uint64_t
-get_legacy_timestamp(xmlNodePtr root, char *path)
-{
-	uint64_t tval = 0;
-	xmlNodePtr node;
-	xmlChar *lpath = NULL;
-	xmlChar *timestamp = NULL;
-
-	for (node = root->xmlChildrenNode; node != NULL;
-		node = node->next) {
-	    if (xmlStrcmp(node->name, (xmlChar *)"legacy") == 0) {
-		/* a possible legacy node for this path */
-		lpath = xmlGetProp(node, (xmlChar *)"path");
-		if (lpath != NULL && xmlStrcmp(lpath, (xmlChar *)path) == 0) {
-		    /* now have the node, extract the data */
-		    timestamp = xmlGetProp(node, (xmlChar *)"timestamp");
-		    if (timestamp != NULL) {
-			tval = strtoull((char *)timestamp, NULL, 0);
-			break;
-		    }
-		}
-		if (lpath != NULL) {
-		    xmlFree(lpath);
-		    lpath = NULL;
-		}
-	    }
-	}
-	if (lpath != NULL)
-	    xmlFree(lpath);
-	if (timestamp != NULL)
-	    xmlFree(timestamp);
-	return (tval);
-}
-
-/*
  * set_legacy_timestamp(root, path, timevalue)
  *
  * add the current timestamp value to the configuration for use in
@@ -282,30 +247,25 @@ is_shared(sa_share_t share)
 }
 
 /*
- * checksubdir(newpath, strictness)
+ * checksubdirgroup(group, newpath, strictness)
  *
- * checksubdir determines if the specified path (newpath) is a
- * subdirectory of another share. It calls issubdir() from the old
- * share implementation to do the complicated work. The strictness
- * parameter determines how strict a check to make against the
- * path. The strictness values mean:
+ * check all the specified newpath against all the paths in the
+ * group. This is a helper function for checksubdir to make it easier
+ * to also check ZFS subgroups.
+ * The strictness values mean:
  * SA_CHECK_NORMAL == only check newpath against shares that are active
  * SA_CHECK_STRICT == check newpath against both active shares and those
  *		      stored in the repository
  */
 static int
-checksubdir(char *newpath, int strictness)
+checksubdirgroup(sa_group_t group, char *newpath, int strictness)
 {
-	sa_group_t group;
 	sa_share_t share;
-	int issub;
-	char *path = NULL;
+	char *path;
+	int issub = SA_OK;
 
-	for (issub = 0, group = sa_get_group(NULL);
-		group != NULL && !issub;
-		group = sa_get_next_group(group)) {
-	    for (share = sa_get_share(group, NULL); share != NULL;
-		    share = sa_get_next_share(share)) {
+	for (share = sa_get_share(group, NULL); share != NULL;
+	    share = sa_get_next_share(share)) {
 		/*
 		 * The original behavior of share never checked
 		 * against the permanent configuration
@@ -314,27 +274,63 @@ checksubdir(char *newpath, int strictness)
 		 * could be considered incorrect.  We may tighten this
 		 * up in the future.
 		 */
-		if (strictness == SA_CHECK_NORMAL && !is_shared(share))
-		    continue;
+	    if (strictness == SA_CHECK_NORMAL && !is_shared(share))
+		continue;
 
-		path = sa_get_share_attr(share, "path");
+	    path = sa_get_share_attr(share, "path");
 		/*
 		 * If path is NULL, then a share is in the process of
 		 * construction or someone has modified the property
-		 * group inappropriately. It should be ignored.
+		 * group inappropriately. It should be
+		 * ignored. issubdir() comes from the original share
+		 * implementation and does the difficult part of
+		 * checking subdirectories.
 		 */
-		if (path == NULL)
-		    continue;
-		if (newpath != NULL &&
-		    (strcmp(path, newpath) == 0 || issubdir(newpath, path) ||
-			issubdir(path, newpath))) {
-		    sa_free_attr_string(path);
-		    path = NULL;
-		    issub = SA_INVALID_PATH;
-		    break;
-		}
+	    if (path == NULL)
+		continue;
+	    if (newpath != NULL &&
+		(strcmp(path, newpath) == 0 || issubdir(newpath, path) ||
+		issubdir(path, newpath))) {
 		sa_free_attr_string(path);
 		path = NULL;
+		issub = SA_INVALID_PATH;
+		break;
+	    }
+	    sa_free_attr_string(path);
+	    path = NULL;
+	}
+	return (issub);
+}
+
+/*
+ * checksubdir(newpath, strictness)
+ *
+ * checksubdir determines if the specified path (newpath) is a
+ * subdirectory of another share. It calls checksubdirgroup() to do
+ * the complicated work. The strictness parameter determines how
+ * strict a check to make against the path. The strictness values
+ * mean: SA_CHECK_NORMAL == only check newpath against shares that are
+ * active SA_CHECK_STRICT == check newpath against both active shares
+ * and those * stored in the repository
+ */
+static int
+checksubdir(char *newpath, int strictness)
+{
+	sa_group_t group;
+	int issub;
+	char *path = NULL;
+
+	for (issub = 0, group = sa_get_group(NULL);
+		group != NULL && !issub;
+		group = sa_get_next_group(group)) {
+	    if (sa_group_is_zfs(group)) {
+		sa_group_t subgroup;
+		for (subgroup = sa_get_sub_group(group);
+		    subgroup != NULL && !issub;
+		    subgroup = sa_get_next_group(subgroup))
+		    issub = checksubdirgroup(subgroup, newpath, strictness);
+	    } else {
+		issub = checksubdirgroup(group, newpath, strictness);
 	    }
 	}
 	if (path != NULL)
@@ -378,7 +374,7 @@ validpath(char *path, int strictness)
 		fstype = sa_fstype(path);
 		if (fstype != NULL && strcmp(fstype, "zfs") == 0) {
 		    if (sa_zfs_is_shared(path))
-			error = SA_DUPLICATE_NAME;
+			error = SA_INVALID_NAME;
 		}
 		if (fstype != NULL)
 		    sa_free_fstype(fstype);
@@ -541,6 +537,10 @@ sa_init(int init_service)
 	struct stat st;
 	int legacy = 0;
 	uint64_t tval = 0;
+	int lockfd;
+	sigset_t old;
+	int updatelegacy = B_FALSE;
+	scf_simple_prop_t *prop;
 
 	if (!sa_initialized) {
 	    /* get protocol specific structures */
@@ -557,10 +557,62 @@ sa_init(int init_service)
 		 */
 		scf_handle = sa_scf_init();
 		if (scf_handle != NULL) {
+			/*
+			 * Need to lock the extraction of the
+			 * configuration if the dfstab file has
+			 * changed. Lock everything now and release if
+			 * not needed.  Use a file that isn't being
+			 * manipulated by other parts of the system in
+			 * order to not interfere with locking. Using
+			 * dfstab doesn't work.
+			 */
+		    sablocksigs(&old);
+		    lockfd = open(DFS_LOCK_FILE, O_RDWR);
+		    if (lockfd >= 0) {
+			extern int errno;
+			errno = 0;
+			(void) lockf(lockfd, F_LOCK, 0);
+			/*
+			 * Check whether we are going to need to merge
+			 * any dfstab changes. This is done by
+			 * comparing the value of legacy-timestamp
+			 * with the current st_ctim of the file. If
+			 * they are different, an update is needed and
+			 * the file must remain locked until the merge
+			 * is done in order to prevent multiple
+			 * startups from changing the SMF repository
+			 * at the same time.  The first to get the
+			 * lock will make any changes before the
+			 * others can read the repository.
+			 */
+			prop = scf_simple_prop_get(scf_handle->handle,
+						(const char *)
+						    SA_SVC_FMRI_BASE ":default",
+						"operation",
+						"legacy-timestamp");
+			if (prop != NULL) {
+			    char *i64;
+			    i64 = scf_simple_prop_next_astring(prop);
+			    if (i64 != NULL) {
+				tval = strtoull(i64, NULL, 0);
+			    }
+			    if (stat(SA_LEGACY_DFSTAB, &st) >= 0 &&
+				tval != TSTAMP(st.st_ctim)) {
+				updatelegacy = B_TRUE;
+			    }
+			} else {
+			    /* We haven't set the timestamp before so do it. */
+			    updatelegacy = B_TRUE;
+			}
+		    }
+		    if (updatelegacy == B_FALSE) {
+			/* Don't need the lock anymore */
+			(void) lockf(lockfd, F_ULOCK, 0);
+			(void) close(lockfd);
+		    }
 		    (void) sa_get_config(scf_handle, &sa_config_tree,
 				    &sa_config_doc);
-		    tval = get_legacy_timestamp(sa_config_tree,
-						SA_LEGACY_DFSTAB);
+		    saunblocksigs(&old);
 		    if (tval == 0) {
 			/* first time so make sure default is setup */
 			sa_group_t defgrp;
@@ -573,13 +625,17 @@ sa_init(int init_service)
 				opt = sa_create_optionset(defgrp, "nfs");
 			}
 		    }
-		    if (stat(SA_LEGACY_DFSTAB, &st) >= 0 &&
-			tval != TSTAMP(st.st_ctim)) {
+		    if (updatelegacy == B_TRUE) {
+			sablocksigs(&old);
 			getlegacyconfig(SA_LEGACY_DFSTAB, &sa_config_tree);
 			if (stat(SA_LEGACY_DFSTAB, &st) >= 0)
 			    set_legacy_timestamp(sa_config_tree,
 						SA_LEGACY_DFSTAB,
 						TSTAMP(st.st_ctim));
+			saunblocksigs(&old);
+			/* Safe to unlock now to allow others to run */
+			(void) lockf(lockfd, F_ULOCK, 0);
+			(void) close(lockfd);
 		    }
 		    legacy |= sa_get_zfs_shares("zfs");
 		    legacy |= gettransients(&sa_config_tree);
diff --git a/usr/src/lib/libshare/common/libsharecore.c b/usr/src/lib/libshare/common/libsharecore.c
index a2ed24afca..4b5fdc46c0 100644
--- a/usr/src/lib/libshare/common/libsharecore.c
+++ b/usr/src/lib/libshare/common/libsharecore.c
@@ -96,6 +96,39 @@ extern int set_node_share(void *, char *, char *);
 extern void set_node_attr(void *, char *, char *);
 
 /*
+ * sablocksigs(*sigs)
+ *
+ * block important signals for a critical region. Arg is a pointer to
+ * a sigset_t that is used later for the unblock.
+ */
+void
+sablocksigs(sigset_t *sigs)
+{
+	sigset_t new;
+
+	if (sigs != NULL) {
+	    (void) sigprocmask(SIG_BLOCK, NULL, &new);
+	    (void) sigaddset(&new, SIGHUP);
+	    (void) sigaddset(&new, SIGINT);
+	    (void) sigaddset(&new, SIGQUIT);
+	    (void) sigaddset(&new, SIGTSTP);
+	    (void) sigprocmask(SIG_SETMASK, &new, sigs);
+	}
+}
+
+/*
+ * saunblocksigs(*sigs)
+ *
+ * unblock previously blocked signals from the sigs arg.
+ */
+void
+saunblocksigs(sigset_t *sigs)
+{
+	if (sigs != NULL)
+	    (void) sigprocmask(SIG_SETMASK, sigs, NULL);
+}
+
+/*
  * alloc_sharelist()
  *
  * allocator function to return an zfs_sharelist_t
@@ -259,9 +292,10 @@ getdfstab(FILE *dfs)
 			    item->resource = strdup(resource);
 		    }
 		}
+		if (item != NULL && item->fstype == NULL) {
+		    item->fstype = strdup("nfs"); /* this is the default */
+		}
 	    }
-	    if (item != NULL && item->fstype == NULL)
-		item->fstype = strdup("nfs"); /* this is the default */
 	}
 	first = fix_notice(first);
 	return (first);
@@ -500,17 +534,12 @@ sa_comment_line(char *line, char *err)
 {
 	FILE *dfstab;
 	xfs_sharelist_t *list;
-	sigset_t old, new;
+	sigset_t old;
 
 	dfstab = open_dfstab(SA_LEGACY_DFSTAB);
 	if (dfstab != NULL) {
 		(void) setvbuf(dfstab, NULL, _IOLBF, BUFSIZ * 8);
-		(void) sigprocmask(SIG_BLOCK, NULL, &new);
-		(void) sigaddset(&new, SIGHUP);
-		(void) sigaddset(&new, SIGINT);
-		(void) sigaddset(&new, SIGQUIT);
-		(void) sigaddset(&new, SIGTSTP);
-		(void) sigprocmask(SIG_SETMASK, &new, &old);
+		sablocksigs(&old);
 		(void) lockf(fileno(dfstab), F_LOCK, 0);
 		list = getdfstab(dfstab);
 		rewind(dfstab);
@@ -524,7 +553,7 @@ sa_comment_line(char *line, char *err)
 		(void) fsync(fileno(dfstab));
 		(void) lockf(fileno(dfstab), F_ULOCK, 0);
 		(void) fclose(dfstab);
-		(void) sigprocmask(SIG_SETMASK, &old, NULL);
+		saunblocksigs(&old);
 		if (list != NULL)
 		    dfs_free_list(list);
 	}
@@ -546,17 +575,12 @@ sa_delete_legacy(sa_share_t share)
 	char *path;
 	sa_optionset_t optionset;
 	sa_group_t parent;
-	sigset_t old, new;
+	sigset_t old;
 
 	dfstab = open_dfstab(SA_LEGACY_DFSTAB);
 	if (dfstab != NULL) {
 		(void) setvbuf(dfstab, NULL, _IOLBF, BUFSIZ * 8);
-		(void) sigprocmask(SIG_BLOCK, NULL, &new);
-		(void) sigaddset(&new, SIGHUP);
-		(void) sigaddset(&new, SIGINT);
-		(void) sigaddset(&new, SIGQUIT);
-		(void) sigaddset(&new, SIGTSTP);
-		(void) sigprocmask(SIG_SETMASK, &new, &old);
+		sablocksigs(&old);
 		path = sa_get_share_attr(share, "path");
 		parent = sa_get_parent_group(share);
 		if (parent != NULL) {
@@ -591,7 +615,7 @@ sa_delete_legacy(sa_share_t share)
 		    (void) lockf(fileno(dfstab), F_ULOCK, 0);
 		}
 		(void) fsync(fileno(dfstab));
-		(void) sigprocmask(SIG_SETMASK, &old, NULL);
+		saunblocksigs(&old);
 		(void) fclose(dfstab);
 		sa_free_attr_string(path);
 	} else {
@@ -624,7 +648,7 @@ sa_update_legacy(sa_share_t share, char *proto)
 	int ret = SA_OK;
 	xfs_sharelist_t *list;
 	char *path;
-	sigset_t old, new;
+	sigset_t old;
 	char *persist;
 
 	ret = sa_proto_update_legacy(proto, share);
@@ -640,12 +664,7 @@ sa_update_legacy(sa_share_t share, char *proto)
 	    dfstab = open_dfstab(SA_LEGACY_DFSTAB);
 	    if (dfstab != NULL) {
 		(void) setvbuf(dfstab, NULL, _IOLBF, BUFSIZ * 8);
-		(void) sigprocmask(SIG_BLOCK, NULL, &new);
-		(void) sigaddset(&new, SIGHUP);
-		(void) sigaddset(&new, SIGINT);
-		(void) sigaddset(&new, SIGQUIT);
-		(void) sigaddset(&new, SIGTSTP);
-		(void) sigprocmask(SIG_SETMASK, &new, &old);
+		sablocksigs(&old);
 		path = sa_get_share_attr(share, "path");
 		(void) lockf(fileno(dfstab), F_LOCK, 0);
 		list = getdfstab(dfstab);
@@ -657,7 +676,7 @@ sa_update_legacy(sa_share_t share, char *proto)
 		(void) fflush(dfstab);
 		(void) lockf(fileno(dfstab), F_ULOCK, 0);
 		(void) fsync(fileno(dfstab));
-		(void) sigprocmask(SIG_SETMASK, &old, NULL);
+		saunblocksigs(&old);
 		(void) fclose(dfstab);
 		sa_free_attr_string(path);
 		if (list != NULL)
@@ -1058,6 +1077,7 @@ parse_dfstab(char *dfstab, xmlNodePtr root)
 							    list->options,
 							    list->fstype);
 			}
+			sa_format_free(oldprops);
 		    }
 		}
 	    } else {
@@ -1168,6 +1188,7 @@ get_share_list(int *errp)
 
 	if ((fp = fopen(SHARETAB, "r")) != NULL) {
 		struct share	*sharetab_entry;
+		(void) lockf(fileno(fp), F_LOCK, 0);
 
 		while (getshare(fp, &sharetab_entry) > 0) {
 		    newp = alloc_sharelist();
@@ -1203,6 +1224,7 @@ get_share_list(int *errp)
 		    if (newp->description == NULL)
 			goto err;
 		}
+		(void) lockf(fileno(fp), F_ULOCK, 0);
 		(void) fclose(fp);
 	} else {
 	    *errp = errno;
@@ -1802,6 +1824,50 @@ emptyshare(struct share *sh)
 }
 
 /*
+ * checkshare(struct share *)
+ *
+ * If the share to write to sharetab is not present, need to add.  If
+ * the share is present, replace if options are different else we want
+ * to keep it.
+ * Return values:
+ *	1 - keep
+ *	2 - replace
+ * The CHK_NEW value isn't currently returned.
+ */
+#define	CHK_NEW		0
+#define	CHK_KEEP	1
+#define	CHK_REPLACE	2
+static int
+checkshare(struct share *sh)
+{
+	xfs_sharelist_t *list, *head;
+	int err;
+	int ret = CHK_NEW;
+
+	head = list = get_share_list(&err);
+	while (list != NULL && ret == CHK_NEW) {
+	    if (strcmp(sh->sh_path, list->path) == 0) {
+		/* Have the same path so check if replace or keep */
+		if (strcmp(sh->sh_opts, list->options) == 0)
+		    ret = CHK_KEEP;
+		else
+		    ret = CHK_REPLACE;
+	    }
+	    list = list->next;
+	}
+	if (head != NULL) {
+	    dfs_free_list(head);
+	}
+	/*
+	 * Just in case it was added by another process after our
+	 * scan, we always replace even if we think it is new.
+	 */
+	if (ret == CHK_NEW)
+	    ret = CHK_REPLACE;
+	return (ret);
+}
+
+/*
  * sa_update_sharetab(share, proto)
  *
  * Update the sharetab file with info from the specified share.
@@ -1816,7 +1882,8 @@ sa_update_sharetab(sa_share_t share, char *proto)
 	char *path;
 	int logging = 0;
 	FILE *sharetab;
-	sigset_t old, new;
+	sigset_t old;
+	int action;
 
 	path = sa_get_share_attr(share, "path");
 	if (path != NULL) {
@@ -1827,22 +1894,32 @@ sa_update_sharetab(sa_share_t share, char *proto)
 	    }
 	    if (sharetab != NULL) {
 		(void) setvbuf(sharetab, NULL, _IOLBF, BUFSIZ * 8);
-		(void) sigprocmask(SIG_BLOCK, NULL, &new);
-		(void) sigaddset(&new, SIGHUP);
-		(void) sigaddset(&new, SIGINT);
-		(void) sigaddset(&new, SIGQUIT);
-		(void) sigaddset(&new, SIGTSTP);
-		(void) sigprocmask(SIG_SETMASK, &new, &old);
-		(void) lockf(fileno(sharetab), F_LOCK, 0);
-		(void) remshare(sharetab, path, &logging);
-		/* fill in share structure and write it out */
+		sablocksigs(&old);
+		/*
+		 * Fill in share structure and write it out if the
+		 * share isn't already shared with the same options.
+		 */
 		(void) fillshare(share, proto, &shtab);
-		(void) putshare(sharetab, &shtab);
+		/*
+		 * If share is new or changed, remove the old,
+		 * otherwise keep it in place since it hasn't changed.
+		 */
+		action = checkshare(&shtab);
+		(void) lockf(fileno(sharetab), F_LOCK, 0);
+		switch (action) {
+		case CHK_REPLACE:
+		    (void) remshare(sharetab, path, &logging);
+		    (void) putshare(sharetab, &shtab);
+		    break;
+		case CHK_KEEP:
+		    /* Don't do anything */
+		    break;
+		}
 		emptyshare(&shtab);
 		(void) fflush(sharetab);
 		(void) lockf(fileno(sharetab), F_ULOCK, 0);
 		(void) fsync(fileno(sharetab));
-		(void) sigprocmask(SIG_SETMASK, &old, NULL);
+		saunblocksigs(&old);
 		(void) fclose(sharetab);
 	    } else {
 		if (errno == EACCES || errno == EPERM) {
@@ -1868,7 +1945,7 @@ sa_delete_sharetab(char *path, char *proto)
 	int ret = SA_OK;
 	int logging = 0;
 	FILE *sharetab;
-	sigset_t old, new;
+	sigset_t old;
 #ifdef lint
 	proto = proto;
 #endif
@@ -1880,18 +1957,13 @@ sa_delete_sharetab(char *path, char *proto)
 	    }
 	    if (sharetab != NULL) {
 		/* should block keyboard level signals around the lock */
-		(void) sigprocmask(SIG_BLOCK, NULL, &new);
-		(void) sigaddset(&new, SIGHUP);
-		(void) sigaddset(&new, SIGINT);
-		(void) sigaddset(&new, SIGQUIT);
-		(void) sigaddset(&new, SIGTSTP);
-		(void) sigprocmask(SIG_SETMASK, &new, &old);
+		sablocksigs(&old);
 		(void) lockf(fileno(sharetab), F_LOCK, 0);
 		ret = remshare(sharetab, path, &logging);
 		(void) fflush(sharetab);
 		(void) lockf(fileno(sharetab), F_ULOCK, 0);
 		(void) fsync(fileno(sharetab));
-		(void) sigprocmask(SIG_SETMASK, &old, NULL);
+		saunblocksigs(&old);
 		(void) fclose(sharetab);
 	    } else {
 		if (errno == EACCES || errno == EPERM) {