[OPW PATCH] tools/xl:Making _dispose function simplicity for libxl_dominfo

[OPW PATCH] tools/xl:Making _dispose function simplicity for libxl_dominfo

[Uma Sharma]

This patch simplifies the call to dispose for libxl_dominfo *info.
_dispose was called multiple times in tools/libxl/xl_cmdimpl.c
IDL generated libxl types should be used only after calling the init
function.

Signed-off-by: Uma Sharma <uma.sharma523@gmail.com>
--
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 5325a52..9a6ceb3 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -4375,12 +4375,8 @@ int main_list(int argc, char **argv)
         list_domains_details(info, nb_domain);
     else
         list_domains(verbose, context, 0 /* claim */, numa, info, nb_domain);
-
-    if (info_free)
-        libxl_dominfo_list_free(info, nb_domain);
-    else
-	libxl_dominfo_dispose(info);
-
+    
+    libxl_dominfo_list_free(info, nb_domain);
     return 0;
 }

Re: [OPW PATCH] tools/xl:Making _dispose function simplicity for libxl_dominfo

[Ian Campbell]

On Mon, 2014-10-27 at 00:25 +0530, Uma Sharma wrote:
> This patch simplifies the call to dispose for libxl_dominfo *info.
> _dispose was called multiple times in tools/libxl/xl_cmdimpl.c
> IDL generated libxl types should be used only after calling the init
> function.
> 
> Signed-off-by: Uma Sharma <uma.sharma523@gmail.com>
> --
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 5325a52..9a6ceb3 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -4375,12 +4375,8 @@ int main_list(int argc, char **argv)
>          list_domains_details(info, nb_domain);
>      else
>          list_domains(verbose, context, 0 /* claim */, numa, info, nb_domain);
> -
> -    if (info_free)
> -        libxl_dominfo_list_free(info, nb_domain);
> -    else
> -	libxl_dominfo_dispose(info);
> -
> +    
> +    libxl_dominfo_list_free(info, nb_domain);

I don't think this is right, libxl_dominfo_list_free will dispose each
of the array members and then free the memory corresponding to the array
itself. But when info_free is false info is a pointer to a static buffer
on the stack, not to a dynamically allocated array.

Even if the change is correct, it doesn't seem to correspond to the
commit message.

Ian.

Re: [OPW PATCH] tools/xl:Making _dispose function simplicity for libxl_dominfo

[Andrew Cooper]

On 26/10/14 18:55, Uma Sharma wrote:
> This patch simplifies the call to dispose for libxl_dominfo *info.
> _dispose was called multiple times in tools/libxl/xl_cmdimpl.c
> IDL generated libxl types should be used only after calling the init
> function.
>
> Signed-off-by: Uma Sharma <uma.sharma523@gmail.com>
> --
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 5325a52..9a6ceb3 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -4375,12 +4375,8 @@ int main_list(int argc, char **argv)
>          list_domains_details(info, nb_domain);
>      else
>          list_domains(verbose, context, 0 /* claim */, numa, info, nb_domain);
> -
> -    if (info_free)
> -        libxl_dominfo_list_free(info, nb_domain);
> -    else
> -	libxl_dominfo_dispose(info);
> -
> +    
> +    libxl_dominfo_list_free(info, nb_domain);

Unrelated to the code in the patch, observe from the first - and first +
that you have introduced trailing whitespace with this change, which
should be avoided.

If you can, configure your editor to highlight trailing whitespace.

~Andrew

Re: [OPW PATCH] tools/xl:Making _dispose function simplicity for libxl_dominfo

[Uma Sharma]

Apologies for the wrong commit message. I will look into the patch
again and try to improve it.

Regards,
Uma Sharma

On Mon, Oct 27, 2014 at 3:36 PM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 26/10/14 18:55, Uma Sharma wrote:
>> This patch simplifies the call to dispose for libxl_dominfo *info.
>> _dispose was called multiple times in tools/libxl/xl_cmdimpl.c
>> IDL generated libxl types should be used only after calling the init
>> function.
>>
>> Signed-off-by: Uma Sharma <uma.sharma523@gmail.com>
>> --
>> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
>> index 5325a52..9a6ceb3 100644
>> --- a/tools/libxl/xl_cmdimpl.c
>> +++ b/tools/libxl/xl_cmdimpl.c
>> @@ -4375,12 +4375,8 @@ int main_list(int argc, char **argv)
>>          list_domains_details(info, nb_domain);
>>      else
>>          list_domains(verbose, context, 0 /* claim */, numa, info, nb_domain);
>> -
>> -    if (info_free)
>> -        libxl_dominfo_list_free(info, nb_domain);
>> -    else
>> -     libxl_dominfo_dispose(info);
>> -
>> +
>> +    libxl_dominfo_list_free(info, nb_domain);
>
> Unrelated to the code in the patch, observe from the first - and first +
> that you have introduced trailing whitespace with this change, which
> should be avoided.
>
> If you can, configure your editor to highlight trailing whitespace.
>
> ~Andrew
>