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

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 _______________________________________________ Libvirt-cim mailing list Libvirt-cim(a)redhat.com https://www.redhat.com/mailman/listinfo/libvirt-cim