tools/xl:Making _dispose function simplicity for libxl_dominfo

PATCH In review Uma Sharma 3 participants 4 messages Oct 26, 2014 – Oct 27, 2014

Share & tools

Uma Sharma

author

2014-10-26 18:55 UTC #0 lore raw

PATCH 1/1 tools/xl:Making _dispose function simplicity for libxl_dominfo

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 <[email protected]>

tools/libxl/xl_cmdimpl.c +2 -6

index 5325a52..9a6ceb3 100644
@@ -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;
 }

Ian Campbell

2014-10-27 09:47 UTC #1 lore raw

Quoted 13 lines · 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 <[email protected]>
--
tools/libxl/xl_cmdimpl.c +0 -0
index 5325a52..9a6ceb3 100644
@@ -4375,12 +4375,8 @@ int main_list(int argc, char **argv)
         list_domains_details(info, nb_domain);

tools/libxl/xl_cmdimpl.c int main_list(int argc, char **argv) · line 4379

Quoted 10 lines

tools/libxl/xl_cmdimpl.c +2 -6

     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.

Andrew Cooper

2014-10-27 10:06 UTC #2 lore raw

In reply to Uma Sharma

Quoted 13 lines · 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 <[email protected]>
--
tools/libxl/xl_cmdimpl.c +0 -0
index 5325a52..9a6ceb3 100644
@@ -4375,12 +4375,8 @@ int main_list(int argc, char **argv)
         list_domains_details(info, nb_domain);

tools/libxl/xl_cmdimpl.c int main_list(int argc, char **argv) · line 4379

Quoted 10 lines

tools/libxl/xl_cmdimpl.c +2 -6

     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

Uma Sharma

author

2014-10-27 21:41 UTC #3 lore raw

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

Regards,
Uma Sharma

Quoted 33 lines · Andrew Cooper

Quoted 13 lines · 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 <[email protected]>
--
tools/libxl/xl_cmdimpl.c +0 -0
index 5325a52..9a6ceb3 100644
@@ -4375,12 +4375,8 @@ int main_list(int argc, char **argv)
         list_domains_details(info, nb_domain);
tools/libxl/xl_cmdimpl.c int main_list(int argc, char **argv) · line 4379
Quoted 10 lines
tools/libxl/xl_cmdimpl.c +2 -6
     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