[PATCH v2 1/7] hyperv: implement domainSetAutostart

[Matt Coleman]

> On Oct 14, 2020, at 3:24 AM, Pino Toscano <ptoscano at redhat.com> wrote:
> 
> On Tuesday, 13 October 2020 07:13:58 CEST Matt Coleman wrote:
>> +    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.

I originally tried to declare them as const char*, but it failed to 
compile with the following error:

../src/hyperv/hyperv_driver.c: In function ‘hypervDomainSetAutostart’:
../src/hyperv/hyperv_driver.c:2707:56: error: passing argument 3 of 
‘hypervSetEmbeddedProperty’ discards ‘const’ qualifier from pointer target 
type [-Werror=discarded-qualifiers]
 2707 |                               autostart ? enabledValue : disabledValue) < 0) {
      |                               ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~
In file included from ../src/hyperv/hyperv_driver.c:36:
../src/hyperv/hyperv_wmi.h:150:15: note: expected ‘char *’ but argument 
is of type ‘const char *’
  150 |         char *value);
      |         ~~~~~~^~~~~

hypervSetEmbeddedProperty() is just a wrapper for virHashUpdateEntry().

> 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.

I'll make these consistent. I think it'll be easier to maintain if the 
declarations set a NULL or dummy value and then configuration for all 
Hyper-V versions happens later in a conditional.

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

hypervInvokeMethod() only returns the XML response data for successful 
requests currently. I’ll look into changing it to always return the XML 
response data (if available).

Thanks for the feedback on these patches. I will be submitting v3 of 
this patchset, but probably won’t get to it until early next week.


-- 
Matt