On Thu, Apr 02, 2009 at 09:54:02AM +0100, Eric B Munson wrote:
> Currently hugeadm will happily remount hugetlbfs to the same directory
> if --create-*-mounts is specified more than once with the same arguments.
> This is a problem because the newest mount of a directory will mask the
> previous one(s) causing confusion when applications attempt tp share
> segements. This patch makes all the --create-*-mounts switches check
> to see if the requested directory is already mounted and if so skip
> the mount request.
>
> Signed-off-by: Eric B Munson <ebmun...@us.ibm.com>
> ---
> Changes from V2:
> Removed extra check of list pointer from mount_dir
>
> Changes from V1:
> Free list of mounts after mounted check fails
>
> hugeadm.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++----------
> 1 files changed, 56 insertions(+), 12 deletions(-)
>
> diff --git a/hugeadm.c b/hugeadm.c
> index 22f47a4..87ff62d 100644
> --- a/hugeadm.c
> +++ b/hugeadm.c
> @@ -181,11 +181,12 @@ void print_mounts(struct mount_list *current, int
> longest)
> }
> }
>
> -void mounts_list_all(void)
> +/* Caller is expected to free the list of mounts */
> +struct mount_list * collect_mounts(int *longest)
Minor - unnecessary whitespace between * and collect_mounts. checkpatch
complains about it.
No comment giving a hint as to what collect_mounts() actually does. It
needs a one-line description because "collect" doesn't really tell me
anything until I read the function itself in detail.
Because longest is a return parameter, it might even need a longer
description.
> {
> FILE *mounts;
> struct mount_list *list, *current, *previous = NULL;
> - int length, longest = MIN_COL;
> + int length;
>
> /* First try /proc/mounts, then /etc/mtab */
> mounts = setmntent(PROCMOUNTS, "r");
> @@ -209,8 +210,8 @@ void mounts_list_all(void)
> while (getmntent_r(mounts, &(current->entry), current->data,
> MAX_SIZE_MNTENT)) {
> if (strcasecmp(current->entry.mnt_type, FS_NAME) == 0) {
> length = strlen(current->entry.mnt_dir);
> - if (length > longest)
> - longest = length;
> + if (length > *longest)
> + *longest = length;
>
> current->next = malloc(sizeof(struct mount_list));
> if (!current->next) {
> @@ -228,16 +229,29 @@ void mounts_list_all(void)
> if (previous) {
> free(previous->next);
> previous->next = NULL;
> - print_mounts(list, longest);
> - } else {
> - /* No hugetlbfs mounts were found */
> - printf("No hugetlbfs mount point found.\n");
> + return list;
> }
>
> - current = list;
> - while (current) {
> - previous = current;
> - current = current->next;
> + return NULL;
> +}
> +
Whitespace at end of line there.
> +void mounts_list_all(void)
> +{
> + struct mount_list *list, *previous;
> + int longest = MIN_COL;
> +
> + list = collect_mounts(&longest);
> +
> + if (!list) {
> + ERROR("No hugetlbfs mount point found.\n");
> + return;
> + }
> +
> + print_mounts(list, longest);
> +
> + while (list) {
> + previous = list;
> + list = list->next;
> free(previous);
> }
> }
Overall, it feels like the patch up until here deserves to be a separate
patch and this split into two.
> @@ -315,12 +329,42 @@ int ensure_dir(char *path, mode_t mode, uid_t uid,
> gid_t gid)
> return 0;
> }
>
> +int check_mount(struct mount_list *list, char *path)
> +{
> + while (list) {
> + if (!strcmp(list->entry.mnt_dir, path))
> + return 1;
> + list = list->next;
> + }
> + return 0;
> +}
Either a better function name or a one-line description.
check_if_already_mounted() would be a more descriptive name otherwise a
reader will think "check mount for what?"
> +
> int mount_dir(char *path, char *options, mode_t mode, uid_t uid, gid_t gid)
> {
> struct passwd *pwd;
> struct group *grp;
> struct mntent entry;
> FILE *mounts;
> + struct mount_list *list, *previous;
> + int dummy = 0;
> +
> + list = collect_mounts(&dummy);
> +
> + if (list && check_mount(list, path)) {
> + INFO("Directory %s is already mounted\n", path);
I think this should be a warning. Otherwise without increasing the
verbosity, this will be easily missed.
> + while (list) {
> + previous = list;
> + list = list->next;
> + free(previous);
> + }
> + return 0;
> + }
> +
> + while (list) {
> + previous = list;
> + list = list->next;
> + free(previous);
> + }
>
> if (opt_dry_run) {
> pwd = getpwuid(uid);
Functionally, the patch seems fine though.
--
Mel Gorman
Part-time Phd Student Linux Technology Center
University of Limerick IBM Dublin Software Lab
------------------------------------------------------------------------------
_______________________________________________
Libhugetlbfs-devel mailing list
Libhugetlbfs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libhugetlbfs-devel
