[PATCH v2 1/7] hyperv: implement domainSetAutostart

[Michal Prívozník]

On 10/17/20 10:39 PM, Matt Coleman wrote:
>> On Oct 15, 2020, at 8:57 AM, Michal Privoznik <mprivozn at redhat.com> wrote:
>>
>> On 10/13/20 7:13 AM, Matt Coleman wrote:
>>> +    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.
> 
> Pino only suggested using a uniform approach but didn’t recommend a
> specific solution. Is the single if statement how you’d prefer to see it
> done? I think it would be clearer if it were more verbose: initially
> declare the variables either as NULL or with a dummy value, then set
> all of the variables’ values in conditional blocks for each supported
> Hyper-V version.

I don't have a strong preference. But it seems a bit weird to init only 
some variables when declaring them to desired value and then have if() 
to init the rest. I understand why you needed to do it though => so that 
@enabledValue and @disabledValue are allocated and you could overwrite 
them in VERSION_V2 branch. Speaking of which, let me see if I can do 
something about that.

As for you original question, how about initializing all variables to 
NULL and replacing if (version == _V1) { } else if (version == _V2) {} 
with a switch statement? My worry is that if we ever introduce _V3 these 
if-else statements won't catch it, with switch the compiler will notify 
us about missing case.


> 
>>> + 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.
> 
> Thanks for that patch - makes it much clearer.
> 
>> Anyway, this is what I suggest is squashed in (I can squash it before pushing if you agree):
> 
> That sounds good to me unless you liked my earlier idea to be more
> verbose about the conditionals. If you do, then I think I should just
> revise the whole patchset.
> 
> If you prefer the shorter conditionals, then it looks like we could go
> forward with 1, 2, 5, and 6 (if you can squash my requested change into
> it - see that thread) for now. There are some small changes I’d like to
> make to patches 3 and 4 in this series based on Pino’s comments.
> 
> Based on the number of proposed squashed changes, the fact that I want
> to make changes to two commits in the middle of this set, and since
> slight changes will need to be made in order to work with your
> ownership transfer patchset, it seems to me like I should revise it all
> and send v3. Do you agree?

Right, at this point I agree that v3 is more clearer.

Michal