From 1f4e1f7f0d66f583f3c7c943628cf8fb857606fc Mon Sep 17 00:00:00 2001 From: Alex Elder Date: Mon, 3 Oct 2011 12:49:18 +0000 Subject: [PATCH] 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 Reviewed-by: Christoph Hellwig --- libxcmd/paths.c | 150 +++++++++++++++++++++++++----------------------- 1 file changed, 77 insertions(+), 73 deletions(-) diff --git a/libxcmd/paths.c b/libxcmd/paths.c index 755307ef4..9160d0954 100644 --- 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 +/* + * 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); } } -- 2.47.2