Re: [PATCH]xl: Some small fixes to xl

[Jeremy Fitzhardinge <jeremy@goop.org>]

On 05/21/2010 12:01 AM, Yang Hongyang wrote:
> The patch fixes following problems:
> -When use mem-set, I got suspicious error output:
>  # xl mem-set 1 256g
>  setting domid 1 memory to : 268435456
>  [0] libxl.c:2535:libxl_set_memory_target: memory_dynamic_max must be less than or equal to memory_static_max
>  : Success
> -parse_mem_size_kb() returns type int64_t
> -String generated by strdup() should be freed
> -When using 'xl help', mem-max and mem-set's output is not as intend, and it's also
>  breaks bash completion, fix it.
>
> diff -r 840f269d95fb tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c	Wed May 19 22:59:52 2010 +0100
> +++ b/tools/libxl/libxl.c	Fri May 21 22:56:02 2010 +0800
> @@ -2531,7 +2531,7 @@
>          }
>  
>          if (target_memkb > memorykb) {
> -            XL_LOG_ERRNO(ctx, XL_LOG_ERROR,
> +            XL_LOG(ctx, XL_LOG_ERROR,
>                  "memory_dynamic_max must be less than or equal to memory_static_max\n");
>              return 1;
>          }
> diff -r 840f269d95fb tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c	Wed May 19 22:59:52 2010 +0100
> +++ b/tools/libxl/xl_cmdimpl.c	Fri May 21 22:56:02 2010 +0800
> @@ -1300,7 +1300,7 @@
>  
>  void set_memory_target(char *p, char *mem)
>  {
> -    long long int memorykb;
> +    int64_t memorykb;
>  
>      find_domain(p);
>  
> @@ -1310,7 +1310,7 @@
>          exit(3);
>      }
>  
> -    printf("setting domid %d memory to : %lld\n", domid, memorykb);
> +    printf("setting domid %d memory to : %ld\n", domid, memorykb);
>   

This is wrong; "int64_t" isn't necessarily long.  What was wrong with
the previous version?

    J

>      libxl_set_memory_target(&ctx, domid, memorykb, /* enforce */ 1);
>  }
>  
> @@ -3644,7 +3644,8 @@
>      int fd;
>      char buf[512];
>      uint32_t uptime = 0;
> -    char *uptime_str = 0;
> +    char *uptime_str = NULL;
> +    char *now_str = NULL;
>  
>      fd = open("/proc/uptime", O_RDONLY);
>      if (fd == -1)
> @@ -3661,9 +3662,10 @@
>  
>      if (short_mode)
>      {
> +        now_str = current_time_to_string(now);
>          uptime_str = uptime_to_string(uptime, 1);
> -        printf(" %s up %s, %s (%d)\n", current_time_to_string(now),
> -               uptime_str, libxl_domid_to_name(&ctx, 0), 0);
> +        printf(" %s up %s, %s (%d)\n", now_str, uptime_str,
> +               libxl_domid_to_name(&ctx, 0), 0);
>      }
>      else
>      {
> @@ -3672,6 +3674,8 @@
>                 0, uptime_str);
>      }
>  
> +    if (now_str)
> +        free(now_str);
>      if (uptime_str)
>          free(uptime_str);
>      return;
> @@ -3684,7 +3688,8 @@
>  {
>      uint32_t s_time = 0;
>      uint32_t uptime = 0;
> -    char *uptime_str = 0;
> +    char *uptime_str = NULL;
> +    char *now_str = NULL;
>  
>      s_time = libxl_vm_get_start_time(&ctx, domuid);
>      if (s_time == -1)
> @@ -3692,9 +3697,10 @@
>      uptime = now - s_time;
>      if (short_mode)
>      {
> +        now_str = current_time_to_string(now);
>          uptime_str = uptime_to_string(uptime, 1);
> -        printf(" %s up %s, %s (%d)\n", current_time_to_string(now),
> -               uptime_str, libxl_domid_to_name(&ctx, domuid), domuid);
> +        printf(" %s up %s, %s (%d)\n", now_str, uptime_str,
> +               libxl_domid_to_name(&ctx, domuid), domuid);
>      }
>      else
>      {
> @@ -3703,6 +3709,8 @@
>                 domuid, uptime_str);
>      }
>  
> +    if (now_str)
> +        free(now_str);
>      if (uptime_str)
>          free(uptime_str);
>      return;
> diff -r 840f269d95fb tools/libxl/xl_cmdtable.c
> --- a/tools/libxl/xl_cmdtable.c	Wed May 19 22:59:52 2010 +0100
> +++ b/tools/libxl/xl_cmdtable.c	Fri May 21 22:56:02 2010 +0800
> @@ -110,17 +110,13 @@
>      },
>      { "mem-max",
>        &main_memmax,
> -      "Set the maximum amount reservation for a domain.\n"
> -      "Units default to kilobytes, but can be suffixed with\n"
> -      "'b' (bytes), 'k' (KB), 'm' (MB), 'g' (GB) or 't' (TB)",
> -      "<Domain> <MemKB>",
> +      "Set the maximum amount reservation for a domain",
> +      "<Domain> <MemKB['b'[bytes]|'k'[KB]|'m'[MB]|'g'[GB]|'t'[TB]]>",
>      },
>      { "mem-set",
>        &main_memset,
> -      "Set the current memory usage for a domain.\n"
> -      "Units default to kilobytes, but can be suffixed with\n"
> -      "'b' (bytes), 'k' (KB), 'm' (MB), 'g' (GB) or 't' (TB)",
> -      "<Domain> <MemKB>",
> +      "Set the current memory usage for a domain",
> +      "<Domain> <MemKB['b'[bytes]|'k'[KB]|'m'[MB]|'g'[GB]|'t'[TB]]>",
>      },
>      { "button-press",
>        &main_button_press,
>
>