Even though we don't accept any flags, it is unfriendly to callers
that use the modern API to have to fall back to the flag-free API.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
src/vbox/vbox_common.c | 24 ++++++++++++++++++++++--
1 file changed, 22 insertions(+), 2 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index 54e31bec9d..44c98cadf6 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -553,7 +553,8 @@ static int vboxConnectClose(virConnectPtr conn)
}
static int
-vboxDomainSave(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED)
+vboxDomainSaveFlags(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED,
+ const char *dxml, unsigned int flags)
{
vboxDriverPtr data = dom->conn->privateData;
IConsole *console = NULL;
@@ -564,6 +565,9 @@ vboxDomainSave(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED)
nsresult rc;
int ret = -1;
+ virCheckFlags(0, -1);
+ virCheckNonNullArgReturn(dxml, -1);
+
if (!data->vboxObj)
return ret;
@@ -607,6 +611,12 @@ vboxDomainSave(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED)
return ret;
}
+static int
+vboxDomainSave(virDomainPtr dom, const char *path)
+{
+ return vboxDomainSaveFlags(dom, path, NULL, 0);
+}
+
static int vboxConnectGetVersion(virConnectPtr conn, unsigned long *version)
{
vboxDriverPtr data = conn->privateData;
@@ -2717,7 +2727,8 @@ static char *vboxDomainGetOSType(virDomainPtr dom ATTRIBUTE_UNUSED) {
return osType;
}
-static int vboxDomainSetMemory(virDomainPtr dom, unsigned long memory)
+static int vboxDomainSetMemoryFlags(virDomainPtr dom, unsigned long memory,
+ unsigned int flags)
{
vboxDriverPtr data = dom->conn->privateData;
IMachine *machine = NULL;
@@ -2727,6 +2738,8 @@ static int vboxDomainSetMemory(virDomainPtr dom, unsigned long memory)
nsresult rc;
int ret = -1;
+ virCheckFlags(0, -1);
+
if (!data->vboxObj)
return ret;
@@ -2775,6 +2788,11 @@ static int vboxDomainSetMemory(virDomainPtr dom, unsigned long memory)
return ret;
}
+static int vboxDomainSetMemory(virDomainPtr dom, unsigned long memory)
+{
+ return vboxDomainSetMemoryFlags(dom, memory, 0);
+}
+
static int vboxDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info)
{
vboxDriverPtr data = dom->conn->privateData;
@@ -7995,9 +8013,11 @@ static virHypervisorDriver vboxCommonDriver = {
.domainDestroyFlags = vboxDomainDestroyFlags, /* 0.9.4 */
.domainGetOSType = vboxDomainGetOSType, /* 0.6.3 */
.domainSetMemory = vboxDomainSetMemory, /* 0.6.3 */
+ .domainSetMemoryFlags = vboxDomainSetMemoryFlags, /* 5.6.0 */
.domainGetInfo = vboxDomainGetInfo, /* 0.6.3 */
.domainGetState = vboxDomainGetState, /* 0.9.2 */
.domainSave = vboxDomainSave, /* 0.6.3 */
+ .domainSaveFlags = vboxDomainSaveFlags, /* 5.6.0 */
.domainSetVcpus = vboxDomainSetVcpus, /* 0.7.1 */
.domainSetVcpusFlags = vboxDomainSetVcpusFlags, /* 0.8.5 */
.domainGetVcpusFlags = vboxDomainGetVcpusFlags, /* 0.8.5 */
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Jul 09, 2019 at 12:46:30 -0500, Eric Blake wrote:
> Even though we don't accept any flags, it is unfriendly to callers
> that use the modern API to have to fall back to the flag-free API.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> src/vbox/vbox_common.c | 24 ++++++++++++++++++++++--
> 1 file changed, 22 insertions(+), 2 deletions(-)
[...]
> @@ -2775,6 +2788,11 @@ static int vboxDomainSetMemory(virDomainPtr dom, unsigned long memory)
> return ret;
> }
>
> +static int vboxDomainSetMemory(virDomainPtr dom, unsigned long memory)
> +{
> + return vboxDomainSetMemoryFlags(dom, memory, 0);
The old API was operating on a live VM only so this shim should imply
VIR_DOMAIN_AFFECT_LIVE.
> +}
> +
> static int vboxDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info)
> {
> vboxDriverPtr data = dom->conn->privateData;
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 7/10/19 2:09 AM, Peter Krempa wrote:
> On Tue, Jul 09, 2019 at 12:46:30 -0500, Eric Blake wrote:
>> Even though we don't accept any flags, it is unfriendly to callers
>> that use the modern API to have to fall back to the flag-free API.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>> src/vbox/vbox_common.c | 24 ++++++++++++++++++++++--
>> 1 file changed, 22 insertions(+), 2 deletions(-)
>
> [...]
>
>> @@ -2775,6 +2788,11 @@ static int vboxDomainSetMemory(virDomainPtr dom, unsigned long memory)
>> return ret;
>> }
>>
>> +static int vboxDomainSetMemory(virDomainPtr dom, unsigned long memory)
>> +{
>> + return vboxDomainSetMemoryFlags(dom, memory, 0);
>
> The old API was operating on a live VM only so this shim should imply
> VIR_DOMAIN_AFFECT_LIVE.
Hmm. Commit 667ac11e used flag 0 for the test driver. But if I want to
reduce the number of driver callbacks by instead teaching the remote
driver when to call the legacy RPC, it's easier if ALL drivers shim-call
SetMemoryFlags(VIR_DOMAIN_AFFECT_LIVE) rather than some calling with 0
and others calling with AFFECT_LIVE.
The current list:
v5.5.0:src/hyperv/hyperv_driver.c: return
hypervDomainSetMemoryFlags(domain, memory, 0);
v5.5.0:src/libxl/libxl_driver.c: return
libxlDomainSetMemoryFlags(dom, memory, VIR_DOMAIN_MEM_LIVE);
v5.5.0:src/libxl/libxl_driver.c: return
libxlDomainSetMemoryFlags(dom, memory, VIR_DOMAIN_MEM_MAXIMUM);
v5.5.0:src/lxc/lxc_driver.c: return lxcDomainSetMemoryFlags(dom,
newmem, VIR_DOMAIN_AFFECT_LIVE);
v5.5.0:src/lxc/lxc_driver.c: return lxcDomainSetMemoryFlags(dom,
newmax, VIR_DOMAIN_MEM_MAXIMUM);
v5.5.0:src/qemu/qemu_driver.c: return qemuDomainSetMemoryFlags(dom,
newmem, VIR_DOMAIN_AFFECT_LIVE);
v5.5.0:src/qemu/qemu_driver.c: return qemuDomainSetMemoryFlags(dom,
memory, VIR_DOMAIN_MEM_MAXIMUM);
my series adds:
test
vbox
xenapi
So it looks like hyperv has a bug in being inconsistent from everyone
else, if I follow your advice for the 3 new shims, although it currently
lacks a SetMaxMemory API at all.
You also appear to be right that making SetMaxMemory a shim everywhere
for SetMemoryFlags(VIR_DOMAIN_MEM_MAXMIUM) is a sensible idea, at least
for drivers that have a notion of setting max memory, but since not all
drivers had SetMaxMemory, it's harder to argue that the presence of the
new interface must require the legacy pair (especially if the new
interface blindly requires a specific flags pattern). I'll have to look
more closely at what each driver is doing.
Our public docs currently state:
* and will fail for transient domains. If neither flag is specified
* (that is, @flags is VIR_DOMAIN_AFFECT_CURRENT), then an inactive domain
* modifies persistent setup, while an active domain is hypervisor-dependent
* on whether just live or both live and persistent state is changed.
which means that calling SetMemory() may be a synonym to either
SetMemoryFlags(_CURRENT==0) or to SetMemoryFlags(_LIVE). But by
tweaking hyperv, we could change it so that the spec is more
tightly-worded as always being equivalent to SetMemoryFlags(_LIVE).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Jul 09, 2019 at 12:46:30 -0500, Eric Blake wrote:
> Even though we don't accept any flags, it is unfriendly to callers
> that use the modern API to have to fall back to the flag-free API.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> src/vbox/vbox_common.c | 24 ++++++++++++++++++++++--
> 1 file changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
> index 54e31bec9d..44c98cadf6 100644
> --- a/src/vbox/vbox_common.c
> +++ b/src/vbox/vbox_common.c
> @@ -553,7 +553,8 @@ static int vboxConnectClose(virConnectPtr conn)
> }
>
> static int
> -vboxDomainSave(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED)
> +vboxDomainSaveFlags(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED,
> + const char *dxml, unsigned int flags)
> {
> vboxDriverPtr data = dom->conn->privateData;
> IConsole *console = NULL;
> @@ -564,6 +565,9 @@ vboxDomainSave(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED)
> nsresult rc;
> int ret = -1;
>
> + virCheckFlags(0, -1);
> + virCheckNonNullArgReturn(dxml, -1);
This reports: invalid argument: dxml in vboxDomainSave must not be NULL
I'm not certain that the internal function name makes sense to external
users.
> +
> if (!data->vboxObj)
> return ret;
>
> @@ -607,6 +611,12 @@ vboxDomainSave(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED)
> return ret;
> }
>
> +static int
> +vboxDomainSave(virDomainPtr dom, const char *path)
> +{
> + return vboxDomainSaveFlags(dom, path, NULL, 0);
So, this passes NULL 'dxml' into vboxDomainSaveFlags which explicitly
rejects it. What's the point?
Ah I see. Was the above supposed to be virCheckNullArgGoto?
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 7/10/19 2:02 AM, Peter Krempa wrote:
> On Tue, Jul 09, 2019 at 12:46:30 -0500, Eric Blake wrote:
>> Even though we don't accept any flags, it is unfriendly to callers
>> that use the modern API to have to fall back to the flag-free API.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>> src/vbox/vbox_common.c | 24 ++++++++++++++++++++++--
>> 1 file changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
>> index 54e31bec9d..44c98cadf6 100644
>> --- a/src/vbox/vbox_common.c
>> +++ b/src/vbox/vbox_common.c
>> @@ -553,7 +553,8 @@ static int vboxConnectClose(virConnectPtr conn)
>> }
>>
>> static int
>> -vboxDomainSave(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED)
>> +vboxDomainSaveFlags(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED,
>> + const char *dxml, unsigned int flags)
>> {
>> vboxDriverPtr data = dom->conn->privateData;
>> IConsole *console = NULL;
>> @@ -564,6 +565,9 @@ vboxDomainSave(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED)
>> nsresult rc;
>> int ret = -1;
>>
>> + virCheckFlags(0, -1);
>> + virCheckNonNullArgReturn(dxml, -1);
>
> This reports: invalid argument: dxml in vboxDomainSave must not be NULL
>
> I'm not certain that the internal function name makes sense to external
> users.
In which case I can hand-roll a more specific error instead of reusing
the common macro. But I do see pre-existing uses of
virCheckNonNullArgXXX in src/esx and src/vz prior to this patch, so it's
not the first time we've used the common macros.
>
>> +
>> if (!data->vboxObj)
>> return ret;
>>
>> @@ -607,6 +611,12 @@ vboxDomainSave(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED)
>> return ret;
>> }
>>
>> +static int
>> +vboxDomainSave(virDomainPtr dom, const char *path)
>> +{
>> + return vboxDomainSaveFlags(dom, path, NULL, 0);
>
> So, this passes NULL 'dxml' into vboxDomainSaveFlags which explicitly
> rejects it. What's the point?
>
> Ah I see. Was the above supposed to be virCheckNullArgGoto?
D'oh. Yes, I got it backwards. The function wants to succeed only if
the user omitted the optional dxml argument.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 7/10/19 6:50 AM, Eric Blake wrote:
> On 7/10/19 2:02 AM, Peter Krempa wrote:
>> On Tue, Jul 09, 2019 at 12:46:30 -0500, Eric Blake wrote:
>>> Even though we don't accept any flags, it is unfriendly to callers
>>> that use the modern API to have to fall back to the flag-free API.
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>> ---
>>> -vboxDomainSave(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED)
>>> +vboxDomainSaveFlags(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED,
>>> + const char *dxml, unsigned int flags)
>>> {
>>> vboxDriverPtr data = dom->conn->privateData;
>>> IConsole *console = NULL;
>>> @@ -564,6 +565,9 @@ vboxDomainSave(virDomainPtr dom, const char *path ATTRIBUTE_UNUSED)
>>> nsresult rc;
>>> int ret = -1;
>>>
>>> + virCheckFlags(0, -1);
>>> + virCheckNonNullArgReturn(dxml, -1);
>>
>> This reports: invalid argument: dxml in vboxDomainSave must not be NULL
>>
Revisiting this thread:
internal.h has:
virCheckNullArgGoto (complains if argument is not NULL)
virCheckNonNullArgReturn (complains if argument is NULL)
virCheckNonNullArgGoto (complains if argument is NULL)
but is missing virCheckNullArgReturn, which is the form I really meant
to use here. I have no idea if that missing macro is intentional, but it
would be easy enough to add.
>> I'm not certain that the internal function name makes sense to external
>> users.
>
> In which case I can hand-roll a more specific error instead of reusing
> the common macro. But I do see pre-existing uses of
> virCheckNonNullArgXXX in src/esx and src/vz prior to this patch, so it's
> not the first time we've used the common macros.
Directly calling virReportInvalidNonNullArg() would be the only way to
hand-roll this properly, but no one outside of internal.h and
src/util/virobject.c uses it. I'd prefer to sick with
virCheckNullArgReturn().
>>> +static int
>>> +vboxDomainSave(virDomainPtr dom, const char *path)
>>> +{
>>> + return vboxDomainSaveFlags(dom, path, NULL, 0);
>>
>> So, this passes NULL 'dxml' into vboxDomainSaveFlags which explicitly
>> rejects it. What's the point?
>>
>> Ah I see. Was the above supposed to be virCheckNullArgGoto?
>
> D'oh. Yes, I got it backwards. The function wants to succeed only if
> the user omitted the optional dxml argument.
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.