[libvirt] [PATCH 0/3] RFC: setvcpus: support --current option
Hi all, This patchset adds the --current option to "virsh setvcpus" command. Currently "virsh setvcpus" command supports "--live" and "--config" , but "--current" option.
From view of consistency, it's reasonable to support "--current" option too.
*[PATCH 1/3] setvcpus: extend virDomainSetVcpusFlags API to support current flag *[PATCH 2/3] setvcpus: extend qemuDomainSetVcpusFlags() to support current flag *[PATCH 3/3] setvcpus: add "--current" option to "virsh setvcpus" Best regards, Taku Izumi <izumi.taku@jp.fujitsu.com>
This patch extends virDomainSetVcpusFlags API to support VIR_DOMAIN_AFFECT_CURRENT flag. Now because most APIs accept VIR_DOMAIN_AFFECT_CURRENT flags, virDomainSetVcpusFlags API should also do. Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com> --- include/libvirt/libvirt.h.in | 4 ++-- src/libvirt.c | 10 +++++++--- 2 files changed, 9 insertions(+), 5 deletions(-) Index: libvirt/src/libvirt.c =================================================================== --- libvirt.orig/src/libvirt.c +++ libvirt/src/libvirt.c @@ -6863,10 +6863,14 @@ error: * does not support it or if growing the number is arbitrary limited. * This function requires privileged access to the hypervisor. * - * @flags must include VIR_DOMAIN_AFFECT_LIVE to affect a running + * @flags may include VIR_DOMAIN_AFFECT_LIVE to affect a running * domain (which may fail if domain is not active), or * VIR_DOMAIN_AFFECT_CONFIG to affect the next boot via the XML * description of the domain. Both flags may be set. + * 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. * * If @flags includes VIR_DOMAIN_VCPU_MAXIMUM, then * VIR_DOMAIN_AFFECT_LIVE must be clear, and only the maximum virtual @@ -6874,6 +6878,7 @@ error: * equal to virConnectGetMaxVcpus(). Otherwise, this call affects the * current virtual CPU limit, which must be less than or equal to the * maximum limit. + * Not all hypervisors can support all flag combinations. * * Returns 0 in case of success, -1 in case of failure. */ @@ -6899,8 +6904,7 @@ virDomainSetVcpusFlags(virDomainPtr doma } /* Perform some argument validation common to all implementations. */ - if (nvcpus < 1 || (unsigned short) nvcpus != nvcpus || - (flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) == 0) { + if (nvcpus < 1 || (unsigned short) nvcpus != nvcpus) { virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); goto error; } Index: libvirt/include/libvirt/libvirt.h.in =================================================================== --- libvirt.orig/include/libvirt/libvirt.h.in +++ libvirt/include/libvirt/libvirt.h.in @@ -1234,8 +1234,8 @@ typedef virVcpuInfo *virVcpuInfoPtr; /* Flags for controlling virtual CPU hot-plugging. */ typedef enum { - /* Must choose at least one of these two bits; SetVcpus can choose both; - see virDomainModificationImpact for details. */ + /* See virDomainModificationImpact for these flags. */ + VIR_DOMAIN_VCPU_CURRENT = VIR_DOMAIN_AFFECT_CURRENT, VIR_DOMAIN_VCPU_LIVE = VIR_DOMAIN_AFFECT_LIVE, VIR_DOMAIN_VCPU_CONFIG = VIR_DOMAIN_AFFECT_CONFIG,
On 07/15/2011 01:00 AM, Taku Izumi wrote:
This patch extends virDomainSetVcpusFlags API to support VIR_DOMAIN_AFFECT_CURRENT flag.
Now because most APIs accept VIR_DOMAIN_AFFECT_CURRENT flags, virDomainSetVcpusFlags API should also do.
Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com> --- include/libvirt/libvirt.h.in | 4 ++-- src/libvirt.c | 10 +++++++--- 2 files changed, 9 insertions(+), 5 deletions(-)
Thanks for the consistency efforts. ACK and pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
This patch extends qemudDomainSetVcpusFlags() function to support VIR_DOMAIN_AFFECT_CURRENT flag. Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com> --- src/qemu/qemu_driver.c | 37 +++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-) Index: libvirt/src/qemu/qemu_driver.c =================================================================== --- libvirt.orig/src/qemu/qemu_driver.c +++ libvirt/src/qemu/qemu_driver.c @@ -2915,20 +2915,12 @@ qemudDomainSetVcpusFlags(virDomainPtr do const char * type; int max; int ret = -1; + bool isActive; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG | VIR_DOMAIN_VCPU_MAXIMUM, -1); - /* At least one of LIVE or CONFIG must be set. MAXIMUM cannot be - * mixed with LIVE. */ - if ((flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) == 0 || - (flags & (VIR_DOMAIN_VCPU_MAXIMUM | VIR_DOMAIN_AFFECT_LIVE)) == - (VIR_DOMAIN_VCPU_MAXIMUM | VIR_DOMAIN_AFFECT_LIVE)) { - qemuReportError(VIR_ERR_INVALID_ARG, - _("invalid flag combination: (0x%x)"), flags); - return -1; - } if (!nvcpus || (unsigned short) nvcpus != nvcpus) { qemuReportError(VIR_ERR_INVALID_ARG, _("argument out of range: %d"), nvcpus); @@ -2950,7 +2942,32 @@ qemudDomainSetVcpusFlags(virDomainPtr do if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; - if (!virDomainObjIsActive(vm) && (flags & VIR_DOMAIN_AFFECT_LIVE)) { + isActive = virDomainObjIsActive(vm); + + if (flags == VIR_DOMAIN_AFFECT_CURRENT) { + if (isActive) + flags = VIR_DOMAIN_AFFECT_LIVE; + else + flags = VIR_DOMAIN_AFFECT_CONFIG; + } + if (flags == VIR_DOMAIN_VCPU_MAXIMUM) { + if (isActive) + flags |= VIR_DOMAIN_AFFECT_LIVE; + else + flags |= VIR_DOMAIN_AFFECT_CONFIG; + } + + /* At least one of LIVE or CONFIG must be set. MAXIMUM cannot be + * mixed with LIVE. */ + if ((flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) == 0 || + (flags & (VIR_DOMAIN_VCPU_MAXIMUM | VIR_DOMAIN_AFFECT_LIVE)) == + (VIR_DOMAIN_VCPU_MAXIMUM | VIR_DOMAIN_AFFECT_LIVE)) { + qemuReportError(VIR_ERR_INVALID_ARG, + _("invalid flag combination: (0x%x)"), flags); + goto endjob; + } + + if (!isActive && (flags & VIR_DOMAIN_AFFECT_LIVE)) { qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain is not running")); goto endjob;
On 07/15/2011 01:01 AM, Taku Izumi wrote:
This patch extends qemudDomainSetVcpusFlags() function to support VIR_DOMAIN_AFFECT_CURRENT flag.
We've been renaming qemudDomain to qemuDomain lately, so now is as good a time to make the change for this function as any. Especially since you used that shorter name in your subject line :)
Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com> --- src/qemu/qemu_driver.c | 37 +++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-)
@@ -2950,7 +2942,32 @@ qemudDomainSetVcpusFlags(virDomainPtr do if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup;
- if (!virDomainObjIsActive(vm) && (flags & VIR_DOMAIN_AFFECT_LIVE)) { + isActive = virDomainObjIsActive(vm); + + if (flags == VIR_DOMAIN_AFFECT_CURRENT) { + if (isActive) + flags = VIR_DOMAIN_AFFECT_LIVE; + else + flags = VIR_DOMAIN_AFFECT_CONFIG; + } + if (flags == VIR_DOMAIN_VCPU_MAXIMUM) { + if (isActive) + flags |= VIR_DOMAIN_AFFECT_LIVE; + else + flags |= VIR_DOMAIN_AFFECT_CONFIG; + }
That's a bit redundant. How about: if ((flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) == 0) and a single set of flags |=. Or even better, peel off VIR_DOMAIN_VCPU_MAXIMUM into a helper bool before doing anything else.
+ + /* At least one of LIVE or CONFIG must be set. MAXIMUM cannot be + * mixed with LIVE. */ + if ((flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) == 0 ||
This condition is now guaranteed to be false, given the earlier manipulations.
+ (flags & (VIR_DOMAIN_VCPU_MAXIMUM | VIR_DOMAIN_AFFECT_LIVE)) == + (VIR_DOMAIN_VCPU_MAXIMUM | VIR_DOMAIN_AFFECT_LIVE)) { + qemuReportError(VIR_ERR_INVALID_ARG, + _("invalid flag combination: (0x%x)"), flags);
This error message is inaccurate - we've possibly modified flags, so it might not match what the user passed in. Rather, we should just state the restriction.
+ goto endjob; + } + + if (!isActive && (flags & VIR_DOMAIN_AFFECT_LIVE)) {
ACK with nits fixed, so I'm squashing this in before pushing: diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index 0b1d5ec..f3a495f 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -2906,8 +2906,8 @@ unsupported: static int -qemudDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, - unsigned int flags) +qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, + unsigned int flags) { struct qemud_driver *driver = dom->conn->privateData; virDomainObjPtr vm; @@ -2916,6 +2916,7 @@ qemudDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, int max; int ret = -1; bool isActive; + bool maximum; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG | @@ -2943,27 +2944,20 @@ qemudDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, goto cleanup; isActive = virDomainObjIsActive(vm); + maximum = (flags & VIR_DOMAIN_VCPU_MAXIMUM) != 0; + flags &= ~VIR_DOMAIN_VCPU_MAXIMUM; if (flags == VIR_DOMAIN_AFFECT_CURRENT) { if (isActive) - flags = VIR_DOMAIN_AFFECT_LIVE; - else - flags = VIR_DOMAIN_AFFECT_CONFIG; - } - if (flags == VIR_DOMAIN_VCPU_MAXIMUM) { - if (isActive) flags |= VIR_DOMAIN_AFFECT_LIVE; else flags |= VIR_DOMAIN_AFFECT_CONFIG; } - /* At least one of LIVE or CONFIG must be set. MAXIMUM cannot be - * mixed with LIVE. */ - if ((flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) == 0 || - (flags & (VIR_DOMAIN_VCPU_MAXIMUM | VIR_DOMAIN_AFFECT_LIVE)) == - (VIR_DOMAIN_VCPU_MAXIMUM | VIR_DOMAIN_AFFECT_LIVE)) { - qemuReportError(VIR_ERR_INVALID_ARG, - _("invalid flag combination: (0x%x)"), flags); + /* MAXIMUM cannot be mixed with LIVE. */ + if (maximum && (flags & VIR_DOMAIN_AFFECT_LIVE)) { + qemuReportError(VIR_ERR_INVALID_ARG, "%s", + _("cannot adjust maximum on running domain")); goto endjob; } @@ -2992,7 +2986,7 @@ qemudDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, goto endjob; } - if (!(flags & VIR_DOMAIN_VCPU_MAXIMUM) && vm->def->maxvcpus < max) { + if (!maximum && vm->def->maxvcpus < max) { max = vm->def->maxvcpus; } @@ -3007,15 +3001,14 @@ qemudDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, goto endjob; switch (flags) { - case VIR_DOMAIN_VCPU_MAXIMUM | VIR_DOMAIN_AFFECT_CONFIG: - persistentDef->maxvcpus = nvcpus; - if (nvcpus < persistentDef->vcpus) - persistentDef->vcpus = nvcpus; - ret = 0; - break; - case VIR_DOMAIN_AFFECT_CONFIG: - persistentDef->vcpus = nvcpus; + if (maximum) { + persistentDef->maxvcpus = nvcpus; + if (nvcpus < persistentDef->vcpus) + persistentDef->vcpus = nvcpus; + } else { + persistentDef->vcpus = nvcpus; + } ret = 0; break; @@ -3046,9 +3039,9 @@ cleanup: } static int -qemudDomainSetVcpus(virDomainPtr dom, unsigned int nvcpus) +qemuDomainSetVcpus(virDomainPtr dom, unsigned int nvcpus) { - return qemudDomainSetVcpusFlags(dom, nvcpus, VIR_DOMAIN_AFFECT_LIVE); + return qemuDomainSetVcpusFlags(dom, nvcpus, VIR_DOMAIN_AFFECT_LIVE); } @@ -8597,8 +8590,8 @@ static virDriver qemuDriver = { .domainRestore = qemuDomainRestore, /* 0.2.0 */ .domainCoreDump = qemudDomainCoreDump, /* 0.7.0 */ .domainScreenshot = qemuDomainScreenshot, /* 0.9.2 */ - .domainSetVcpus = qemudDomainSetVcpus, /* 0.4.4 */ - .domainSetVcpusFlags = qemudDomainSetVcpusFlags, /* 0.8.5 */ + .domainSetVcpus = qemuDomainSetVcpus, /* 0.4.4 */ + .domainSetVcpusFlags = qemuDomainSetVcpusFlags, /* 0.8.5 */ .domainGetVcpusFlags = qemudDomainGetVcpusFlags, /* 0.8.5 */ .domainPinVcpu = qemudDomainPinVcpu, /* 0.4.4 */ .domainPinVcpuFlags = qemudDomainPinVcpuFlags, /* 0.9.3 */ -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
This patch adds the --current option to "virsh setvcpus" command. Currently "virsh setvcpus" command supports "--live" and "--config" , but "--current" option.
From view of consistency, it's reasonable to support "--current" option too.
When --current is specified, it affects a "current" domain. Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com> --- tools/virsh.c | 26 ++++++++++++++++++++++---- tools/virsh.pod | 6 ++++-- 2 files changed, 26 insertions(+), 6 deletions(-) Index: libvirt/tools/virsh.c =================================================================== --- libvirt.orig/tools/virsh.c +++ libvirt/tools/virsh.c @@ -3257,6 +3257,7 @@ static const vshCmdOptDef opts_setvcpus[ {"maximum", VSH_OT_BOOL, 0, N_("set maximum limit on next boot")}, {"config", VSH_OT_BOOL, 0, N_("affect next boot")}, {"live", VSH_OT_BOOL, 0, N_("affect running domain")}, + {"current", VSH_OT_BOOL, 0, N_("affect current domain")}, {NULL, 0, 0, NULL} }; @@ -3269,9 +3270,24 @@ cmdSetvcpus(vshControl *ctl, const vshCm int maximum = vshCommandOptBool(cmd, "maximum"); int config = vshCommandOptBool(cmd, "config"); int live = vshCommandOptBool(cmd, "live"); - int flags = ((maximum ? VIR_DOMAIN_VCPU_MAXIMUM : 0) | - (config ? VIR_DOMAIN_AFFECT_CONFIG : 0) | - (live ? VIR_DOMAIN_AFFECT_LIVE : 0)); + int current = vshCommandOptBool(cmd, "current"); + int flags = 0; + + if (current) { + if (live || config) { + vshError(ctl, "%s", _("--current must be specified exclusively")); + return false; + } + flags = VIR_DOMAIN_AFFECT_CURRENT; + } else { + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; + /* neither option is specified */ + if (!live && !config && !maximum) + flags = -1; + } if (!vshConnectionUsability(ctl, ctl->conn)) return false; @@ -3284,7 +3300,7 @@ cmdSetvcpus(vshControl *ctl, const vshCm goto cleanup; } - if (!flags) { + if (flags == -1) { if (virDomainSetVcpus(dom, count) != 0) { ret = false; } @@ -3294,6 +3310,8 @@ cmdSetvcpus(vshControl *ctl, const vshCm if (maximum) { vshDebug(ctl, VSH_ERR_DEBUG, "--maximum flag was given\n"); + flags |= VIR_DOMAIN_VCPU_MAXIMUM; + /* If neither the --config nor --live flags were given, OR if just the --live flag was given, we need to error out warning the user that the --maximum flag can only be used Index: libvirt/tools/virsh.pod =================================================================== --- libvirt.orig/tools/virsh.pod +++ libvirt/tools/virsh.pod @@ -740,7 +740,7 @@ exclusive. If no flag is specified, beha on hypervisor. =item B<setvcpus> I<domain-id> I<count> optional I<--maximum> I<--config> -I<--live> +I<--live> I<--current> Change the number of virtual CPUs active in a guest domain. By default, this command works on active guest domains. To change the settings for an @@ -758,7 +758,9 @@ If I<--live> is specified, the guest dom takes place immediately. Both the I<--config> and I<--live> flags may be specified together if supported by the hypervisor. -When neither the I<--config> nor I<--live> flags are given, the I<--live> +If I<--current> is specified, affect the current guest state. + +When neither flags are given, the I<--live> flag is assumed and the guest domain must be active. In this situation it is up to the hypervisor whether the I<--config> flag is also assumed, and therefore whether the XML configuration is adjusted to make the change
On 07/15/2011 01:02 AM, Taku Izumi wrote:
This patch adds the --current option to "virsh setvcpus" command. Currently "virsh setvcpus" command supports "--live" and "--config" , but "--current" option.
From view of consistency, it's reasonable to support "--current" option too.
When --current is specified, it affects a "current" domain.
Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com> --- tools/virsh.c | 26 ++++++++++++++++++++++---- tools/virsh.pod | 6 ++++-- 2 files changed, 26 insertions(+), 6 deletions(-)
@@ -3294,6 +3310,8 @@ cmdSetvcpus(vshControl *ctl, const vshCm if (maximum) { vshDebug(ctl, VSH_ERR_DEBUG, "--maximum flag was given\n");
+ flags |= VIR_DOMAIN_VCPU_MAXIMUM;
This is a bit separated from the rest of flags settings, so it took me two reads to make sure you didn't lose the bit, but your logic is valid. So no changes needed.
=item B<setvcpus> I<domain-id> I<count> optional I<--maximum> I<--config> -I<--live> +I<--live> I<--current>
Oh fun - merge conflict with my patch to tweak the use of "optional" in virsh.pod: https://www.redhat.com/archives/libvir-list/2011-July/msg00868.html
Change the number of virtual CPUs active in a guest domain. By default, this command works on active guest domains. To change the settings for an @@ -758,7 +758,9 @@ If I<--live> is specified, the guest dom takes place immediately. Both the I<--config> and I<--live> flags may be specified together if supported by the hypervisor.
-When neither the I<--config> nor I<--live> flags are given, the I<--live> +If I<--current> is specified, affect the current guest state. + +When neither flags are given, the I<--live>
s/neither/no/
flag is assumed and the guest domain must be active. In this situation it is up to the hypervisor whether the I<--config> flag is also assumed, and therefore whether the XML configuration is adjusted to make the change
ACK with the one word nit fixed, so series pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 07/15/2011 12:54 AM, Taku Izumi wrote:
Hi all,
This patchset adds the --current option to "virsh setvcpus" command. Currently "virsh setvcpus" command supports "--live" and "--config" , but "--current" option.
From view of consistency, it's reasonable to support "--current" option too.
*[PATCH 1/3] setvcpus: extend virDomainSetVcpusFlags API to support current flag *[PATCH 2/3] setvcpus: extend qemuDomainSetVcpusFlags() to support current flag *[PATCH 3/3] setvcpus: add "--current" option to "virsh setvcpus"
Guess what - qemuDomainGetVcpusFlags also needs to learn the _CURRENT option. Do you want to take that on next? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Taku Izumi