libshare: nfs: open temporary file once

Reviewed-by: Don Brady <don.brady@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: John Kennedy <john.kennedy@delphix.com>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes #12067

lib/libshare/nfs.c

@@ -77,10 +77,18 @@ nfs_exports_unlock(const char *name)
 	nfs_lock_fd = -1;
 }
 
-static char *
+struct tmpfile {
-nfs_init_tmpfile(const char *prefix, const char *mdir)
+	/*
+	 * This only needs to be as wide as ZFS_EXPORTS_FILE and mktemp suffix,
+	 * 64 is more than enough.
+	 */
+	char name[64];
+	FILE *fp;
+};
+
+static boolean_t
+nfs_init_tmpfile(const char *prefix, const char *mdir, struct tmpfile *tmpf)
 {
-	char *tmpfile = NULL;
 	struct stat sb;
 
 	if (mdir != NULL &&
@@ -88,72 +96,89 @@ nfs_init_tmpfile(const char *prefix, const char *mdir)
 	    mkdir(mdir, 0755) < 0) {
 		fprintf(stderr, "failed to create %s: %s\n",
 		    mdir, strerror(errno));
-		return (NULL);
+		return (B_FALSE);
 	}
 
-	if (asprintf(&tmpfile, "%s.XXXXXXXX", prefix) == -1) {
+	strcpy(tmpf->name, prefix);
-		fprintf(stderr, "Unable to allocate temporary file\n");
+	strcat(tmpf->name, ".XXXXXXXX");
-		return (NULL);
-	}
 
-	int fd = mkostemp(tmpfile, O_CLOEXEC);
+	int fd = mkostemp(tmpf->name, O_CLOEXEC);
 	if (fd == -1) {
 		fprintf(stderr, "Unable to create temporary file: %s",
 		    strerror(errno));
-		free(tmpfile);
+		return (B_FALSE);
-		return (NULL);
 	}
-	close(fd);
+
-	return (tmpfile);
+	tmpf->fp = fdopen(fd, "w+");
+	if (tmpf->fp == NULL) {
+		fprintf(stderr, "Unable to reopen temporary file: %s",
+		    strerror(errno));
+		close(fd);
+		return (B_FALSE);
+	}
+
+	return (B_TRUE);
+}
+
+static void
+nfs_abort_tmpfile(struct tmpfile *tmpf)
+{
+	unlink(tmpf->name);
+	fclose(tmpf->fp);
 }
 
 static int
-nfs_fini_tmpfile(const char *exports, char *tmpfile)
+nfs_fini_tmpfile(const char *exports, struct tmpfile *tmpf)
 {
-	if (rename(tmpfile, exports) == -1) {
+	if (fflush(tmpf->fp) != 0) {
-		fprintf(stderr, "Unable to rename %s: %s\n", tmpfile,
+		fprintf(stderr, "Failed to write to temporary file: %s\n",
 		    strerror(errno));
-		unlink(tmpfile);
+		nfs_abort_tmpfile(tmpf);
-		free(tmpfile);
 		return (SA_SYSTEM_ERR);
 	}
-	free(tmpfile);
+
+	if (rename(tmpf->name, exports) == -1) {
+		fprintf(stderr, "Unable to rename %s -> %s: %s\n",
+		    tmpf->name, exports, strerror(errno));
+		nfs_abort_tmpfile(tmpf);
+		return (SA_SYSTEM_ERR);
+	}
+
+	fclose(tmpf->fp);
 	return (SA_OK);
 }
 
 int
 nfs_toggle_share(const char *lockfile, const char *exports,
     const char *expdir, sa_share_impl_t impl_share,
-    int(*cbk)(sa_share_impl_t impl_share, char *filename))
+    int(*cbk)(sa_share_impl_t impl_share, FILE *tmpfile))
 {
 	int error;
-	char *filename;
+	struct tmpfile tmpf;
 
-	if ((filename = nfs_init_tmpfile(exports, expdir)) == NULL)
+	if (!nfs_init_tmpfile(exports, expdir, &tmpf))
 		return (SA_SYSTEM_ERR);
 
 	error = nfs_exports_lock(lockfile);
 	if (error != 0) {
-		unlink(filename);
+		nfs_abort_tmpfile(&tmpf);
-		free(filename);
 		return (error);
 	}
 
-	error = nfs_copy_entries(filename, impl_share->sa_mountpoint);
+	error = nfs_copy_entries(tmpf.fp, impl_share->sa_mountpoint);
 	if (error != SA_OK)
 		goto fullerr;
 
-	error = cbk(impl_share, filename);
+	error = cbk(impl_share, tmpf.fp);
 	if (error != SA_OK)
 		goto fullerr;
 
-	error = nfs_fini_tmpfile(exports, filename);
+	error = nfs_fini_tmpfile(exports, &tmpf);
 	nfs_exports_unlock(lockfile);
 	return (error);
 
 fullerr:
-	unlink(filename);
+	nfs_abort_tmpfile(&tmpf);
-	free(filename);
 	nfs_exports_unlock(lockfile);
 	return (error);
 }

lib/libshare/nfs.h

@@ -30,7 +30,7 @@
 
 void libshare_nfs_init(void);
 
-int nfs_copy_entries(char *filename, const char *mountpoint);
+int nfs_copy_entries(FILE *tmpfile, const char *mountpoint);
 int nfs_toggle_share(const char *lockfile, const char *exports,
     const char *expdir, sa_share_impl_t impl_share,
-    int(*cbk)(sa_share_impl_t impl_share, char *filename));
+    int(*cbk)(sa_share_impl_t impl_share, FILE *tmpfile));

lib/libshare/os/freebsd/nfs.c

@@ -144,23 +144,16 @@ translate_opts(const char *shareopts)
 }
 
 /*
- * This function copies all entries from the exports file to "filename",
+ * This function copies all entries from the exports file to newfp,
  * omitting any entries for the specified mountpoint.
  */
 int
-nfs_copy_entries(char *filename, const char *mountpoint)
+nfs_copy_entries(FILE *newfp, const char *mountpoint)
 {
 	int error = SA_OK;
 	char *line;
 
 	FILE *oldfp = fopen(ZFS_EXPORTS_FILE, "re");
-	FILE *newfp = fopen(filename, "w+e");
-	if (newfp == NULL) {
-		fprintf(stderr, "failed to open %s file: %s", filename,
-		    strerror(errno));
-		fclose(oldfp);
-		return (SA_SYSTEM_ERR);
-	}
 	fputs(FILE_HEADER, newfp);
 
 	/*
@@ -175,47 +168,27 @@ nfs_copy_entries(char *filename, const char *mountpoint)
 		}
 		if (fclose(oldfp) != 0) {
 			fprintf(stderr, "Unable to close file %s: %s\n",
-			    filename, strerror(errno));
+			    ZFS_EXPORTS_FILE, strerror(errno));
 			error = error != 0 ? error : SA_SYSTEM_ERR;
 		}
 	}
 
-	if (error == 0 && ferror(newfp) != 0) {
+	if (error == SA_OK && ferror(newfp) != 0)
 		error = ferror(newfp);
-	}
 
-	if (fclose(newfp) != 0) {
-		fprintf(stderr, "Unable to close file %s: %s\n",
-		    filename, strerror(errno));
-		error = error != 0 ? error : SA_SYSTEM_ERR;
-	}
 	return (error);
 }
 
 static int
-nfs_enable_share_impl(sa_share_impl_t impl_share, char *filename)
+nfs_enable_share_impl(sa_share_impl_t impl_share, FILE *tmpfile)
 {
-	FILE *fp = fopen(filename, "a+e");
-	if (fp == NULL) {
-		fprintf(stderr, "failed to open %s file: %s", filename,
-		    strerror(errno));
-		return (SA_SYSTEM_ERR);
-	}
-
 	char *shareopts = FSINFO(impl_share, nfs_fstype)->shareopts;
 	if (strcmp(shareopts, "on") == 0)
 		shareopts = "";
 
-	if (fprintf(fp, "%s\t%s\n", impl_share->sa_mountpoint,
+	if (fprintf(tmpfile, "%s\t%s\n", impl_share->sa_mountpoint,
 	    translate_opts(shareopts)) < 0) {
-		fprintf(stderr, "failed to write to %s\n", filename);
+		fprintf(stderr, "failed to write to temporary file\n");
-		fclose(fp);
-		return (SA_SYSTEM_ERR);
-	}
-
-	if (fclose(fp) != 0) {
-		fprintf(stderr, "Unable to close file %s: %s\n",
-		    filename, strerror(errno));
 		return (SA_SYSTEM_ERR);
 	}
 
@@ -231,7 +204,7 @@ nfs_enable_share(sa_share_impl_t impl_share)
 }
 
 static int
-nfs_disable_share_impl(sa_share_impl_t impl_share, char *filename)
+nfs_disable_share_impl(sa_share_impl_t impl_share, FILE *tmpfile)
 {
 	return (SA_OK);
 }

lib/libshare/os/linux/nfs.c

@@ -51,7 +51,7 @@ static sa_fstype_t *nfs_fstype;
 typedef int (*nfs_shareopt_callback_t)(const char *opt, const char *value,
     void *cookie);
 
-typedef int (*nfs_host_callback_t)(const char *sharepath, const char *filename,
+typedef int (*nfs_host_callback_t)(FILE *tmpfile, const char *sharepath,
     const char *host, const char *security, const char *access, void *cookie);
 
 /*
@@ -122,7 +122,7 @@ typedef struct nfs_host_cookie_s {
 	nfs_host_callback_t callback;
 	const char *sharepath;
 	void *cookie;
-	const char *filename;
+	FILE *tmpfile;
 	const char *security;
 } nfs_host_cookie_t;
 
@@ -203,7 +203,7 @@ foreach_nfs_host_cb(const char *opt, const char *value, void *pcookie)
 				}
 			}
 
-			error = udata->callback(udata->filename,
+			error = udata->callback(udata->tmpfile,
 			    udata->sharepath, host, udata->security,
 			    access, udata->cookie);
 
@@ -226,7 +226,7 @@ foreach_nfs_host_cb(const char *opt, const char *value, void *pcookie)
  * Invokes a callback function for all NFS hosts that are set for a share.
  */
 static int
-foreach_nfs_host(sa_share_impl_t impl_share, char *filename,
+foreach_nfs_host(sa_share_impl_t impl_share, FILE *tmpfile,
     nfs_host_callback_t callback, void *cookie)
 {
 	nfs_host_cookie_t udata;
@@ -235,7 +235,7 @@ foreach_nfs_host(sa_share_impl_t impl_share, char *filename,
 	udata.callback = callback;
 	udata.sharepath = impl_share->sa_mountpoint;
 	udata.cookie = cookie;
-	udata.filename = filename;
+	udata.tmpfile = tmpfile;
 	udata.security = "sys";
 
 	shareopts = FSINFO(impl_share, nfs_fstype)->shareopts;
@@ -393,7 +393,7 @@ get_linux_shareopts(const char *shareopts, char **plinux_opts)
  * automatically exported upon boot or whenever the nfs server restarts.
  */
 static int
-nfs_add_entry(const char *filename, const char *sharepath,
+nfs_add_entry(FILE *tmpfile, const char *sharepath,
     const char *host, const char *security, const char *access_opts,
     void *pcookie)
 {
@@ -408,50 +408,29 @@ nfs_add_entry(const char *filename, const char *sharepath,
 	if (linux_opts == NULL)
 		linux_opts = "";
 
-	FILE *fp = fopen(filename, "a+e");
+	if (fprintf(tmpfile, "%s %s(sec=%s,%s,%s)\n", sharepath, linuxhost,
-	if (fp == NULL) {
-		fprintf(stderr, "failed to open %s file: %s", filename,
-		    strerror(errno));
-		free(linuxhost);
-		return (SA_SYSTEM_ERR);
-	}
-
-	if (fprintf(fp, "%s %s(sec=%s,%s,%s)\n", sharepath, linuxhost,
 	    security, access_opts, linux_opts) < 0) {
-		fprintf(stderr, "failed to write to %s\n", filename);
+		fprintf(stderr, "failed to write to temporary file\n");
 		free(linuxhost);
-		fclose(fp);
 		return (SA_SYSTEM_ERR);
 	}
 
 	free(linuxhost);
-	if (fclose(fp) != 0) {
-		fprintf(stderr, "Unable to close file %s: %s\n",
-		    filename, strerror(errno));
-		return (SA_SYSTEM_ERR);
-	}
 	return (SA_OK);
 }
 
 /*
- * This function copies all entries from the exports file to "filename",
+ * This function copies all entries from the exports file to newfp,
  * omitting any entries for the specified mountpoint.
  */
 int
-nfs_copy_entries(char *filename, const char *mountpoint)
+nfs_copy_entries(FILE *newfp, const char *mountpoint)
 {
 	char *buf = NULL;
 	size_t buflen = 0;
 	int error = SA_OK;
 
 	FILE *oldfp = fopen(ZFS_EXPORTS_FILE, "re");
-	FILE *newfp = fopen(filename, "w+e");
-	if (newfp == NULL) {
-		fprintf(stderr, "failed to open %s file: %s", filename,
-		    strerror(errno));
-		fclose(oldfp);
-		return (SA_SYSTEM_ERR);
-	}
 	fputs(FILE_HEADER, newfp);
 
 	/*
@@ -482,21 +461,15 @@ nfs_copy_entries(char *filename, const char *mountpoint)
 		}
 		if (fclose(oldfp) != 0) {
 			fprintf(stderr, "Unable to close file %s: %s\n",
-			    filename, strerror(errno));
+			    ZFS_EXPORTS_FILE, strerror(errno));
 			error = error != 0 ? error : SA_SYSTEM_ERR;
 		}
 	}
 
-	if (error == 0 && ferror(newfp) != 0) {
+	if (error == SA_OK && ferror(newfp) != 0)
 		error = ferror(newfp);
-	}
 
 	free(buf);
-	if (fclose(newfp) != 0) {
-		fprintf(stderr, "Unable to close file %s: %s\n",
-		    filename, strerror(errno));
-		error = error != 0 ? error : SA_SYSTEM_ERR;
-	}
 	return (error);
 }
 
@@ -504,7 +477,7 @@ nfs_copy_entries(char *filename, const char *mountpoint)
  * Enables NFS sharing for the specified share.
  */
 static int
-nfs_enable_share_impl(sa_share_impl_t impl_share, char *filename)
+nfs_enable_share_impl(sa_share_impl_t impl_share, FILE *tmpfile)
 {
 	char *shareopts, *linux_opts;
 	int error;
@@ -514,7 +487,7 @@ nfs_enable_share_impl(sa_share_impl_t impl_share, char *filename)
 	if (error != SA_OK)
 		return (error);
 
-	error = foreach_nfs_host(impl_share, filename, nfs_add_entry,
+	error = foreach_nfs_host(impl_share, tmpfile, nfs_add_entry,
 	    linux_opts);
 	free(linux_opts);
 	return (error);
@@ -532,7 +505,7 @@ nfs_enable_share(sa_share_impl_t impl_share)
  * Disables NFS sharing for the specified share.
  */
 static int
-nfs_disable_share_impl(sa_share_impl_t impl_share, char *filename)
+nfs_disable_share_impl(sa_share_impl_t impl_share, FILE *tmpfile)
 {
 	return (SA_OK);
 }

lib/libshare/os/linux/smb.c

@@ -254,7 +254,7 @@ smb_enable_share_one(const char *sharename, const char *sharepath)
 	argv[5] = (char *)name;
 	argv[6] = (char *)sharepath;
 	argv[7] = (char *)comment;
-	argv[8] = "Everyone:F";
+	argv[8] = (char *)"Everyone:F";
 	argv[9] = NULL;
 
 	rc = libzfs_run_process(argv[0], argv, 0);