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

[Jim Fehlig]

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,
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.

> 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