[PATCH v2 2/7] hyperv: implement nodeGetFreeMemory

[Michal Prívozník]

On 10/17/20 10:32 PM, Matt Coleman wrote:
>> On Oct 15, 2020, at 8:56 AM, Michal Privoznik <mprivozn at redhat.com> 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