[PATCH 0/6] xfsprogs: tolerate mount or project errors

[PATCH 0/6] xfsprogs: tolerate mount or project errors

[Alex Elder]

This series adds a few more cleanups to libxcmd, but they (and the
ones recently) were really just ground work leading up to addressing
a specific underlying problem, which is done in the last patch (or
maybe the last two).

libxcmd includes code to initialize a table with entries that
represent mount points and project directories that may be subject
to quota enforcement.  These are then available as targets of
various operations.

When adding an entry to the table, various things can go wrong.
For example, a path defined in the projects file might not exist,
a mount point specified might not be accessible to the user, or
a memory allocation could fail.

Currently, if any error occurs when inserting an entry into this
"fs_table", exit() is called so the executing program just quits.

Since this table is simply used to define the available targets
of operations, there is no real need to exit when an error occurs
setting up just one of its entries.  In some cases everything else
might be just fine, and normally the entry that causes an error is
not even something the user cares to operate on anyway.

The trouble report that led to fixing this had to do with a
situation in which automount left some sort of artifact in the list
of mounted filesystems, and any attempt to run xfs_growfs was met
with an error that prevented it from being run.  The errant entry in
/proc/self/mounts contained something like this in the mnt_fsname
field returned by getmntent(): "/tmp/auto7fGuI5 (deleted)"

This series addresses this by both ignoring errors while
initializing fs_table, and by not exiting when an error occurs.

					-Alex

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 1/6] xfsprogs: libxcmd: rearrange some routines

[Alex Elder]

Move the definition of a few routines around in the file to avoid
forward references in upcoming patches.

Signed-off-by: Alex Elder <aelder@sgi.com>
---
 libxcmd/paths.c |  162 ++++++++++++++++++++++++++++---------------------------
 1 files changed, 82 insertions(+), 80 deletions(-)

diff --git a/libxcmd/paths.c b/libxcmd/paths.c
index 1e78099..ae9db32 100644
--- a/libxcmd/paths.c
+++ b/libxcmd/paths.c
@@ -143,6 +143,59 @@ fs_table_destroy(void)
 	fs_count = 0;
 }
 
+/*
+ * Table iteration (cursor-based) interfaces
+ */
+
+/*
+ * Initialize an fs_table cursor.  If a directory path is supplied,
+ * the cursor is set up to appear as though the table contains only
+ * a single entry which represents the directory specified.
+ * Otherwise it is set up to prepare for visiting all entries in the
+ * global table, starting with the first.  "flags" can be either
+ * FS_MOUNT_POINT or FS_PROJECT_PATH to limit what type of entries
+ * will be selected by fs_cursor_next_entry().  0 can be used as a
+ * wild card (selecting either type).
+ */
+void
+fs_cursor_initialise(
+	char		*dir,
+	uint		flags,
+	fs_cursor_t	*cur)
+{
+	fs_path_t	*path;
+
+	memset(cur, 0, sizeof(*cur));
+	if (dir) {
+		if ((path = fs_table_lookup(dir, flags)) == NULL)
+			return;
+		cur->local = *path;
+		cur->count = 1;
+		cur->table = &cur->local;
+	} else {
+		cur->count = fs_count;
+		cur->table = fs_table;
+	}
+	cur->flags = flags;
+}
+
+/*
+ * Use the cursor to find the next entry in the table having the
+ * type specified by the cursor's "flags" field.
+ */
+struct fs_path *
+fs_cursor_next_entry(
+	fs_cursor_t	*cur)
+{
+	while (cur->index < cur->count) {
+		fs_path_t	*next = &cur->table[cur->index++];
+
+		if (!cur->flags || (cur->flags & next->fs_flags))
+			return next;
+	}
+	return NULL;
+}
+
 
 #if defined(HAVE_GETMNTENT)
 #include <mntent.h>
@@ -303,6 +356,21 @@ fs_mount_point_from_path(
 	return fs;
 }
 
+void
+fs_table_insert_mount(
+	char		*mount)
+{
+	int		error;
+
+	error = fs_table_initialise_mounts(mount);
+	if (error) {
+		fs_table_destroy();
+		fprintf(stderr, _("%s: cannot setup path for mount %s: %s\n"),
+			progname, mount, strerror(error));
+		exit(1);
+	}
+}
+
 static int
 fs_table_initialise_projects(
 	char		*project)
@@ -348,52 +416,37 @@ fs_table_initialise_projects(
 }
 
 void
-fs_table_initialise(void)
+fs_table_insert_project(
+	char		*project)
 {
 	int		error;
 
-	error = fs_table_initialise_mounts(NULL);
-	if (!error)
-		error = fs_table_initialise_projects(NULL);
-	if (error) {
-		fs_table_destroy();
-		fprintf(stderr, _("%s: cannot initialise path table: %s\n"),
-			progname, strerror(error));
+	if (!fs_count) {
+		fprintf(stderr, _("%s: no mount table yet, so no projects\n"),
+			progname);
 		exit(1);
 	}
-}
-
-void
-fs_table_insert_mount(
-	char		*mount)
-{
-	int		error;
-
-	error = fs_table_initialise_mounts(mount);
+	error = fs_table_initialise_projects(project);
 	if (error) {
 		fs_table_destroy();
-		fprintf(stderr, _("%s: cannot setup path for mount %s: %s\n"),
-			progname, mount, strerror(error));
+		fprintf(stderr, _("%s: cannot setup path for project %s: %s\n"),
+			progname, project, strerror(error));
 		exit(1);
 	}
 }
 
 void
-fs_table_insert_project(
-	char		*project)
+fs_table_initialise(void)
 {
 	int		error;
 
-	if (!fs_count) {
-		fprintf(stderr, _("%s: no mount table yet, so no projects\n"),
-			progname);
-		exit(1);
-	}
-	error = fs_table_initialise_projects(project);
+	error = fs_table_initialise_mounts(NULL);
+	if (!error)
+		error = fs_table_initialise_projects(NULL);
 	if (error) {
 		fs_table_destroy();
-		fprintf(stderr, _("%s: cannot setup path for project %s: %s\n"),
-			progname, project, strerror(error));
+		fprintf(stderr, _("%s: cannot initialise path table: %s\n"),
+			progname, strerror(error));
 		exit(1);
 	}
 }
@@ -428,55 +481,4 @@ fs_table_insert_project_path(
 		exit(1);
 	}
 }
-/*
- * Table iteration (cursor-based) interfaces
- */
 
-/*
- * Initialize an fs_table cursor.  If a directory path is supplied,
- * the cursor is set up to appear as though the table contains only
- * a single entry which represents the directory specified.
- * Otherwise it is set up to prepare for visiting all entries in the
- * global table, starting with the first.  "flags" can be either
- * FS_MOUNT_POINT or FS_PROJECT_PATH to limit what type of entries
- * will be selected by fs_cursor_next_entry().  0 can be used as a
- * wild card (selecting either type).
- */
-void
-fs_cursor_initialise(
-	char		*dir,
-	uint		flags,
-	fs_cursor_t	*cur)
-{
-	fs_path_t	*path;
-
-	memset(cur, 0, sizeof(*cur));
-	if (dir) {
-		if ((path = fs_table_lookup(dir, flags)) == NULL)
-			return;
-		cur->local = *path;
-		cur->count = 1;
-		cur->table = &cur->local;
-	} else {
-		cur->count = fs_count;
-		cur->table = fs_table;
-	}
-	cur->flags = flags;
-}
-
-/*
- * Use the cursor to find the next entry in the table having the
- * type specified by the cursor's "flags" field.
- */
-struct fs_path *
-fs_cursor_next_entry(
-	fs_cursor_t	*cur)
-{
-	while (cur->index < cur->count) {
-		fs_path_t	*next = &cur->table[cur->index++];
-
-		if (!cur->flags || (cur->flags & next->fs_flags))
-			return next;
-	}
-	return NULL;
-}
-- 
1.7.6.2

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/6] xfsprogs: libxcmd: avoid using strtok()

[Alex Elder]

The strtok() library routine overwrites delimiting bytes in the
string it is supplied.  It is also not length-constrained.

Since we're making a duplicate of the string anyway, and since we
are only finding the end of a single token, we can do both without
the need to modify the passed-in mount entry structure.

Add checking for memory allocation failures, and if one occurs just
exit (as is the practice elsewhere in this file).

Signed-off-by: Alex Elder <aelder@sgi.com>
---
 libxcmd/paths.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/libxcmd/paths.c b/libxcmd/paths.c
index ae9db32..ed93110 100644
--- a/libxcmd/paths.c
+++ b/libxcmd/paths.c
@@ -216,12 +216,21 @@ fs_extract_mount_options(
 
 	/* Do this only after we've finished processing mount options */
 	if (fslog) {
-		strtok(fslog, " ,");
-		fslog = strdup(fslog);
+		fslog = strndup(fslog, strcspn(fslog, " ,"));
+		if (!fslog) {
+			fprintf(stderr, _("%s: %s: out of memory (fslog)\n"),
+				    progname, __func__);
+			exit(1);
+		}
 	}
 	if (fsrt) {
-		strtok(fsrt, " ,");
-		fsrt = strdup(fsrt);
+		fsrt = strndup(fsrt, strcspn(fsrt, " ,"));
+		if (!fsrt) {
+			fprintf(stderr, _("%s: %s: out of memory (fsrt)\n"),
+				    progname, __func__);
+			free(fslog);
+			exit(1);
+		}
 	}
 
 	*logp = fslog;
-- 
1.7.6.2

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/6] xfsprogs: libxcmd: avoid using strtok()

[Christoph Hellwig]

Looks reasonable.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 3/6] xfsprogs: libxcmd: encapsulate fs_table initialization

[Alex Elder]

Change fs_table_initialise() so it takes an array of mount points
and an array of project identifiers as arguments (along with their
respective sizes).

Change the quota code to provide fs_table_initialise() these arrays
rather than doing the individual mount point and project insertion
by itself.  Other users just pass 0 counts, which results in filling
fs_table with entries for all mounted filesystems and all defined
projects.

This allows a few fs_table functions to be given private scope.

Signed-off-by: Alex Elder <aelder@sgi.com>
---
 growfs/xfs_growfs.c |    2 +-
 include/path.h      |    4 +---
 io/parent.c         |    2 +-
 libxcmd/paths.c     |   49 ++++++++++++++++++++++++++++++++++++++-----------
 quota/init.c        |   15 ++-------------
 5 files changed, 43 insertions(+), 29 deletions(-)

diff --git a/growfs/xfs_growfs.c b/growfs/xfs_growfs.c
index 98ce5df..a6d298b 100644
--- a/growfs/xfs_growfs.c
+++ b/growfs/xfs_growfs.c
@@ -193,7 +193,7 @@ main(int argc, char **argv)
 	if (dflag + lflag + rflag == 0)
 		aflag = 1;
 
-	fs_table_initialise();
+	fs_table_initialise(0, NULL, 0, NULL);
 	fs = fs_table_lookup(argv[optind], FS_MOUNT_POINT);
 	if (!fs) {
 		fprintf(stderr, _("%s: %s is not a mounted XFS filesystem\n"),
diff --git a/include/path.h b/include/path.h
index b8c8b31..757ba49 100644
--- a/include/path.h
+++ b/include/path.h
@@ -47,11 +47,9 @@ extern fs_path_t *fs_table;	/* array of entries in fs table  */
 extern fs_path_t *fs_path;	/* current entry in the fs table */
 extern char *mtab_file;
 
-extern void fs_table_initialise(void);
+extern void fs_table_initialise(int, char *[], int, char *[]);
 extern void fs_table_destroy(void);
 
-extern void fs_table_insert_mount(char *__mount);
-extern void fs_table_insert_project(char *__project);
 extern void fs_table_insert_project_path(char *__dir, uint __projid);
 
 
diff --git a/io/parent.c b/io/parent.c
index 5d356e6..47faaa0 100644
--- a/io/parent.c
+++ b/io/parent.c
@@ -377,7 +377,7 @@ parent_f(int argc, char **argv)
 
 	if (!tab_init) {
 		tab_init = 1;
-		fs_table_initialise();
+		fs_table_initialise(0, NULL, 0, NULL);
 	}
 	fs = fs_table_lookup(file->name, FS_MOUNT_POINT);
 	if (!fs) {
diff --git a/libxcmd/paths.c b/libxcmd/paths.c
index ed93110..755307e 100644
--- a/libxcmd/paths.c
+++ b/libxcmd/paths.c
@@ -365,7 +365,7 @@ fs_mount_point_from_path(
 	return fs;
 }
 
-void
+static void
 fs_table_insert_mount(
 	char		*mount)
 {
@@ -424,7 +424,7 @@ fs_table_initialise_projects(
 	return error;
 }
 
-void
+static void
 fs_table_insert_project(
 	char		*project)
 {
@@ -444,20 +444,47 @@ fs_table_insert_project(
 	}
 }
 
+/*
+ * Initialize fs_table to contain the given set of mount points and
+ * projects.  If mount_count is zero, mounts is ignored and the
+ * table is populated with mounted filesystems.  If project_count is
+ * zero, projects is ignored and the table is populated with all
+ * projects defined in the projects file.
+ */
 void
-fs_table_initialise(void)
+fs_table_initialise(
+	int	mount_count,
+	char	*mounts[],
+	int	project_count,
+	char	*projects[])
 {
-	int		error;
+	int	error;
+	int	i;
 
-	error = fs_table_initialise_mounts(NULL);
-	if (!error)
+	if (mount_count) {
+		for (i = 0; i < mount_count; i++)
+			fs_table_insert_mount(mounts[i]);
+	} else {
+		error = fs_table_initialise_mounts(NULL);
+		if (error)
+			goto out_exit;
+	}
+	if (project_count) {
+		for (i = 0; i < project_count; i++)
+			fs_table_insert_project(projects[i]);
+	} else {
 		error = fs_table_initialise_projects(NULL);
-	if (error) {
-		fs_table_destroy();
-		fprintf(stderr, _("%s: cannot initialise path table: %s\n"),
-			progname, strerror(error));
-		exit(1);
+		if (error)
+			goto out_exit;
 	}
+
+	return;
+
+out_exit:
+	fs_table_destroy();
+	fprintf(stderr, _("%s: cannot initialise path table: %s\n"),
+		progname, strerror(error));
+	exit(1);
 }
 
 void 
diff --git a/quota/init.c b/quota/init.c
index 96b6389..3293b90 100644
--- a/quota/init.c
+++ b/quota/init.c
@@ -135,19 +135,8 @@ init(
 		}
 	}
 
-	if (optind == argc) {
-		fs_table_initialise();
-	} else {
-		while (optind < argc)
-			fs_table_insert_mount(argv[optind++]);
-		if (!nprojopts)
-			fs_table_insert_project(NULL);
-		else
-			for (c = 0; c < nprojopts; c++)
-				fs_table_insert_project(projopts[c]);
-	}
-	if (projopts)
-		free(projopts);
+	fs_table_initialise(argc - optind, &argv[optind], nprojopts, projopts);
+	free(projopts);
 
 	init_commands();
 	add_args_command(init_args_command);
-- 
1.7.6.2

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/6] xfsprogs: libxcmd: encapsulate fs_table initialization

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 4/6] xfsprogs: libxcmd: isolate strdup() calls to fs_table_insert()

[Alex Elder]

Calls to fs_table_insert() are made in four places, and in all four
the mount directory and device name arguments passed are the result
of calls to strdup().  Rather than have all the callers handle
allocating and freeing of these strings, consolidate that into
fs_table_insert().

Only one place passes non-null values for the fslog and fsrt
arguments, and in that case it's easier to keep the allocation of
duplicate strings where they are in the caller.  Add a comment in
fs_table_insert() to ensure that's understood.

Note also that fs_table_insert() is always called with both its
dir and fsname arguments non-null, so drop a check for that at
the top of the function.

Signed-off-by: Alex Elder <aelder@sgi.com>
---
 libxcmd/paths.c |  138 +++++++++++++++++++++++++++----------------------------
 1 files changed, 67 insertions(+), 71 deletions(-)

diff --git a/libxcmd/paths.c b/libxcmd/paths.c
index 755307e..13873ef 100644
--- a/libxcmd/paths.c
+++ b/libxcmd/paths.c
@@ -95,21 +95,37 @@ fs_table_insert(
 {
 	dev_t		datadev, logdev, rtdev;
 	struct fs_path	*tmp_fs_table;
-
-	if (!dir || !fsname)
-		return EINVAL;
+	int		error;
 
 	datadev = logdev = rtdev = 0;
-	if (fs_device_number(dir, &datadev))
-		return errno;
-	if (fslog && fs_device_number(fslog, &logdev))
-		return errno;
-	if (fsrt && fs_device_number(fsrt, &rtdev))
-		return errno;
+	error = fs_device_number(dir, &datadev);
+	if (error)
+		goto out_nodev;
+	if (fslog && (error = fs_device_number(fslog, &logdev)))
+		goto out_nodev;
+	if (fsrt && (error = fs_device_number(fsrt, &rtdev)))
+		goto out_nodev;
+
+	/*
+	 * Make copies of the directory and data device path.
+	 * The log device and real-time device, if non-null,
+	 * are already the result of strdup() calls so we
+	 * don't need to duplicate those.  In fact, this
+	 * function is assumed to "consume" both of those
+	 * pointers, meaning if an error occurs they will
+	 * both get freed.
+	 */
+	error = ENOMEM;
+	dir = strdup(dir);
+	if (!dir)
+		goto out_nodev;
+	fsname = strdup(fsname);
+	if (!fsname)
+		goto out_noname;
 
 	tmp_fs_table = realloc(fs_table, sizeof(fs_path_t) * (fs_count + 1));
 	if (!tmp_fs_table)
-		return ENOMEM;
+		goto out_norealloc;
 	fs_table = tmp_fs_table;
 
 	fs_path = &fs_table[fs_count];
@@ -123,7 +139,19 @@ fs_table_insert(
 	fs_path->fs_logdev = logdev;
 	fs_path->fs_rtdev = rtdev;
 	fs_count++;
+
 	return 0;
+
+out_norealloc:
+	free(fsname);
+out_noname:
+	free(dir);
+out_nodev:
+	/* "Consume" fslog and fsrt even if there's an error */
+	free(fslog);
+	free(fsrt);
+
+	return error;
 }
 
 void
@@ -200,6 +228,14 @@ fs_cursor_next_entry(
 #if defined(HAVE_GETMNTENT)
 #include <mntent.h>
 
+/*
+ * Determines whether the "logdev" or "rtdev" mount options are
+ * present for the given mount point.  If so, the value for each (a
+ * device path) is returned in the pointers whose addresses are
+ * provided.  The pointers are assigned NULL for an option not
+ * present.  Note that the path buffers returned are allocated
+ * dynamically and it is the caller's responsibility to free them.
+ */
 static void
 fs_extract_mount_options(
 	struct mntent	*mnt,
@@ -243,11 +279,11 @@ fs_table_initialise_mounts(
 {
 	struct mntent	*mnt;
 	FILE		*mtp;
-	char		*dir, *fsname, *fslog, *fsrt;
+	char		*fslog, *fsrt;
 	int		error, found;
 
 	error = found = 0;
-	dir = fsname = fslog = fsrt = NULL;
+	fslog = fsrt = NULL;
 
 	if (!mtab_file) {
 		mtab_file = PROC_MOUNTS;
@@ -266,26 +302,16 @@ fs_table_initialise_mounts(
 		     (strcmp(path, mnt->mnt_fsname) != 0)))
 			continue;
 		found = 1;
-		dir = strdup(mnt->mnt_dir);
-		fsname = strdup(mnt->mnt_fsname);
-		if (!dir || !fsname) {
-			error = ENOMEM;
-			break;
-		}
 		fs_extract_mount_options(mnt, &fslog, &fsrt);
-		if ((error = fs_table_insert(dir, 0, FS_MOUNT_POINT,
-						fsname, fslog, fsrt)))
+		error = fs_table_insert(mnt->mnt_dir, 0, FS_MOUNT_POINT,
+					mnt->mnt_fsname, fslog, fsrt);
+		if (error)
 			break;
 	}
 	endmntent(mtp);
 	if (!error && path && !found)
 		error = ENXIO;
-	if (error) {
-		if (dir) free(dir);
-		if (fsrt) free(fsrt);
-		if (fslog) free(fslog);
-		if (fsname) free(fsname);
-	}
+
 	return error;
 }
 
@@ -297,12 +323,9 @@ fs_table_initialise_mounts(
 	char		*path)
 {
 	struct statfs	*stats;
-	char		*dir, *fsname, *fslog, *fsrt;
 	int		i, count, error, found;
 
 	error = found = 0;
-	dir = fsname = fslog = fsrt = NULL;
-
 	if ((count = getmntinfo(&stats, 0)) < 0) {
 		fprintf(stderr, _("%s: getmntinfo() failed: %s\n"),
 				progname, strerror(errno));
@@ -317,25 +340,16 @@ fs_table_initialise_mounts(
 		     (strcmp(path, stats[i].f_mntfromname) != 0)))
 			continue;
 		found = 1;
-		dir = strdup(stats[i].f_mntonname);
-		fsname = strdup(stats[i].f_mntfromname);
-		if (!dir || !fsname) {
-			error = ENOMEM;
-			break;
-		}
 		/* TODO: external log and realtime device? */
-		if ((error = fs_table_insert(dir, 0, FS_MOUNT_POINT,
-						fsname, fslog, fsrt)))
+		error = fs_table_insert(stats[i].f_mntonname, 0,
+					FS_MOUNT_POINT, stats[i].f_mntfromname,
+					NULL, NULL);
+		if (error)
 			break;
 	}
 	if (!error && path && !found)
 		error = ENXIO;
-	if (error) {
-		if (dir) free(dir);
-		if (fsrt) free(fsrt);
-		if (fslog) free(fslog);
-		if (fsname) free(fsname);
-	}
+
 	return error;
 }
 
@@ -387,7 +401,6 @@ fs_table_initialise_projects(
 	fs_project_path_t *path;
 	fs_path_t	*fs;
 	prid_t		prid = 0;
-	char		*dir = NULL, *fsname = NULL;
 	int		error = 0, found = 0;
 
 	if (project)
@@ -403,24 +416,17 @@ fs_table_initialise_projects(
 			continue;
 		}
 		found = 1;
-		dir = strdup(path->pp_pathname);
-		fsname = strdup(fs->fs_name);
-		if (!dir || !fsname) {
-			error = ENOMEM;
-			break;
-		}
-		if ((error = fs_table_insert(dir, path->pp_prid,
-					FS_PROJECT_PATH, fsname, NULL, NULL)))
+		error = fs_table_insert(path->pp_pathname, path->pp_prid,
+					FS_PROJECT_PATH, fs->fs_name,
+					NULL, NULL);
+		if (error)
 			break;
 	}
 	endprpathent();
 
 	if (!error && project && !found)
 		error = ENOENT;
-	if (error) {
-		if (dir) free(dir);
-		if (fsname) free(fsname);
-	}
+
 	return error;
 }
 
@@ -489,31 +495,21 @@ out_exit:
 
 void 
 fs_table_insert_project_path(
-	char		*udir,
+	char		*dir,
 	prid_t		prid)
 {
 	fs_path_t	*fs;
-	char		*dir = NULL, *fsname = NULL;
 	int		error = 0;
 
-	if ((fs = fs_mount_point_from_path(udir)) != NULL) {
-		dir = strdup(udir);
-		fsname = strdup(fs->fs_name);
-		if (dir && fsname)
-			error = fs_table_insert(dir, prid,
-					FS_PROJECT_PATH, fsname, NULL, NULL);
-		else
-			error = ENOMEM;
+	if ((fs = fs_mount_point_from_path(dir)) != NULL) {
+		error = fs_table_insert(dir, prid, FS_PROJECT_PATH,
+					fs->fs_name, NULL, NULL);
 	} else
 		error = ENOENT;
 
 	if (error) {
-		if (dir)
-			free(dir);
-		if (fsname)
-			free(fsname);
 		fprintf(stderr, _("%s: cannot setup path for project dir %s: %s\n"),
-				progname, udir, strerror(error));
+				progname, dir, strerror(error));
 		exit(1);
 	}
 }
-- 
1.7.6.2

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/6] xfsprogs: libxcmd: isolate strdup() calls to fs_table_insert()

[Christoph Hellwig]

> @@ -95,21 +95,37 @@ fs_table_insert(
>  {
>  	dev_t		datadev, logdev, rtdev;
>  	struct fs_path	*tmp_fs_table;
> -
> -	if (!dir || !fsname)
> -		return EINVAL;
> +	int		error;
>  
>  	datadev = logdev = rtdev = 0;
> -	if (fs_device_number(dir, &datadev))
> -		return errno;
> -	if (fslog && fs_device_number(fslog, &logdev))
> -		return errno;
> -	if (fsrt && fs_device_number(fsrt, &rtdev))
> -		return errno;
> +	error = fs_device_number(dir, &datadev);
> +	if (error)
> +		goto out_nodev;
> +	if (fslog && (error = fs_device_number(fslog, &logdev)))
> +		goto out_nodev;
> +	if (fsrt && (error = fs_device_number(fsrt, &rtdev)))
> +		goto out_nodev;

If you touch these anyway please move assignments outside the
conditionals, e.g.

	 if (fslog) {
	 	error = fs_device_number(fslog, &logdev);
		if (error)
			goto out_nodev;
	}

> -	char		*dir = NULL, *fsname = NULL;
>  	int		error = 0;
>  
> -	if ((fs = fs_mount_point_from_path(udir)) != NULL) {
> -		dir = strdup(udir);
> -		fsname = strdup(fs->fs_name);
> -		if (dir && fsname)
> -			error = fs_table_insert(dir, prid,
> -					FS_PROJECT_PATH, fsname, NULL, NULL);
> -		else
> -			error = ENOMEM;
> +	if ((fs = fs_mount_point_from_path(dir)) != NULL) {

Same here.

Otherwise looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 4/6] xfsprogs: libxcmd: isolate strdup() calls to fs_table_insert()

[Alex Elder]

On Thu, 2011-10-06 at 15:45 -0400, Christoph Hellwig wrote:
...

> If you touch these anyway please move assignments outside the
> conditionals, e.g.

OK.  No problem.  I was opting for less indent depth
but *almost* did what you say in the first place...

					-Alex

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 5/6] xfsprogs: libxcmd: avoid exiting when an error occurs

[Alex Elder]

In a number of spots handling setting up fs_table, libxcmd simply
prints a message and exits if an error occurs.  There should be no
real need to exit in these cases.  Notifying the user that something
went wrong is appropriate but this should not preclude continued
operation.  In a few cases the contents of fs_table built up so far
are discarded as well, and this too can be avoided.

Make it so errors do not lead to exits, nor do they result in
destroying fs_table.  Doing this requires returning a value from
fs_extract_mount_options() so its caller can skip other processing
in this case.  But in most cases we simply no longer exit, and no
longer destroy the fs_table.  This means there is no more use for
fs_table_destroy(), so it can be removed.

There is a sort of short-circuit exit in fs_table_insert_project()
that is unnecessary as well, so get rid of it.

Signed-off-by: Alex Elder <aelder@sgi.com>
---
 libxcmd/paths.c |   66 ++++++++++++++++--------------------------------------
 1 files changed, 20 insertions(+), 46 deletions(-)

diff --git a/libxcmd/paths.c b/libxcmd/paths.c
index 13873ef..a6adc51 100644
--- a/libxcmd/paths.c
+++ b/libxcmd/paths.c
@@ -154,23 +154,6 @@ out_nodev:
 	return error;
 }
 
-void
-fs_table_destroy(void)
-{
-	while (--fs_count >= 0) {
-		free(fs_table[fs_count].fs_name);
-		if (fs_table[fs_count].fs_log)
-			free(fs_table[fs_count].fs_log);
-		if (fs_table[fs_count].fs_rt)
-			free(fs_table[fs_count].fs_rt);
-		free(fs_table[fs_count].fs_dir);
-	}
-	if (fs_table)
-		free(fs_table);
-	fs_table = NULL;
-	fs_count = 0;
-}
-
 /*
  * Table iteration (cursor-based) interfaces
  */
@@ -236,7 +219,7 @@ fs_cursor_next_entry(
  * present.  Note that the path buffers returned are allocated
  * dynamically and it is the caller's responsibility to free them.
  */
-static void
+static int
 fs_extract_mount_options(
 	struct mntent	*mnt,
 	char		**logp,
@@ -253,24 +236,27 @@ fs_extract_mount_options(
 	/* Do this only after we've finished processing mount options */
 	if (fslog) {
 		fslog = strndup(fslog, strcspn(fslog, " ,"));
-		if (!fslog) {
-			fprintf(stderr, _("%s: %s: out of memory (fslog)\n"),
-				    progname, __func__);
-			exit(1);
-		}
+		if (!fslog)
+			goto out_nomem;
 	}
 	if (fsrt) {
 		fsrt = strndup(fsrt, strcspn(fsrt, " ,"));
 		if (!fsrt) {
-			fprintf(stderr, _("%s: %s: out of memory (fsrt)\n"),
-				    progname, __func__);
 			free(fslog);
-			exit(1);
+			goto out_nomem;
 		}
 	}
-
 	*logp = fslog;
 	*rtp = fsrt;
+
+	return 0;
+
+out_nomem:
+	*logp = NULL;
+	*rtp = NULL;
+	fprintf(stderr, _("%s: unable to extract mount options for \"%s\"\n"),
+		progname, mnt->mnt_dir);
+	return ENOMEM;
 }
 
 static int
@@ -302,7 +288,8 @@ fs_table_initialise_mounts(
 		     (strcmp(path, mnt->mnt_fsname) != 0)))
 			continue;
 		found = 1;
-		fs_extract_mount_options(mnt, &fslog, &fsrt);
+		if (fs_extract_mount_options(mnt, &fslog, &fsrt))
+			continue;
 		error = fs_table_insert(mnt->mnt_dir, 0, FS_MOUNT_POINT,
 					mnt->mnt_fsname, fslog, fsrt);
 		if (error)
@@ -386,12 +373,9 @@ fs_table_insert_mount(
 	int		error;
 
 	error = fs_table_initialise_mounts(mount);
-	if (error) {
-		fs_table_destroy();
+	if (error)
 		fprintf(stderr, _("%s: cannot setup path for mount %s: %s\n"),
 			progname, mount, strerror(error));
-		exit(1);
-	}
 }
 
 static int
@@ -436,18 +420,10 @@ fs_table_insert_project(
 {
 	int		error;
 
-	if (!fs_count) {
-		fprintf(stderr, _("%s: no mount table yet, so no projects\n"),
-			progname);
-		exit(1);
-	}
 	error = fs_table_initialise_projects(project);
-	if (error) {
-		fs_table_destroy();
+	if (error)
 		fprintf(stderr, _("%s: cannot setup path for project %s: %s\n"),
 			progname, project, strerror(error));
-		exit(1);
-	}
 }
 
 /*
@@ -473,7 +449,7 @@ fs_table_initialise(
 	} else {
 		error = fs_table_initialise_mounts(NULL);
 		if (error)
-			goto out_exit;
+			goto out_error;
 	}
 	if (project_count) {
 		for (i = 0; i < project_count; i++)
@@ -481,16 +457,14 @@ fs_table_initialise(
 	} else {
 		error = fs_table_initialise_projects(NULL);
 		if (error)
-			goto out_exit;
+			goto out_error;
 	}
 
 	return;
 
-out_exit:
-	fs_table_destroy();
+out_error:
 	fprintf(stderr, _("%s: cannot initialise path table: %s\n"),
 		progname, strerror(error));
-	exit(1);
 }
 
 void 
-- 
1.7.6.2

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 5/6] xfsprogs: libxcmd: avoid exiting when an error occurs

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 6/6] xfsprogs: libxcmd: ignore errors when initializing fs_table

[Alex Elder]

When initializing fs_table, the full set of mounted filesystems and
the full set of defined projects are (or may be) examined.  If an
error ever occurs looking at one of these entries, the processing
loop just quits, skipping all remaining mounts or projects.

One mount or project being problematic is no reason to give
up entirely.  It may be that it is completely unrelated to
the mount point or project that the user wants to operate on.

So instead of quitting when an error occurs while adding
something to fs_table, proceed until all entries are added.

Meanwhile, the two affected functions are used for either
installing one entry in the table or for initializing the
table based on the full set of mounts or projects.  In
the former case, once the entry matching that was requested
has been found there is no need to continue searching for
other entries, so break out of the loop immediately in
that case.

It so happens that these two changes affect the exact
same portion of the code...

SGI PV 1017024

Signed-off-by: Alex Elder <aelder@sgi.com>
---
 libxcmd/paths.c |   27 +++++++++++++++------------
 1 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/libxcmd/paths.c b/libxcmd/paths.c
index a6adc51..2afd9bb 100644
--- a/libxcmd/paths.c
+++ b/libxcmd/paths.c
@@ -287,16 +287,17 @@ fs_table_initialise_mounts(
 		    ((strcmp(path, mnt->mnt_dir) != 0) &&
 		     (strcmp(path, mnt->mnt_fsname) != 0)))
 			continue;
-		found = 1;
 		if (fs_extract_mount_options(mnt, &fslog, &fsrt))
 			continue;
-		error = fs_table_insert(mnt->mnt_dir, 0, FS_MOUNT_POINT,
+		(void) fs_table_insert(mnt->mnt_dir, 0, FS_MOUNT_POINT,
 					mnt->mnt_fsname, fslog, fsrt);
-		if (error)
+		if (path) {
+			found = 1;
 			break;
+		}
 	}
 	endmntent(mtp);
-	if (!error && path && !found)
+	if (path && !found)
 		error = ENXIO;
 
 	return error;
@@ -326,15 +327,16 @@ fs_table_initialise_mounts(
 		    ((strcmp(path, stats[i].f_mntonname) != 0) &&
 		     (strcmp(path, stats[i].f_mntfromname) != 0)))
 			continue;
-		found = 1;
 		/* TODO: external log and realtime device? */
-		error = fs_table_insert(stats[i].f_mntonname, 0,
+		(void) fs_table_insert(stats[i].f_mntonname, 0,
 					FS_MOUNT_POINT, stats[i].f_mntfromname,
 					NULL, NULL);
-		if (error)
+		if (path) {
+			found = 1;
 			break;
+		}
 	}
-	if (!error && path && !found)
+	if (path && !found)
 		error = ENXIO;
 
 	return error;
@@ -399,16 +401,17 @@ fs_table_initialise_projects(
 					progname, path->pp_pathname, strerror(errno));
 			continue;
 		}
-		found = 1;
-		error = fs_table_insert(path->pp_pathname, path->pp_prid,
+		(void) fs_table_insert(path->pp_pathname, path->pp_prid,
 					FS_PROJECT_PATH, fs->fs_name,
 					NULL, NULL);
-		if (error)
+		if (project) {
+			found = 1;
 			break;
+		}
 	}
 	endprpathent();
 
-	if (!error && project && !found)
+	if (project && !found)
 		error = ENOENT;
 
 	return error;
-- 
1.7.6.2

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 6/6] xfsprogs: libxcmd: ignore errors when initializing fs_table

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/6] xfsprogs: libxcmd: rearrange some routines

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 0/6] xfsprogs: tolerate mount or project errors

[Christoph Hellwig]

On Mon, Oct 03, 2011 at 07:49:14AM -0500, Alex Elder wrote:
> The trouble report that led to fixing this had to do with a
> situation in which automount left some sort of artifact in the list
> of mounted filesystems, and any attempt to run xfs_growfs was met
> with an error that prevented it from being run.  The errant entry in
> /proc/self/mounts contained something like this in the mnt_fsname
> field returned by getmntent(): "/tmp/auto7fGuI5 (deleted)"

I'd really like to do some sort of QA for this.  Given that xfs_quota
already has an (uncodumented) -t flag to use a replacement for
/proc/mounts and /etc/mtab this seems fairly easy to do.

If you're motivated for even more cleanups it would be good if all
xfs_quota options are actully documented, and making sure the
/proc/mounts, /etc/mtab and co handling in xfsprogs doesn't differ
for different tools.  Currently libxcmd, libxfs and xfs_fsr all
have their own variants.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 0/6] xfsprogs: tolerate mount or project errors

[Alex Elder]

On Mon, 2011-10-03 at 09:31 -0400, Christoph Hellwig wrote:
> On Mon, Oct 03, 2011 at 07:49:14AM -0500, Alex Elder wrote:
> > The trouble report that led to fixing this had to do with a
> > situation in which automount left some sort of artifact in the list
> > of mounted filesystems, and any attempt to run xfs_growfs was met
> > with an error that prevented it from being run.  The errant entry in
> > /proc/self/mounts contained something like this in the mnt_fsname
> > field returned by getmntent(): "/tmp/auto7fGuI5 (deleted)"
> 
> I'd really like to do some sort of QA for this.  Given that xfs_quota
> already has an (uncodumented) -t flag to use a replacement for
> /proc/mounts and /etc/mtab this seems fairly easy to do.

Yes I agree, and that's exactly what I intend to do (and how I
was thinking of doing it).  I also owe a quota test for the
doubling of what gets reported from a month ago or so.  I
thought I could address both issues in the same test (unless
someone feels it's important to have each test have a more
specific focus).

> If you're motivated for even more cleanups it would be good if all
> xfs_quota options are actully documented, and making sure the
> /proc/mounts, /etc/mtab and co handling in xfsprogs doesn't differ
> for different tools.  Currently libxcmd, libxfs and xfs_fsr all
> have their own variants.

To be honest I kind of went further with this than I intended
to and I had to sort of put a stop to it...  I have three or
four other cleanup patches started but I just have to move on
and so posted what I have working and am content with.

I think what you suggest are all good but for now I'm not
planning to work on them.

					-Alex



_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 0/6] xfsprogs: tolerate mount or project errors

[Christoph Hellwig]

On Mon, Oct 03, 2011 at 09:21:25AM -0500, Alex Elder wrote:
> > I'd really like to do some sort of QA for this.  Given that xfs_quota
> > already has an (uncodumented) -t flag to use a replacement for
> > /proc/mounts and /etc/mtab this seems fairly easy to do.
> 
> Yes I agree, and that's exactly what I intend to do (and how I
> was thinking of doing it).  I also owe a quota test for the
> doubling of what gets reported from a month ago or so.  I
> thought I could address both issues in the same test (unless
> someone feels it's important to have each test have a more
> specific focus).

I think these should be separate tests as they test very different
things.  What could be rolled into the first one would be a test
for RT quota reporting - despite the comment in your commit those
should work just fine.

> > If you're motivated for even more cleanups it would be good if all
> > xfs_quota options are actully documented, and making sure the
> > /proc/mounts, /etc/mtab and co handling in xfsprogs doesn't differ
> > for different tools.  Currently libxcmd, libxfs and xfs_fsr all
> > have their own variants.
> 
> To be honest I kind of went further with this than I intended
> to and I had to sort of put a stop to it...  I have three or
> four other cleanup patches started but I just have to move on
> and so posted what I have working and am content with.
> 
> I think what you suggest are all good but for now I'm not
> planning to work on them.

It's not that important, but maybe we should keep a todo list
for these kinds of tidyups.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs