Re: [Libvirt-cim] [PATCH V2 0/3] libvirt-cim patches
----------------------------------------
Date: Thu, 9 May 2013 09:03:20 -0400
From: jferlan(a)redhat.com
To: libvirt-cim(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvirt-cim