Re: [PATCH v2 2/7] hyperv: implement nodeGetFreeMemory

> On Oct 15, 2020, at 8:56 AM, Michal Privoznik <mprivozn(a)redhat.com&gt; 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?