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

[Michal Privoznik <mprivozn@xxxxxxxxxx>]

On 10/13/20 7:13 AM, Matt Coleman wrote:
> Co-authored-by: Sri Ramanujam <sramanujam@xxxxxxxxx>
> Signed-off-by: Matt Coleman <matt@xxxxxxxxx>
> ---
>   src/hyperv/hyperv_driver.c | 28 ++++++++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
>
> diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
> index 79b48a9dff..c4fca4685e 100644
> --- a/src/hyperv/hyperv_driver.c
> +++ b/src/hyperv/hyperv_driver.c
> @@ -1400,6 +1400,33 @@ hypervDomainSetAutostart(virDomainPtr domain, int autostart)
>   
>   
>   
> +static unsigned long long
> +hypervNodeGetFreeMemory(virConnectPtr conn)
> +{
> +    unsigned long long res = 0;
> +    hypervPrivate *priv = conn->privateData;
> +    Win32_OperatingSystem *operatingSystem = NULL;
> +    g_auto(virBuffer) query = { g_string_new(WIN32_OPERATINGSYSTEM_WQL_SELECT), 0 };
> +
> +    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 ;-)

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;
 }



Michal