[Libvirt-cim] [PATCH] Add LibvirtVersion to VSMS

[Kaitlin Rupert]

Jim Fehlig wrote:
> Kaitlin Rupert wrote:
>> Thanks Jim! This looks good, I've got a minor change below...
>>
>>> @@ -2483,6 +2485,25 @@
>>> CMSetProperty(inst, "Caption",
>>> (CMPIValue *)"Unknown Hypervisor", CMPI_chars);
>>>
>>> + if (virGetVersion(&lv_version, hv_type, &hv_version) < 0) {
>>> + CU_DEBUG("Unable to get libvirt version");
>>> + lv_version= 0;
>>> + hv_version= 0;
>>> + }
>>> +
>>> + if (asprintf(&lv_version_string, "%lu.%lu.%lu",
>>> + lv_version / 1000000,
>>> + (lv_version % 1000000) / 1000,
>>> + (lv_version % 1000000) % 1000) == -1)
>>> + lv_version_string = NULL;
>>> +
>>> + if (lv_version_string != NULL)
>>> + CMSetProperty(inst, "LibvirtVersion",
>>> + (CMPIValue *)lv_version_string, CMPI_chars);
>>> + else
>>> + CMSetProperty(inst, "LibvirtVersion",
>>> + (CMPIValue *)"Unknown libvirt", CMPI_chars);
>>> +
>> Instead of the if / else here, in the case when lv_version_string, can
>> you assign it the value of "Unknown libvirt". Then you can just call
>> CMSetProperty() with lv_version_string, as lv_version_string shouldn't
>> be NULL at that point. This approach is more inline with the
>> convention followed up the rest of the code.
> 
> I was following the convention established by the Caption property,

As, you're right.  My mistake.  Generally, we do something like:

if (str == NULL)
	str = strdup(val);

//Call CMSetProperty() here

> which includes the hypervisor version. If asprintf() fails (most likely
> because of memory allocation failure), I would need to dup "Unkown
> libvirt version" in lv_version_string, which would also fail :-). I
> could assign the static string to another variable, but I don't think
> that is any better than the current logic.

Yes, agreed.  It's fine to leave it as is.  I missed seeing that Caption 
was assigned using this approach.

> 
>> Also, I think "Unknown libvirt version" is a little more clear.
> 
> Agreed. I'll wait for your advice on the above before submitting a
> follow up patch.
> 
> Thanks,
> Jim
> 
> _______________________________________________
> Libvirt-cim mailing list
> Libvirt-cim at redhat.com
> https://www.redhat.com/mailman/listinfo/libvirt-cim


-- 
Kaitlin Rupert
IBM Linux Technology Center
kaitlin at linux.vnet.ibm.com