> 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?
--
Matt
