Re: [PATCH v2 1/7] hyperv: implement domainSetAutostart
> On Oct 15, 2020, at 8:57 AM, Michal Privoznik 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
Re: [PATCH v2 6/7] hyperv: fix domainManagedSave on Hyper-V V2
> On Oct 13, 2020, at 1:14 AM, Matt Coleman wrote: > > +if (priv->wmiVersion == HYPERV_WMI_VERSION_V2) > +requestedState = > MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_ENABLED_BUT_OFFLINE; ... > +MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_ENABLED_BUT_OFFLINE = 6, I realized I named this after the description of the (related) Msvm_ComputerSystem EnabledState value, not the RequestStateChange() method’s RequestedState parameter that this enum actually represents. https://docs.microsoft.com/en-us/windows/win32/hyperv_v2/msvm-computersystem#Enabled_but_Offline https://docs.microsoft.com/en-us/windows/win32/hyperv_v2/requeststatechange-msvm-computersystem#Offline If another squashed change is ok, this should be “MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_OFFLINE” instead of “MSVM_COMPUTERSYSTEM_REQUESTEDSTATE_ENABLED_BUT_OFFLINE”. I feel like we’ve discussed a lot of changes getting squashed into this patchset and that I should probably just revise it all and send v3. -- Matt
Re: [PATCH v2 2/7] hyperv: implement nodeGetFreeMemory
> On Oct 15, 2020, at 8:56 AM, Michal Privoznik wrote:
>
> On 10/13/20 7:13 AM, Matt Coleman wrote:
>> +if (hypervGetWmiClass(Win32_OperatingSystem, ) < 0 ||
>> +!operatingSystem) {
>> +virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("Could not get free memory for host %s"),
>> + conn->uri->server);
>
> IIUC, hypervGetWmiClass() reports an error (in fact more accurate one) on
> failure. So this overwrite doesn't look good. Also, there is no point calling
> free if @operatingSystem is NULL ;-)
Thanks for pointing all of that out.
>
> How about this squashed in?
>
> diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
> index 0f6d3cb946..195cb4997a 100644
> --- a/src/hyperv/hyperv_driver.c
> +++ b/src/hyperv/hyperv_driver.c
> @@ -1403,20 +1403,19 @@ hypervNodeGetFreeMemory(virConnectPtr conn)
> Win32_OperatingSystem *operatingSystem = NULL;
> g_auto(virBuffer) query = {
> g_string_new(WIN32_OPERATINGSYSTEM_WQL_SELECT), 0 };
>
> -if (hypervGetWmiClass(Win32_OperatingSystem, ) < 0 ||
> -!operatingSystem) {
> +if (hypervGetWmiClass(Win32_OperatingSystem, ) < 0)
> +return 0;
> +
> +if (!operatingSystem) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
>_("Could not get free memory for host %s"),
>conn->uri->server);
> -goto cleanup;
> +return 0;
> }
>
> /* Return free memory in bytes */
> res = operatingSystem->data.common->FreePhysicalMemory * 1024;
> -
> -cleanup:
> hypervFreeObject(priv, (hypervObject *) operatingSystem);
> -
> return res;
> }
This is a solid improvement.
The pattern `if (hypervGetWmiClass() < 0 || !foo)` is used in several
other places. Would you like me to fix those other occurrences?
--
Matt
