Re: [PATCH v2 1/7] hyperv: implement domainSetAutostart

[Pino Toscano <ptoscano@xxxxxxxxxx>]

On Tuesday, 13 October 2020 07:13:58 CEST Matt Coleman wrote:
> Co-authored-by: Sri Ramanujam <sramanujam@xxxxxxxxx>
> Signed-off-by: Matt Coleman <matt@xxxxxxxxx>
> ---
>  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";

Not that it is an issue: IMHO using for enabledValue/disabledValue the
same approach as methodName/embeddedParamName, i.e. simple const char*
ponting to static strings, is easier.

Also there is a bit of style mixup:
- methodName/embeddedParamName/etc are NULL by default, set to a value
  for V1 or V2 (left NULL otherwise)
- enabledValue/disabledValue are set to the values for V1, and changed
  to V2
- other patches in this series (#5 and #6) also do "V1 by default,
  change for V2"
IMHO a single style would make things easier, especially in case there
will ever be a V3.

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

Minor question: is there really no way to get the reason (in form of
string) of the failure?

-- 
Pino Toscano

Attachment: signature.asc
Description: This is a digitally signed message part.