[PATCH v2 1/7] hyperv: implement domainSetAutostart

[Matt Coleman]

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

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

Thanks for all the feedback!

-- 
Matt