xfsprogs: libxcmd: isolate strdup() calls to fs_table_insert()

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>
Reviewed-by: Christoph Hellwig <hch@lst.de>

diff --git a/libxcmd/paths.c b/libxcmd/paths.c
index 755307ef43a82346dcdc521b805b98ab40882c46..9160d09544ac10c93e40b49b3c0e3cee8702ddea 100644 (file)
--- a/libxcmd/paths.c
+++ b/libxcmd/paths.c
@@ -95,21 +95,43 @@ 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);
+               if (error)
+                       goto out_nodev;
+       }
+       if (fsrt) {
+               error = fs_device_number(fsrt, &rtdev);
+               if (error)
+                       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 +145,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 +234,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 +285,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 +308,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 +329,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 +346,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 +407,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)
@@ -397,30 +416,24 @@ fs_table_initialise_projects(
        while ((path = getprpathent()) != NULL) {
                if (project && prid != path->pp_prid)
                        continue;
-               if ((fs = fs_mount_point_from_path(path->pp_pathname)) == NULL) {
+               fs = fs_mount_point_from_path(path->pp_pathname);
+               if (!fs) {
                        fprintf(stderr, _("%s: cannot find mount point for path `%s': %s\n"),
                                        progname, path->pp_pathname, strerror(errno));
                        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 +502,22 @@ 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;
-       } else
+       fs = fs_mount_point_from_path(dir);
+       if (fs)
+               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);
        }
 }