----------------------------------------
Date: Thu, 9 May 2013 09:03:20 -0400 From: jferlan@redhat.com To: libvirt-cim@redhat.com Subject: Re: [Libvirt-cim] [PATCH V2 0/3] libvirt-cim patches
On 05/02/2013 03:10 AM, Xu Wang wrote:
V2 updates: 1. adjust comments for force use qemu (disable kvm under nested kvm) 2. fix the problem of cdrom (with blank or null disk) VSMS patch caused in V1 3. fix syntax errors for comments 4. status return updates 5. add description about 8 maximum items limits about vsi support output
Wenchao Xia (1): make force use qemu configurable
Xu Wang (2): VSMS: tip error for invalid disk resource make lldptool command and support output configurable
libvirt-cim.conf | 26 ++++++++++ libxkutil/misc_util.c | 26 ++++++++++ libxkutil/misc_util.h | 3 + src/Virt_SwitchService.c | 66 ++++++++++++++++++++---- src/Virt_VirtualSystemManagementService.c | 77 ++++++++++++++++++++++------- 5 files changed, 168 insertions(+), 30 deletions(-)
Very strange - this is the 2nd time patch 3/3 didn't make it to my email even though it's on the list. In fact my reply nor either of your responses to my reply made it to the mail server. It's as if it has some magic word or setting so that the mail server refuses to download it. I'll Cc you in every mail:-)
In any case, I'm still not convinced there's not a memory leak, but perhaps a Coverity or Valgrind run would answer that. I'll try to set up Coverity run on the sources. It's a static variable so every caller use the same memory space. It needn't released by me and shouldn't be freed because other caller may use it too.
Another consideration - perhaps the get_lldptool_query_options() and get_vsi_support_key_string() should return a char * of the strdup()'d prop.value string. You may also want to consider putting the defaults into those functions too to hide them from the caller.
For example, instead of:
+const char *get_lldptool_query_options(void) +{ + static LibvirtcimConfigProperty prop = { + "lldptool_query_options", CONFIG_STRING, {0}, 0}; + + libvirt_cim_config_get(&prop); + return prop.value_string; +} +
go with
char *get_lldptool_query_options(void) { static LibvirtcimConfigProperty prop = { "lldptool_query_options", CONFIG_STRING, {0}, 0};
libvirt_cim_config_get(&prop); if (prop.value_string) return strdup(prop.value_string);
return strdup("-t -g ncb -V evbcfg"); }
The caller checks for NULL return, errors appropriately and can free() the returned string. Yes, it maybe a feasible way to deal with this command. I discussed with Wenchao for our design ideas and at last we decided to use the old code. The reason is that we think get_xxx method just include fetch value of pointed keywords string and return it to the caller. It wouldn't contain any logic processing. We'll keep the coding style.
John
_______________________________________________ Libvirt-cim mailing list Libvirt-cim@redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim