Re: [PATCH v2 1/7] hyperv: implement domainSetAutostart
On Oct 14, 2020, at 3:24 AM, Pino Toscano <ptoscano(a)redhat.com>
wrote:
On Tuesday, 13 October 2020 07:13:58 CEST Matt Coleman wrote:
> + const char *methodName = NULL, *embeddedParamName = NULL;
> + char enabledValue[] = "2", disabledValue[] = "0";
Not that it is an issue: IMHO using for enabledValue/disabledValue the
same approach as methodName/embeddedParamName, i.e. simple const char*
ponting to static strings, is easier.
I originally tried to declare them as const char*, but it failed to
compile with the following error:
../src/hyperv/hyperv_driver.c: In function ‘hypervDomainSetAutostart’:
../src/hyperv/hyperv_driver.c:2707:56: error: passing argument 3 of
‘hypervSetEmbeddedProperty’ discards ‘const’ qualifier from pointer target
type [-Werror=discarded-qualifiers]
2707 | autostart ? enabledValue : disabledValue) < 0) {
| ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~
In file included from ../src/hyperv/hyperv_driver.c:36:
../src/hyperv/hyperv_wmi.h:150:15: note: expected ‘char *’ but argument
is of type ‘const char *’
150 | char *value);
| ~~~~~~^~~~~
hypervSetEmbeddedProperty() is just a wrapper for virHashUpdateEntry().
Also there is a bit of style mixup:
- methodName/embeddedParamName/etc are NULL by default, set to a value
for V1 or V2 (left NULL otherwise)
- enabledValue/disabledValue are set to the values for V1, and changed
to V2
- other patches in this series (#5 and #6) also do "V1 by default,
change for V2"
IMHO a single style would make things easier, especially in case there
will ever be a V3.
I'll make these consistent. I think it'll be easier to maintain if the
declarations set a NULL or dummy value and then configuration for all
Hyper-V versions happens later in a conditional.
> + if (hypervInvokeMethod(priv, params, NULL) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Could not set autostart"));
Minor question: is there really no way to get the reason (in form of
string) of the failure?
hypervInvokeMethod() only returns the XML response data for successful
requests currently. I’ll look into changing it to always return the XML
response data (if available).
Thanks for the feedback on these patches. I will be submitting v3 of
this patchset, but probably won’t get to it until early next week.
--
Matt