Re: [Libhugetlbfs-devel] [PATCH] Add --verbose to hugeadm and allow the option to be specified in any order

[Mel Gorman]

On Fri, Apr 03, 2009 at 05:24:33PM -0700, Avantika Mathur wrote:
>

> This patch adds the --verbose/-v options to hugeadm.  When --verbose <0-99> is
> specified, the libhugetlbfs verbosity level is set to that value. The 
> verbosity
> level is increased by one each time -v is specified.  If the level is 
> increased to 99, this sets HUGETLB_DEBUG.
> The patch also also adds support for multiple options to hugeadm to be
> specified in any order.  Without this the verbose option would have to be
> specified before any other options.
> 
> Signed-off-by: Avantika Mathur 
> ---
> 
> Index: libhugetlbfs-save/hugeadm.c
> ===
> --- libhugetlbfs-save.orig/hugeadm.c  2009-04-03 16:43:43.0 -0700
> +++ libhugetlbfs-save/hugeadm.c   2009-04-03 16:43:46.0 -0700
> @@ -42,6 +42,13 @@
>  #include 
>  
>  #define REPORT_UTIL "hugeadm"
> +#define REPORT(level, prefix, format, ...)   
>  \
> + do {  \
> + if (verbose_level >= level)   \
> + fprintf(stderr, "hugeadm:" prefix ": " format,   \
> + ##__VA_ARGS__);   \
> + } while (0);
> +

Hmm, was it not possible to do something like

#define REPORT_UTIL "hugeadm"
#include "libhugetlbfs_internal.h"

and use the REPORT macro there?

Ahh, I see you were using hugectl as a template. Well, same question
applies really, is hugectl redefining REPORT unnecessarily?

Also worth checking out if the verbosity functions can be shared between
hugectl and hugeadm to reduce duplicated code a little.

>  #include "libhugetlbfs_internal.h"
>  #include "hugetlbfs.h"
>  
> @@ -102,11 +109,66 @@
>   OPTION("--explain", "Gives a overview of the status of the system");
>   CONT("with respect to huge page availability");
>  
> + OPTION("--verbose , -v", "Increases/sets tracing levels");
>   OPTION("--help, -h", "Prints this message");
>  }
>  
>  int opt_dry_run = 0;
>  int opt_hard = 0;
> +int verbose_level = VERBOSITY_DEFAULT;
> +
> +void setup_environment(char *var, char *val)
> +{
> + setenv(var, val, 1);
> + INFO("%s='%s'\n", var, val);
> +

If you are going to report the verbosity level, make it DEBUG. The
current verbosity level is not interesting enough to be INFO.

> + if (opt_dry_run)
> + printf("%s='%s'\n", var, val);
> +}
> +
> +void verbose_init(void)
> +{
> + char *env;
> +
> + env = getenv("HUGETLB_VERBOSE");
> + if (env)
> + verbose_level = atoi(env);
> + env = getenv("HUGETLB_DEBUG");
> + if (env)
> + verbose_level = VERBOSITY_MAX;
> +}
> +
> +void verbose(char *which)
> +{
> + int new_level;
> +
> + if (which) {
> + new_level = atoi(which);
> + if (new_level < 0 || new_level > 99) {
> + ERROR("%d: verbosity out of range 0-99\n",
> + new_level);
> + exit(EXIT_FAILURE);
> + }
> + } else {
> + new_level = verbose_level + 1;
> + if (new_level == 100) {
> + WARNING("verbosity limited to 99\n");
> + new_level--;
> + }
> + }
> + verbose_level = new_level;
> +}
> +
> +void verbose_expose(void)
> +{
> + char level[3];
> +
> + if (verbose_level == 99) {
> + setup_environment("HUGETLB_DEBUG", "yes");
> + }
> + snprintf(level, sizeof(level), "%d", verbose_level);
> + setup_environment("HUGETLB_VERBOSE", level);
> +}
>  
>  /*
>   * getopts return values for options which are long only.
> @@ -635,12 +697,17 @@
>   int ops;
>   int has_hugepages = kernel_has_hugepages();
>  
> - char opts[] = "+hd";
> + char opts[] = "+hdv";
>   char base[PATH_MAX];
> - char *opt_min_adj[MAX_POOLS];
> - int ret = 0, index = 0, minadj_count = 0;
> + char *opt_min_adj[MAX_POOLS], *opt_max_adj[MAX_POOLS];
> + char *opt_user_mounts = NULL, *opt_group_mounts = NULL;
> + int opt_list_mounts = 0, opt_pool_list = 0, opt_create_mounts = 0;
> + int opt_global_mounts = 0, opt_pgsizes = 0, opt_pgsizes_all = 0;
> + int opt_explain = 0, minadj_count = 0, maxadj_count = 0;
> + int ret = 0, index = 0;
>   struct option long_opts[] = {
>   {"help",   no_argument, NULL, 'h'},
> + {"verbose",required_argument, NULL, 'v' },
>  
>   {"list-all-mounts", no_argument, NULL, LONG_LIST_ALL_MOUNTS},
>   {"pool-list", no_argument, NULL, LONG_POOL_LIST},
> @@ -662,6 +729,7 @@
>  
>   hugetlbfs_setup_debug();
>   setup_mounts();
> + verbose_init();
>  
>   ops = 0;
>   while (ret != -1) {
> @@ -678,6 +746,10 @@
>   print_usage();
>   exit(EXIT_SUCCESS);
>  

Re: [Libhugetlbfs-devel] [PATCH V3] Check if directory is already mounted

[Mel Gorman]

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 
> ---
> 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 = ge

[Libhugetlbfs-devel] Super SSensual Love Making In Bed

[Leone Hoistion]

Mega Secrets To Super Sensual Love Making In Bed - Be Absoluteely Mind Blowing


Trees standing here. Forsaking every object of least, christina
seemed to intimate as much and.
--
___
Libhugetlbfs-devel mailing list
Libhugetlbfs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libhugetlbfs-devel