On 10/17/20 10:32 PM, Matt Coleman wrote:
On Oct 15, 2020, at 8:56 AM, Michal Privoznik <mprivozn@xxxxxxxxxx> wrote:
On 10/13/20 7:13 AM, Matt Coleman wrote:
+ if (hypervGetWmiClass(Win32_OperatingSystem, &operatingSystem) < 0 ||
+ !operatingSystem) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Could not get free memory for host %s"),
+ conn->uri->server);
IIUC, hypervGetWmiClass() reports an error (in fact more accurate one) on failure. So this overwrite doesn't look good. Also, there is no point calling free if @operatingSystem is NULL ;-)
Thanks for pointing all of that out.
How about this squashed in?
diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
index 0f6d3cb946..195cb4997a 100644
--- a/src/hyperv/hyperv_driver.c
+++ b/src/hyperv/hyperv_driver.c
@@ -1403,20 +1403,19 @@ hypervNodeGetFreeMemory(virConnectPtr conn)
Win32_OperatingSystem *operatingSystem = NULL;
g_auto(virBuffer) query = { g_string_new(WIN32_OPERATINGSYSTEM_WQL_SELECT), 0 };
- if (hypervGetWmiClass(Win32_OperatingSystem, &operatingSystem) < 0 ||
- !operatingSystem) {
+ if (hypervGetWmiClass(Win32_OperatingSystem, &operatingSystem) < 0)
+ return 0;
+
+ if (!operatingSystem) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Could not get free memory for host %s"),
conn->uri->server);
- goto cleanup;
+ return 0;
}
/* Return free memory in bytes */
res = operatingSystem->data.common->FreePhysicalMemory * 1024;
-
- cleanup:
hypervFreeObject(priv, (hypervObject *) operatingSystem);
-
return res;
}
This is a solid improvement.
The pattern `if (hypervGetWmiClass() < 0 || !foo)` is used in several
other places. Would you like me to fix those other occurrences?
Yes, please and thank you. I've opened hyperv driver code and realized
it doesn't really match patterns used elsewhere. Any refactor that
addresses that is more than welcome.
Michal
