* [PATCH 2/5] xfsprogs: libxcmd: simplify fs_table_lookup()
2011-09-28 10:57 ` [PATCH 1/5] xfsprogs: libxcmd: don't clobber fs_table on realloc() Alex Elder
@ 2011-09-28 10:57 ` Alex Elder
2011-09-29 1:00 ` Christoph Hellwig
2011-09-28 10:57 ` [PATCH 3/5] xfsprogs: libxcmd: kill "search" arg in fs_device_number() Alex Elder
` (3 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Alex Elder @ 2011-09-28 10:57 UTC (permalink / raw)
To: xfs; +Cc: 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
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 3/5] xfsprogs: libxcmd: kill "search" arg in fs_device_number()
2011-09-28 10:57 ` [PATCH 1/5] xfsprogs: libxcmd: don't clobber fs_table on realloc() Alex Elder
2011-09-28 10:57 ` [PATCH 2/5] xfsprogs: libxcmd: simplify fs_table_lookup() Alex Elder
@ 2011-09-28 10:57 ` Alex Elder
2011-09-29 1:01 ` Christoph Hellwig
2011-09-28 10:57 ` [PATCH 4/5] xfsprogs: libxcmd: move fs_device_number() up in the file Alex Elder
` (2 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Alex Elder @ 2011-09-28 10:57 UTC (permalink / raw)
To: xfs; +Cc: 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
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 4/5] xfsprogs: libxcmd: move fs_device_number() up in the file
2011-09-28 10:57 ` [PATCH 1/5] xfsprogs: libxcmd: don't clobber fs_table on realloc() Alex Elder
2011-09-28 10:57 ` [PATCH 2/5] xfsprogs: libxcmd: simplify fs_table_lookup() Alex Elder
2011-09-28 10:57 ` [PATCH 3/5] xfsprogs: libxcmd: kill "search" arg in fs_device_number() Alex Elder
@ 2011-09-28 10:57 ` Alex Elder
2011-09-29 1:01 ` Christoph Hellwig
2011-09-28 10:57 ` [PATCH 5/5] xfsprogs: libxcmd: use fs_device_number() consistently Alex Elder
2011-09-29 0:59 ` [PATCH 1/5] xfsprogs: libxcmd: don't clobber fs_table on realloc() Christoph Hellwig
4 siblings, 1 reply; 12+ messages in thread
From: Alex Elder @ 2011-09-28 10:57 UTC (permalink / raw)
To: xfs; +Cc: 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
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 5/5] xfsprogs: libxcmd: use fs_device_number() consistently
2011-09-28 10:57 ` [PATCH 1/5] xfsprogs: libxcmd: don't clobber fs_table on realloc() Alex Elder
` (2 preceding siblings ...)
2011-09-28 10:57 ` [PATCH 4/5] xfsprogs: libxcmd: move fs_device_number() up in the file Alex Elder
@ 2011-09-28 10:57 ` Alex Elder
2011-09-29 1:03 ` Christoph Hellwig
2011-09-29 0:59 ` [PATCH 1/5] xfsprogs: libxcmd: don't clobber fs_table on realloc() Christoph Hellwig
4 siblings, 1 reply; 12+ messages in thread
From: Alex Elder @ 2011-09-28 10:57 UTC (permalink / raw)
To: xfs; +Cc: 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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 1/5] xfsprogs: libxcmd: don't clobber fs_table on realloc()
2011-09-28 10:57 ` [PATCH 1/5] xfsprogs: libxcmd: don't clobber fs_table on realloc() Alex Elder
` (3 preceding siblings ...)
2011-09-28 10:57 ` [PATCH 5/5] xfsprogs: libxcmd: use fs_device_number() consistently Alex Elder
@ 2011-09-29 0:59 ` Christoph Hellwig
2011-09-29 14:10 ` Alex Elder
4 siblings, 1 reply; 12+ messages in thread
From: Christoph Hellwig @ 2011-09-29 0:59 UTC (permalink / raw)
To: Alex Elder; +Cc: xfs
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 1/5] xfsprogs: libxcmd: don't clobber fs_table on realloc()
2011-09-29 0:59 ` [PATCH 1/5] xfsprogs: libxcmd: don't clobber fs_table on realloc() Christoph Hellwig
@ 2011-09-29 14:10 ` Alex Elder
0 siblings, 0 replies; 12+ messages in thread
From: Alex Elder @ 2011-09-29 14:10 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
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
^ permalink raw reply [flat|nested] 12+ messages in thread