[PATCH v2 1/7] hyperv: implement domainSetAutostart

[Michal Privoznik]

On 10/13/20 7:13 AM, Matt Coleman wrote:
> Co-authored-by: Sri Ramanujam <sramanujam at datto.com>
> Signed-off-by: Matt Coleman <matt at datto.com>
> ---
>   src/hyperv/hyperv_driver.c | 91 ++++++++++++++++++++++++++++++++++++++
>   1 file changed, 91 insertions(+)
> 
> diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
> index 2ac30fa4c6..79b48a9dff 100644
> --- a/src/hyperv/hyperv_driver.c
> +++ b/src/hyperv/hyperv_driver.c
> @@ -1310,6 +1310,96 @@ hypervDomainGetAutostart(virDomainPtr domain, int *autostart)
>   
>   
>   
> +static int
> +hypervDomainSetAutostart(virDomainPtr domain, int autostart)
> +{
> +    int result = -1;
> +    char uuid_string[VIR_UUID_STRING_BUFLEN];
> +    hypervPrivate *priv = domain->conn->privateData;
> +    Msvm_VirtualSystemSettingData *vssd = NULL;
> +    hypervInvokeParamsListPtr params = NULL;
> +    g_auto(virBuffer) eprQuery = VIR_BUFFER_INITIALIZER;
> +    virHashTablePtr autostartParam = NULL;
> +    hypervWmiClassInfoListPtr embeddedParamClass = NULL;
> +    const char *methodName = NULL, *embeddedParamName = NULL;
> +    char enabledValue[] = "2", disabledValue[] = "0";
> +
> +    if (priv->wmiVersion == HYPERV_WMI_VERSION_V1) {
> +        methodName = "ModifyVirtualSystem";
> +        embeddedParamName = "SystemSettingData";
> +        embeddedParamClass = Msvm_VirtualSystemGlobalSettingData_WmiInfo;
> +    } else if (priv->wmiVersion == HYPERV_WMI_VERSION_V2) {
> +        methodName = "ModifySystemSettings";
> +        embeddedParamName = "SystemSettings";
> +        embeddedParamClass = Msvm_VirtualSystemSettingData_WmiInfo;
> +        enabledValue[0] = '4';
> +        disabledValue[0] = '2';
> +    }

As pino pointed out, this can be just one if.

> +
> +    virUUIDFormat(domain->uuid, uuid_string);
> +
> +    if (hypervGetVSSDFromUUID(priv, uuid_string, &vssd) < 0)
> +        goto cleanup;
> +
> +    params = hypervCreateInvokeParamsList(priv, methodName,
> +                                MSVM_VIRTUALSYSTEMMANAGEMENTSERVICE_SELECTOR,
> +                                Msvm_VirtualSystemManagementService_WmiInfo);
> +
> +    if (!params) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Could not create params"));
> +        goto cleanup;
> +    }
> +
> +    if (priv->wmiVersion == HYPERV_WMI_VERSION_V1) {
> +        virBufferEscapeSQL(&eprQuery,
> +                           MSVM_COMPUTERSYSTEM_WQL_SELECT "WHERE Name = '%s'",
> +                           uuid_string);
> +
> +        if (hypervAddEprParam(params, "ComputerSystem", priv, &eprQuery,
> +                              Msvm_ComputerSystem_WmiInfo) < 0)
> +            goto params_cleanup;
> +    }
> +
> +    autostartParam = hypervCreateEmbeddedParam(priv, embeddedParamClass);
> +
> +    if (hypervSetEmbeddedProperty(autostartParam, "AutomaticStartupAction",
> +                              autostart ? enabledValue : disabledValue) < 0) {
> +        hypervFreeEmbeddedParam(autostartParam);
> +        goto params_cleanup;
> +    }
> +
> +    if (hypervSetEmbeddedProperty(autostartParam, "InstanceID",
> +                                  vssd->data.common->InstanceID) < 0) {
> +        hypervFreeEmbeddedParam(autostartParam);
> +        goto params_cleanup;
> +    }
> +
> +    if (hypervAddEmbeddedParam(params, priv, embeddedParamName, autostartParam,
> +                               embeddedParamClass) < 0) {
> +        hypervFreeEmbeddedParam(autostartParam);
> +        goto params_cleanup;
> +    }
> +
> +    if (hypervInvokeMethod(priv, params, NULL) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("Could not set autostart"));
> +        goto cleanup;
> +    }
> +
> +    result = 0;
> +    goto cleanup;
> +
> + params_cleanup:
> +    hypervFreeInvokeParams(params);

So the only reason for the separate lable is that hypervInvokeMethod() 
consumes @params and thus we must avoid jumping here once it was called. 
Fortunately, hypervFreeInvokeParams() accepts NULL so what we can do is 
to set params = NULL in both success and fail paths.

I'll post a patch that makes hypervInvokeMethod() behave more sanely.

Anyway, this is what I suggest is squashed in (I can squash it before 
pushing if you agree):


diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
index 79b48a9dff..1eae7c38e2 100644
--- a/src/hyperv/hyperv_driver.c
+++ b/src/hyperv/hyperv_driver.c
@@ -1309,7 +1309,6 @@ hypervDomainGetAutostart(virDomainPtr domain, int 
*autostart)
  }


-
  static int
  hypervDomainSetAutostart(virDomainPtr domain, int autostart)
  {
@@ -1320,15 +1319,12 @@ hypervDomainSetAutostart(virDomainPtr domain, 
int autostart)
      hypervInvokeParamsListPtr params = NULL;
      g_auto(virBuffer) eprQuery = VIR_BUFFER_INITIALIZER;
      virHashTablePtr autostartParam = NULL;
-    hypervWmiClassInfoListPtr embeddedParamClass = NULL;
-    const char *methodName = NULL, *embeddedParamName = NULL;
+    hypervWmiClassInfoListPtr embeddedParamClass = 
Msvm_VirtualSystemGlobalSettingData_WmiInfo;
+    const char *methodName = "ModifyVirtualSystem";
+    const char *embeddedParamName = "SystemSettingData";
      char enabledValue[] = "2", disabledValue[] = "0";

-    if (priv->wmiVersion == HYPERV_WMI_VERSION_V1) {
-        methodName = "ModifyVirtualSystem";
-        embeddedParamName = "SystemSettingData";
-        embeddedParamClass = Msvm_VirtualSystemGlobalSettingData_WmiInfo;
-    } else if (priv->wmiVersion == HYPERV_WMI_VERSION_V2) {
+    if (priv->wmiVersion == HYPERV_WMI_VERSION_V2) {
          methodName = "ModifySystemSettings";
          embeddedParamName = "SystemSettings";
          embeddedParamClass = Msvm_VirtualSystemSettingData_WmiInfo;
@@ -1342,8 +1338,8 @@ hypervDomainSetAutostart(virDomainPtr domain, int 
autostart)
          goto cleanup;

      params = hypervCreateInvokeParamsList(priv, methodName,
- 
MSVM_VIRTUALSYSTEMMANAGEMENTSERVICE_SELECTOR,
- 
Msvm_VirtualSystemManagementService_WmiInfo);
+ 
MSVM_VIRTUALSYSTEMMANAGEMENTSERVICE_SELECTOR,
+ 
Msvm_VirtualSystemManagementService_WmiInfo);

      if (!params) {
          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -1358,48 +1354,47 @@ hypervDomainSetAutostart(virDomainPtr domain, 
int autostart)

          if (hypervAddEprParam(params, "ComputerSystem", priv, &eprQuery,
                                Msvm_ComputerSystem_WmiInfo) < 0)
-            goto params_cleanup;
+            goto cleanup;
      }

      autostartParam = hypervCreateEmbeddedParam(priv, embeddedParamClass);

      if (hypervSetEmbeddedProperty(autostartParam, 
"AutomaticStartupAction",
-                              autostart ? enabledValue : disabledValue) 
< 0) {
+                                  autostart ? enabledValue : 
disabledValue) < 0) {
          hypervFreeEmbeddedParam(autostartParam);
-        goto params_cleanup;
+        goto cleanup;
      }

      if (hypervSetEmbeddedProperty(autostartParam, "InstanceID",
                                    vssd->data.common->InstanceID) < 0) {
          hypervFreeEmbeddedParam(autostartParam);
-        goto params_cleanup;
+        goto cleanup;
      }

      if (hypervAddEmbeddedParam(params, priv, embeddedParamName, 
autostartParam,
                                 embeddedParamClass) < 0) {
          hypervFreeEmbeddedParam(autostartParam);
-        goto params_cleanup;
+        goto cleanup;
      }

      if (hypervInvokeMethod(priv, params, NULL) < 0) {
+        params = NULL;
          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                         _("Could not set autostart"));
          goto cleanup;
      }

+    params = NULL;
      result = 0;
-    goto cleanup;

- params_cleanup:
-    hypervFreeInvokeParams(params);
   cleanup:
+    hypervFreeInvokeParams(params);
      hypervFreeObject(priv, (hypervObject *) vssd);

      return result;
  }


-
  static int
  hypervConnectIsEncrypted(virConnectPtr conn)
  {




Michal