Re: [PATCH v2 1/7] hyperv: implement domainSetAutostart
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.
> + 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