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

[Yang Hongyang <yanghy@cn.fujitsu.com>]

Hi jeremy,

Thank you for your review.

On 05/22/2010 03:25 AM, Jeremy Fitzhardinge wrote:
> 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
...snip...
>>      }
>>  
>> -    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?

There's a problem. libxl_set_memory_target() takes memorykb as uint32_t,
so, if you want set as 'long long int', you should also modify this lib
function I think.
For this reason, when I set the memory to 100t, libxl_set_memory_target()
will take the memorykb as 0...

> 
>     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,
>>
>>   
> 
> 
> 


-- 
Regards
Yang Hongyang