[PATCH 0/5] xfsprogs: some libxcmd cleanups

[PATCH 0/5] xfsprogs: some libxcmd cleanups

[Alex Elder]

This series contains a few cleanups in libxcmd.  The main change
makes the device id's used in setting up a table of filesystems be
computed in a centralized and consistent way.  The third patch drops
support for optionally looking for a device in /dev, but as
explained there this was really not a useful feature.  Other than
that these patches should produce net change in behavior.

					-Alex

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

[PATCH 1/5] xfsprogs: libxcmd: don't clobber fs_table on realloc()

[Alex Elder]

In fs_table_insert(), realloc() is called to resize the global
fs_table.  If it fails, it overwrites a previously valid fs_table
pointer with NULL.

Instead, assign the return value to a local temporary and overwrite
fs_table only if the realloc() call succeeds.  The only defined
errno value for a realloc() failure is ENOMEM, so return that
explicitly in the event it fails.

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

diff --git a/libxcmd/paths.c b/libxcmd/paths.c
index 1dbe0be..0e02eb7 100644
--- a/libxcmd/paths.c
+++ b/libxcmd/paths.c
@@ -102,6 +102,7 @@ fs_table_insert(
 	char		*fsrt)
 {
 	dev_t		datadev, logdev, rtdev;
+	struct fs_path	*tmp_fs_table;
 
 	if (!dir || !fsname)
 		return EINVAL;
@@ -114,9 +115,10 @@ fs_table_insert(
 	if (fsrt && (fsrt = fs_device_number(fsrt, &rtdev, 1)) == NULL)
 		return errno;
 
-	fs_table = realloc(fs_table, sizeof(fs_path_t) * (fs_count + 1));
-	if (!fs_table)
-		return errno;
+	tmp_fs_table = realloc(fs_table, sizeof(fs_path_t) * (fs_count + 1));
+	if (!tmp_fs_table)
+		return ENOMEM;
+	fs_table = tmp_fs_table;
 
 	fs_path = &fs_table[fs_count];
 	fs_path->fs_dir = dir;
-- 
1.7.6.2

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

[PATCH 2/5] xfsprogs: libxcmd: simplify fs_table_lookup()

[Alex Elder]

Move a block of invariant code out of the loop in fs_table_lookup(),
and add a few comments.

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

diff --git a/libxcmd/paths.c b/libxcmd/paths.c
index 0e02eb7..5aa343b 100644
--- a/libxcmd/paths.c
+++ b/libxcmd/paths.c
@@ -37,6 +37,11 @@ struct fs_path *fs_path;
 char *mtab_file;
 #define PROC_MOUNTS	"/proc/self/mounts"
 
+/*
+ * Find the FS table entry for the given path.  The "flags" argument
+ * is a mask containing FS_MOUNT_POINT or FS_PROJECT_PATH (or both)
+ * to indicate the type of table entry sought.
+ */
 struct fs_path *
 fs_table_lookup(
 	const char	*dir,
@@ -44,16 +49,25 @@ fs_table_lookup(
 {
 	struct stat64	sbuf;
 	uint		i;
+	dev_t		dev;
 
 	if (stat64(dir, &sbuf) < 0)
 		return NULL;
+
+	/*
+	 * We want to match st_rdev if the directory provided is a
+	 * device special file.  Otherwise we are looking for the
+	 * the device id for the containing filesystem, in st_dev.
+	 */
+	if (S_ISBLK(sbuf.st_mode) || S_ISCHR(sbuf.st_mode))
+		dev = sbuf.st_rdev;
+	else
+		dev = sbuf.st_dev;
+
 	for (i = 0; i < fs_count; i++) {
 		if ((flags & fs_table[i].fs_flags) == 0)
 			continue;
-		if (S_ISBLK(sbuf.st_mode) || S_ISCHR(sbuf.st_mode)) {
-			if (sbuf.st_rdev == fs_table[i].fs_datadev)
-				return &fs_table[i];
-		} else if (sbuf.st_dev == fs_table[i].fs_datadev)
+		if (fs_table[i].fs_datadev == dev)
 			return &fs_table[i];
 	}
 	return NULL;
-- 
1.7.6.2

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

[PATCH 3/5] xfsprogs: libxcmd: kill "search" arg in fs_device_number()

[Alex Elder]

The function fs_device_number() in libxcmd allows the caller to
optionally "search" in /dev for a given device path in order to look
up the dev_t that represents that device path.

If set, all that function does is prepend "/dev/" to the path to see
if that produces a device path that works.  So it appears this might
have been to support providing just the basename of a device as a
shorthand for its full path.

In practice, the paths passed to this function with "search" set are
those used in the mount options for a mounted XFS filesystem for the
optional log and real-time device paths.  When such paths are used
in the XFS mount path, they will have been subject to a AT_FDCWD
path lookup, so unless the process mounting the filesystem was
sitting in /dev no relative path would ever be specified as just the
basename.

Even though the "mounting with CWD=/dev" is a conceivable scenario,
I think it is not likely enough to warrant the special handling to
cover that case in fs_device_number().

So delete the code that retries with a "/dev" prepended, eliminate
the "search" argument that enables it, and fix the callers
accordingly.

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

diff --git a/libxcmd/paths.c b/libxcmd/paths.c
index 5aa343b..f1cd6c7 100644
--- a/libxcmd/paths.c
+++ b/libxcmd/paths.c
@@ -76,33 +76,14 @@ fs_table_lookup(
 static char *
 fs_device_number(
 	char		*name,
-	dev_t		*devnum,
-	int		search)
+	dev_t		*devnum)
 {
 	struct stat64	sbuf;
-	int		len;
-
-	if (stat64(name, &sbuf) < 0) {
-		if (!search)
-			return NULL;
-		len = strlen(name) + 1;
-		name = realloc(name, len + 5);  /* "/dev/ */
-		if (!name) {
-			fprintf(stderr, _("%s: warning - out of memory\n"),
-				progname);
-			return NULL;
-		}
-		memmove(name + 5, name, len);
-		strncpy(name, "/dev/", 5);
-		if (stat64(name, &sbuf) < 0) {
-			fprintf(stderr,
-				_("%s: warning - cannot find %s: %s\n"),
-				progname, name, strerror(errno));
-			free(name);
-			return NULL;
-		}
-	}
+
+	if (stat64(name, &sbuf) < 0)
+		return NULL;
 	*devnum = sbuf.st_dev;
+
 	return name;
 }
 
@@ -122,11 +103,11 @@ fs_table_insert(
 		return EINVAL;
 
 	datadev = logdev = rtdev = 0;
-	if (!fs_device_number(dir, &datadev, 0))
+	if (!fs_device_number(dir, &datadev))
 		return errno;
-	if (fslog && (fslog = fs_device_number(fslog, &logdev, 1)) == NULL)
+	if (fslog && !fs_device_number(fslog, &logdev))
 		return errno;
-	if (fsrt && (fsrt = fs_device_number(fsrt, &rtdev, 1)) == NULL)
+	if (fsrt && !fs_device_number(fsrt, &rtdev))
 		return errno;
 
 	tmp_fs_table = realloc(fs_table, sizeof(fs_path_t) * (fs_count + 1));
-- 
1.7.6.2

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

[PATCH 4/5] xfsprogs: libxcmd: move fs_device_number() up in the file

[Alex Elder]

No content change, just code movement in preparation for the next
patch.

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

diff --git a/libxcmd/paths.c b/libxcmd/paths.c
index f1cd6c7..aa0aeb6 100644
--- a/libxcmd/paths.c
+++ b/libxcmd/paths.c
@@ -37,6 +37,20 @@ struct fs_path *fs_path;
 char *mtab_file;
 #define PROC_MOUNTS	"/proc/self/mounts"
 
+static char *
+fs_device_number(
+	char		*name,
+	dev_t		*devnum)
+{
+	struct stat64	sbuf;
+
+	if (stat64(name, &sbuf) < 0)
+		return NULL;
+	*devnum = sbuf.st_dev;
+
+	return name;
+}
+
 /*
  * Find the FS table entry for the given path.  The "flags" argument
  * is a mask containing FS_MOUNT_POINT or FS_PROJECT_PATH (or both)
@@ -73,20 +87,6 @@ fs_table_lookup(
 	return NULL;
 }
 
-static char *
-fs_device_number(
-	char		*name,
-	dev_t		*devnum)
-{
-	struct stat64	sbuf;
-
-	if (stat64(name, &sbuf) < 0)
-		return NULL;
-	*devnum = sbuf.st_dev;
-
-	return name;
-}
-
 static int
 fs_table_insert(
 	char		*dir,
-- 
1.7.6.2

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

[PATCH 5/5] xfsprogs: libxcmd: use fs_device_number() consistently

[Alex Elder]

The libxcmd code builds up a table that records information about
all filesystems that might be subject to quotas, as well as a set
of directories that are the roots of project quota trees.

When building the table, the device number for each affected
filesystem is determined (in fs_device_number()) using a call to
stat64().  It turns out that in all cases when doing this, a
directory path (and *not* a device special file path) is specified,
in which case the appropriate filesystem device id is found in the
st_dev field produce by the call to stat64() (i.e., the device id
for the mounted filesystem containing the path).  Accordingly,
fs_device_number() always returns the st_dev field.

Another routine, fs_table_lookup(), looks up an entry in this table
based on the path name provided.  However this function allows a
path to a device special file be provided.  In that case the right
device id to use is found in the st_rdev field returned by stat64().

I found this to be confusing, and it took a while to convince
myself that this wasn't actually bug.  (It wasn't initially clear
that device special files were never passed to fs_device_number().)

In order to prevent myself and others from ever wasting time like
this again, use fs_device_number() every time a device number is
needed, and in doing so determine it consistently in all cases (that
is--use st_rdev for device special files and st_dev otherwise).

In the process, change fs_device_number() to return an zero on
success (or an errno) rather than its first argument (or NULL).

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

diff --git a/libxcmd/paths.c b/libxcmd/paths.c
index aa0aeb6..f4110d4 100644
--- a/libxcmd/paths.c
+++ b/libxcmd/paths.c
@@ -37,18 +37,26 @@ struct fs_path *fs_path;
 char *mtab_file;
 #define PROC_MOUNTS	"/proc/self/mounts"
 
-static char *
+static int
 fs_device_number(
-	char		*name,
+	const char	*name,
 	dev_t		*devnum)
 {
 	struct stat64	sbuf;
 
 	if (stat64(name, &sbuf) < 0)
-		return NULL;
-	*devnum = sbuf.st_dev;
+		return errno;
+	/*
+	 * We want to match st_rdev if the path provided is a device
+	 * special file.  Otherwise we are looking for the the
+	 * device id for the containing filesystem, in st_dev.
+	 */
+	if (S_ISBLK(sbuf.st_mode) || S_ISCHR(sbuf.st_mode))
+		*devnum = sbuf.st_rdev;
+	else
+		*devnum = sbuf.st_dev;
 
-	return name;
+	return 0;
 }
 
 /*
@@ -61,23 +69,12 @@ fs_table_lookup(
 	const char	*dir,
 	uint		flags)
 {
-	struct stat64	sbuf;
 	uint		i;
-	dev_t		dev;
+	dev_t		dev = 0;
 
-	if (stat64(dir, &sbuf) < 0)
+	if (fs_device_number(dir, &dev))
 		return NULL;
 
-	/*
-	 * We want to match st_rdev if the directory provided is a
-	 * device special file.  Otherwise we are looking for the
-	 * the device id for the containing filesystem, in st_dev.
-	 */
-	if (S_ISBLK(sbuf.st_mode) || S_ISCHR(sbuf.st_mode))
-		dev = sbuf.st_rdev;
-	else
-		dev = sbuf.st_dev;
-
 	for (i = 0; i < fs_count; i++) {
 		if ((flags & fs_table[i].fs_flags) == 0)
 			continue;
@@ -103,11 +100,11 @@ fs_table_insert(
 		return EINVAL;
 
 	datadev = logdev = rtdev = 0;
-	if (!fs_device_number(dir, &datadev))
+	if (fs_device_number(dir, &datadev))
 		return errno;
-	if (fslog && !fs_device_number(fslog, &logdev))
+	if (fslog && fs_device_number(fslog, &logdev))
 		return errno;
-	if (fsrt && !fs_device_number(fsrt, &rtdev))
+	if (fsrt && fs_device_number(fsrt, &rtdev))
 		return errno;
 
 	tmp_fs_table = realloc(fs_table, sizeof(fs_path_t) * (fs_count + 1));
@@ -293,15 +290,14 @@ fs_mount_point_from_path(
 {
 	fs_cursor_t	cursor;
 	fs_path_t	*fs;
-	struct stat64	s;
+	dev_t		dev = 0;
 
-	if (stat64(dir, &s) < 0) {
+	if (fs_device_number(dir, &dev))
 		return NULL;
-	}
 
 	fs_cursor_initialise(NULL, FS_MOUNT_POINT, &cursor);
 	while ((fs = fs_cursor_next_entry(&cursor))) {
-		if (fs->fs_datadev == s.st_dev)
+		if (fs->fs_datadev == dev)
 			break;
 	}
 	return fs;
-- 
1.7.6.2

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

Re: [PATCH 1/5] xfsprogs: libxcmd: don't clobber fs_table on realloc()

[Christoph Hellwig]

On Wed, Sep 28, 2011 at 05:57:08AM -0500, Alex Elder wrote:
> In fs_table_insert(), realloc() is called to resize the global
> fs_table.  If it fails, it overwrites a previously valid fs_table
> pointer with NULL.
> 
> Instead, assign the return value to a local temporary and overwrite
> fs_table only if the realloc() call succeeds.  The only defined
> errno value for a realloc() failure is ENOMEM, so return that
> explicitly in the event it fails.

Looks good.  Did you encounter this issue in real life?

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

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

Re: [PATCH 2/5] xfsprogs: libxcmd: simplify fs_table_lookup()

[Christoph Hellwig]

On Wed, Sep 28, 2011 at 05:57:09AM -0500, Alex Elder wrote:
> Move a block of invariant code out of the loop in fs_table_lookup(),
> and add a few comments.
> 
> Signed-off-by: Alex Elder <aelder@sgi.com>

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 3/5] xfsprogs: libxcmd: kill "search" arg in fs_device_number()

[Christoph Hellwig]

I guess that code is some sort of IRIX leftover, but I can't really
see any use in a Linux environment.


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

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

Re: [PATCH 4/5] xfsprogs: libxcmd: move fs_device_number() up in the file

[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 5/5] xfsprogs: libxcmd: use fs_device_number() consistently

[Christoph Hellwig]

Yes, beeing consistant here is a good thing.


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

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

Re: [PATCH 1/5] xfsprogs: libxcmd: don't clobber fs_table on realloc()

[Alex Elder]

On Wed, 2011-09-28 at 20:59 -0400, Christoph Hellwig wrote:
> On Wed, Sep 28, 2011 at 05:57:08AM -0500, Alex Elder wrote:
> > In fs_table_insert(), realloc() is called to resize the global
> > fs_table.  If it fails, it overwrites a previously valid fs_table
> > pointer with NULL.
> > 
> > Instead, assign the return value to a local temporary and overwrite
> > fs_table only if the realloc() call succeeds.  The only defined
> > errno value for a realloc() failure is ENOMEM, so return that
> > explicitly in the event it fails.
> 
> Looks good.  Did you encounter this issue in real life?

No, just by inspection.

I'm working on fixing a different problem, and to do
so I need to rearrange things a bit.  Along the way
I found a bunch of little annoyances like this one,
so I figured I might as well fix them while I'm in
there.
  
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thanks a lot for the reviews.

					-Alex


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