Re: [PATCH v2 1/7] hyperv: implement domainSetAutostart
On 10/17/20 10:39 PM, Matt Coleman wrote:
> On Oct 15, 2020, at 8:57 AM, Michal Privoznik
<mprivozn(a)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