[libvirt] [PATCH] vcpu: teach getVcpusFlags about current
Now that virDomainSetVcpusFlags knows about VIR_DOMAIN_AFFECT_CURRENT, so should virDomainGetVcpusFlags. Unfortunately, the virsh counterpart 'virsh vcpucount' has already commandeered --current for a different meaning, so virsh does not have a way to expose this new calling capability unless we either break backward compatibility or consistency with other virsh commands that take --live and --config. * src/libvirt.c (virDomainGetVcpusFlags): Allow VIR_DOMAIN_AFFECT_CURRENT. * src/libxl/libxl_driver.c (libxlDomainGetVcpusFlags): Likewise. * src/qemu/qemu_driver.c (qemudDomainGetVcpusFlags): Likewise. * src/test/test_driver.c (testDomainGetVcpusFlags): Likewise. * src/xen/xen_driver.c (xenUnifiedDomainGetVcpusFlags): Likewise. --- In response to: https://www.redhat.com/archives/libvir-list/2011-July/msg00967.html src/libvirt.c | 4 ++-- src/libxl/libxl_driver.c | 29 +++++++++++++++++++++-------- src/qemu/qemu_driver.c | 29 +++++++++++++++++++++-------- src/test/test_driver.c | 31 +++++++++++++++++++++++-------- src/xen/xen_driver.c | 7 ------- 5 files changed, 67 insertions(+), 33 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index 4de718d..def3fb9 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -6951,8 +6951,8 @@ virDomainGetVcpusFlags(virDomainPtr domain, unsigned int flags) return -1; } - /* Exactly one of these two flags should be set. */ - if (!(flags & VIR_DOMAIN_AFFECT_LIVE) == !(flags & VIR_DOMAIN_AFFECT_CONFIG)) { + /* At most one of these two flags should be set. */ + if ((flags & VIR_DOMAIN_AFFECT_LIVE) && (flags & VIR_DOMAIN_AFFECT_CONFIG)) { virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); goto error; } diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index cc37d05..2e0d377 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -2327,18 +2327,12 @@ libxlDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags) virDomainObjPtr vm; virDomainDefPtr def; int ret = -1; + bool active; virCheckFlags(VIR_DOMAIN_VCPU_LIVE | VIR_DOMAIN_VCPU_CONFIG | VIR_DOMAIN_VCPU_MAXIMUM, -1); - /* Exactly one of LIVE or CONFIG must be set. */ - if (!(flags & VIR_DOMAIN_VCPU_LIVE) == !(flags & VIR_DOMAIN_VCPU_CONFIG)) { - libxlError(VIR_ERR_INVALID_ARG, - _("invalid flag combination: (0x%x)"), flags); - return -1; - } - libxlDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); libxlDriverUnlock(driver); @@ -2348,14 +2342,33 @@ libxlDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags) goto cleanup; } + active = virDomainObjIsActive(vm); + + if ((flags & (VIR_DOMAIN_VCPU_LIVE | VIR_DOMAIN_VCPU_CONFIG)) == 0) { + if (active) + flags |= VIR_DOMAIN_VCPU_LIVE; + else + flags |= VIR_DOMAIN_VCPU_CONFIG; + } + if ((flags & VIR_DOMAIN_VCPU_LIVE) && (flags & VIR_DOMAIN_VCPU_CONFIG)) { + libxlError(VIR_ERR_INVALID_ARG, + _("invalid flag combination: (0x%x)"), flags); + return -1; + } + if (flags & VIR_DOMAIN_VCPU_LIVE) { - if (!virDomainObjIsActive(vm)) { + if (!active) { libxlError(VIR_ERR_OPERATION_INVALID, "%s", _("Domain is not running")); goto cleanup; } def = vm->def; } else { + if (!vm->persistent) { + libxlError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain is transient")); + goto cleanup; + } def = vm->newDef ? vm->newDef : vm->def; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1a3fbfb..536cd5c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3408,18 +3408,12 @@ qemudDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags) virDomainObjPtr vm; virDomainDefPtr def; int ret = -1; + bool active; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG | VIR_DOMAIN_VCPU_MAXIMUM, -1); - /* Exactly one of LIVE or CONFIG must be set. */ - if (!(flags & VIR_DOMAIN_AFFECT_LIVE) == !(flags & VIR_DOMAIN_AFFECT_CONFIG)) { - qemuReportError(VIR_ERR_INVALID_ARG, - _("invalid flag combination: (0x%x)"), flags); - return -1; - } - qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); qemuDriverUnlock(driver); @@ -3432,14 +3426,33 @@ qemudDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags) goto cleanup; } + active = virDomainObjIsActive(vm); + + if ((flags & (VIR_DOMAIN_VCPU_LIVE | VIR_DOMAIN_VCPU_CONFIG)) == 0) { + if (active) + flags |= VIR_DOMAIN_VCPU_LIVE; + else + flags |= VIR_DOMAIN_VCPU_CONFIG; + } + if ((flags & VIR_DOMAIN_AFFECT_LIVE) && (flags & VIR_DOMAIN_AFFECT_CONFIG)) { + qemuReportError(VIR_ERR_INVALID_ARG, + _("invalid flag combination: (0x%x)"), flags); + return -1; + } + if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (!virDomainObjIsActive(vm)) { + if (!active) { qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain not active")); goto cleanup; } def = vm->def; } else { + if (!vm->persistent) { + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("domain is transient")); + goto cleanup; + } def = vm->newDef ? vm->newDef : vm->def; } diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 064a1cd..28da8e7 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -2067,18 +2067,12 @@ testDomainGetVcpusFlags(virDomainPtr domain, unsigned int flags) virDomainObjPtr vm; virDomainDefPtr def; int ret = -1; + bool active; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG | VIR_DOMAIN_VCPU_MAXIMUM, -1); - /* Exactly one of LIVE or CONFIG must be set. */ - if (!(flags & VIR_DOMAIN_AFFECT_LIVE) == !(flags & VIR_DOMAIN_AFFECT_CONFIG)) { - testError(VIR_ERR_INVALID_ARG, - _("invalid flag combination: (0x%x)"), flags); - return -1; - } - testDriverLock(privconn); vm = virDomainFindByUUID(&privconn->domains, domain->uuid); testDriverUnlock(privconn); @@ -2091,14 +2085,35 @@ testDomainGetVcpusFlags(virDomainPtr domain, unsigned int flags) goto cleanup; } + active = virDomainObjIsActive(vm); + + if ((flags & (VIR_DOMAIN_VCPU_LIVE | VIR_DOMAIN_VCPU_CONFIG)) == 0) { + if (active) + flags |= VIR_DOMAIN_VCPU_LIVE; + else + flags |= VIR_DOMAIN_VCPU_CONFIG; + } + if ((flags & VIR_DOMAIN_AFFECT_LIVE) && (flags & VIR_DOMAIN_AFFECT_CONFIG)) { + testError(VIR_ERR_INVALID_ARG, + _("invalid flag combination: (0x%x)"), flags); + return -1; + } + + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (!virDomainObjIsActive(vm)) { + if (!active) { testError(VIR_ERR_OPERATION_INVALID, "%s", _("domain not active")); goto cleanup; } def = vm->def; } else { + if (!vm->persistent) { + testError(VIR_ERR_OPERATION_INVALID, "%s", + _("domain is transient")); + goto cleanup; + } def = vm->newDef ? vm->newDef : vm->def; } diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c index dd1ba6c..0f8b660 100644 --- a/src/xen/xen_driver.c +++ b/src/xen/xen_driver.c @@ -1201,13 +1201,6 @@ xenUnifiedDomainGetVcpusFlags (virDomainPtr dom, unsigned int flags) VIR_DOMAIN_VCPU_CONFIG | VIR_DOMAIN_VCPU_MAXIMUM, -1); - /* Exactly one of LIVE or CONFIG must be set. */ - if (!(flags & VIR_DOMAIN_VCPU_LIVE) == !(flags & VIR_DOMAIN_VCPU_CONFIG)) { - xenUnifiedError(VIR_ERR_INVALID_ARG, - _("invalid flag combination: (0x%x)"), flags); - return -1; - } - if (priv->opened[XEN_UNIFIED_XEND_OFFSET]) { ret = xenDaemonDomainGetVcpusFlags(dom, flags); if (ret != -2) -- 1.7.4.4
On 07/15/2011 05:38 PM, Eric Blake wrote:
Now that virDomainSetVcpusFlags knows about VIR_DOMAIN_AFFECT_CURRENT, so should virDomainGetVcpusFlags.
Unfortunately, the virsh counterpart 'virsh vcpucount' has already commandeered --current for a different meaning, so virsh does not have a way to expose this new calling capability unless we either break backward compatibility or consistency with other virsh commands that take --live and --config.
* src/libvirt.c (virDomainGetVcpusFlags): Allow VIR_DOMAIN_AFFECT_CURRENT. * src/libxl/libxl_driver.c (libxlDomainGetVcpusFlags): Likewise. * src/qemu/qemu_driver.c (qemudDomainGetVcpusFlags): Likewise. * src/test/test_driver.c (testDomainGetVcpusFlags): Likewise. * src/xen/xen_driver.c (xenUnifiedDomainGetVcpusFlags): Likewise. ---
In response to: https://www.redhat.com/archives/libvir-list/2011-July/msg00967.html
Also, squash in some docs: diff --git i/src/libvirt.c w/src/libvirt.c index c34b46c..4b22b3a 100644 --- i/src/libvirt.c +++ w/src/libvirt.c @@ -6933,10 +6933,13 @@ error: * not support it. This function requires privileged access to the * hypervisor. * - * @flags must include either VIR_DOMAIN_AFFECT_LIVE to query a - * running domain (which will fail if domain is not active), or - * VIR_DOMAIN_AFFECT_CONFIG to query the XML description of the - * domain. It is an error to set both flags. + * If @flags includes VIR_DOMAIN_AFFECT_LIVE, this will query a + * running domain (which will fail if domain is not active); if + * it includes VIR_DOMAIN_AFFECT_CONFIG, this will query the XML + * description of the domain. It is an error to set both flags. + * If neither flag is set (that is, VIR_DOMAIN_AFFECT_CURRENT), + * then the configuration queried depends on whether the domain + * is currently running. * * If @flags includes VIR_DOMAIN_VCPU_MAXIMUM, then the maximum * virtual CPU limit is queried. Otherwise, this call queries the -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Rename the existing --current flag to the new name --active, while adding a new flag --current to expose the new VIR_DOMAIN_AFFECT_CURRENT flag of virDomainGetVcpusFlags. For backwards compability, the output does not change (even though the label "current" no longer matches the spelling of the option that would trigger that number in isolation), and we accept "--current --live" as an undocumented synonym for "--active --live" to avoid breaking any existing clients. * tools/virsh.c (cmdVcpucount): Add --active flag, and rearrange existing flag handling to expose VIR_DOMAIN_AFFECT_CURRENT support. * tools/virsh.pod (vcpucount): Document this. --- Incorporating my proposal from: https://www.redhat.com/archives/libvir-list/2011-July/msg01099.html tools/virsh.c | 77 +++++++++++++++++++++++++++++++++++++++--------------- tools/virsh.pod | 19 ++++++++----- 2 files changed, 67 insertions(+), 29 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 2eb218a..65fba0f 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2738,9 +2738,11 @@ static const vshCmdInfo info_vcpucount[] = { static const vshCmdOptDef opts_vcpucount[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, {"maximum", VSH_OT_BOOL, 0, N_("get maximum cap on vcpus")}, - {"current", VSH_OT_BOOL, 0, N_("get current vcpu usage")}, - {"config", VSH_OT_BOOL, 0, N_("get value to be used on next boot")}, + {"active", VSH_OT_BOOL, 0, N_("get number of currently active vcpus")}, {"live", VSH_OT_BOOL, 0, N_("get value from running domain")}, + {"config", VSH_OT_BOOL, 0, N_("get value to be used on next boot")}, + {"current", VSH_OT_BOOL, 0, + N_("get value according to current domain state")}, {NULL, 0, 0, NULL} }; @@ -2750,31 +2752,50 @@ cmdVcpucount(vshControl *ctl, const vshCmd *cmd) virDomainPtr dom; bool ret = true; int maximum = vshCommandOptBool(cmd, "maximum"); - int current = vshCommandOptBool(cmd, "current"); + int active = vshCommandOptBool(cmd, "active"); int config = vshCommandOptBool(cmd, "config"); int live = vshCommandOptBool(cmd, "live"); - bool all = maximum + current + config + live == 0; + int current = vshCommandOptBool(cmd, "current"); + bool all = maximum + active + current + config + live == 0; int count; - if (maximum && current) { - vshError(ctl, "%s", - _("--maximum and --current cannot both be specified")); + /* We want one of each pair of mutually exclusive options; that + * is, use of flags requires exactly two options. We reject the + * use of more than 2 flags later on. */ + if (maximum + active + current + config + live == 1) { + if (maximum || active) { + vshError(ctl, + _("when using --%s, one of --config, --live, or --current " + "must be specified"), + maximum ? "maximum" : "active"); + } else { + vshError(ctl, + _("when using --%s, either --maximum or --active must be " + "specified"), + (current ? "current" : config ? "config" : "live")); + } return false; } - if (config && live) { + + /* Backwards compatibility: prior to 0.9.4, + * VIR_DOMAIN_AFFECT_CURRENT was unsupported, and --current meant + * the opposite of --maximum. Translate the old '--current + * --live' into the new '--active --live', while treating the new + * '--maximum --current' correctly rather than rejecting it as + * '--maximum --active'. */ + if (!maximum && !active && current) { + current = false; + active = true; + } + + if (maximum && active) { vshError(ctl, "%s", - _("--config and --live cannot both be specified")); + _("--maximum and --active cannot both be specified")); return false; } - /* We want one of each pair of mutually exclusive options; that - * is, use of flags requires exactly two options. */ - if (maximum + current + config + live == 1) { - vshError(ctl, - _("when using --%s, either --%s or --%s must be specified"), - (maximum ? "maximum" : current ? "current" - : config ? "config" : "live"), - maximum + current ? "config" : "maximum", - maximum + current ? "live" : "current"); + if (current + config + live > 1) { + vshError(ctl, "%s", + _("--config, --live, and --current are mutually exclusive")); return false; } @@ -2785,8 +2806,20 @@ cmdVcpucount(vshControl *ctl, const vshCmd *cmd) return false; /* In all cases, try the new API first; if it fails because we are - * talking to an older client, try a fallback API before giving - * up. */ + * talking to an older client, generally we try a fallback API + * before giving up. --current requires the new API, since we + * don't know whether the domain is running or inactive. */ + if (active) { + count = virDomainGetVcpusFlags(dom, + maximum ? VIR_DOMAIN_VCPU_MAXIMUM : 0); + if (count < 0) { + virshReportError(ctl); + ret = false; + } else { + vshPrint(ctl, "%d\n", count); + } + } + if (all || (maximum && config)) { count = virDomainGetVcpusFlags(dom, (VIR_DOMAIN_VCPU_MAXIMUM | VIR_DOMAIN_AFFECT_CONFIG)); @@ -2838,7 +2871,7 @@ cmdVcpucount(vshControl *ctl, const vshCmd *cmd) last_error = NULL; } - if (all || (current && config)) { + if (all || (active && config)) { count = virDomainGetVcpusFlags(dom, VIR_DOMAIN_AFFECT_CONFIG); if (count < 0 && (last_error->code == VIR_ERR_NO_SUPPORT || last_error->code == VIR_ERR_INVALID_ARG)) { @@ -2875,7 +2908,7 @@ cmdVcpucount(vshControl *ctl, const vshCmd *cmd) last_error = NULL; } - if (all || (current && live)) { + if (all || (active && live)) { count = virDomainGetVcpusFlags(dom, VIR_DOMAIN_AFFECT_LIVE); if (count < 0 && (last_error->code == VIR_ERR_NO_SUPPORT || last_error->code == VIR_ERR_INVALID_ARG)) { diff --git a/tools/virsh.pod b/tools/virsh.pod index 5dc3d2e..2659ecb 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -816,20 +816,25 @@ is not available the processes will provide an exit code of 1. Undefine the configuration for an inactive domain. Since it's not running the domain name or UUID must be used as the I<domain-id>. -=item B<vcpucount> I<domain-id> [{I<--maximum> | I<--current>} -{I<--config> | I<--live>}] +=item B<vcpucount> I<domain-id> [{I<--maximum> | I<--active>} +{I<--config> | I<--live> | I<--current>}] Print information about the virtual cpu counts of the given I<domain-id>. If no flags are specified, all possible counts are listed in a table; otherwise, the output is limited to just the -numeric value requested. +numeric value requested. For historical reasons, the table +lists the label "current" on the rows that can be queried in isolation +via the I<--active> flag, rather than relating to the I<--current> flag. I<--maximum> requests information on the maximum cap of vcpus that a -domain can add via B<setvcpus>, while I<--current> shows the current +domain can add via B<setvcpus>, while I<--active> shows the current usage; these two flags cannot both be specified. I<--config> -requests information regarding the next time the domain will be -booted, while I<--live> requires a running domain and lists current -values; these two flags cannot both be specified. +requires a persistent domain and requests information regarding the next +time the domain will be booted, I<--live> requires a running domain and +lists current values, and I<--current> queries according to the current +state of the domain (corresponding to I<--live> if running, or +I<--config> if inactive); these three flags are mutually exclusive. +Thus, this command always takes exactly zero or two flags. =item B<vcpuinfo> I<domain-id> -- 1.7.4.4
On 07/18/2011 06:12 PM, Eric Blake wrote:
Rename the existing --current flag to the new name --active, while adding a new flag --current to expose the new VIR_DOMAIN_AFFECT_CURRENT flag of virDomainGetVcpusFlags.
For backwards compability, the output does not change (even though the label "current" no longer matches the spelling of the option that would trigger that number in isolation), and we accept "--current --live" as an undocumented synonym for "--active --live" to avoid breaking any existing clients.
* tools/virsh.c (cmdVcpucount): Add --active flag, and rearrange existing flag handling to expose VIR_DOMAIN_AFFECT_CURRENT support. * tools/virsh.pod (vcpucount): Document this. ---
Incorporating my proposal from: https://www.redhat.com/archives/libvir-list/2011-July/msg01099.html
ACK.
On 07/25/2011 03:33 PM, Laine Stump wrote:
On 07/18/2011 06:12 PM, Eric Blake wrote:
Rename the existing --current flag to the new name --active, while adding a new flag --current to expose the new VIR_DOMAIN_AFFECT_CURRENT flag of virDomainGetVcpusFlags.
For backwards compability, the output does not change (even though the label "current" no longer matches the spelling of the option that would trigger that number in isolation), and we accept "--current --live" as an undocumented synonym for "--active --live" to avoid breaking any existing clients.
* tools/virsh.c (cmdVcpucount): Add --active flag, and rearrange existing flag handling to expose VIR_DOMAIN_AFFECT_CURRENT support. * tools/virsh.pod (vcpucount): Document this. ---
Incorporating my proposal from: https://www.redhat.com/archives/libvir-list/2011-July/msg01099.html
ACK.
Pushed with one fix that I noticed in re-reading the patch:
/* In all cases, try the new API first; if it fails because we are - * talking to an older client, try a fallback API before giving - * up. */ + * talking to an older client, generally we try a fallback API + * before giving up. --current requires the new API, since we + * don't know whether the domain is running or inactive. */ + if (active) { + count = virDomainGetVcpusFlags(dom, + maximum ?
VIR_DOMAIN_VCPU_MAXIMUM : 0); This line should have read "if (current)", not "if (active)". -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 07/15/2011 07:38 PM, Eric Blake wrote:
Now that virDomainSetVcpusFlags knows about VIR_DOMAIN_AFFECT_CURRENT, so should virDomainGetVcpusFlags.
Unfortunately, the virsh counterpart 'virsh vcpucount' has already commandeered --current for a different meaning, so virsh does not have a way to expose this new calling capability unless we either break backward compatibility or consistency with other virsh commands that take --live and --config.
* src/libvirt.c (virDomainGetVcpusFlags): Allow VIR_DOMAIN_AFFECT_CURRENT. * src/libxl/libxl_driver.c (libxlDomainGetVcpusFlags): Likewise. * src/qemu/qemu_driver.c (qemudDomainGetVcpusFlags): Likewise. * src/test/test_driver.c (testDomainGetVcpusFlags): Likewise. * src/xen/xen_driver.c (xenUnifiedDomainGetVcpusFlags): Likewise. ---
In response to: https://www.redhat.com/archives/libvir-list/2011-July/msg00967.html
ACK (including the docs you later squashed in)
On 07/25/2011 03:33 PM, Laine Stump wrote:
On 07/15/2011 07:38 PM, Eric Blake wrote:
Now that virDomainSetVcpusFlags knows about VIR_DOMAIN_AFFECT_CURRENT, so should virDomainGetVcpusFlags.
Unfortunately, the virsh counterpart 'virsh vcpucount' has already commandeered --current for a different meaning, so virsh does not have a way to expose this new calling capability unless we either break backward compatibility or consistency with other virsh commands that take --live and --config.
* src/libvirt.c (virDomainGetVcpusFlags): Allow VIR_DOMAIN_AFFECT_CURRENT. * src/libxl/libxl_driver.c (libxlDomainGetVcpusFlags): Likewise. * src/qemu/qemu_driver.c (qemudDomainGetVcpusFlags): Likewise. * src/test/test_driver.c (testDomainGetVcpusFlags): Likewise. * src/xen/xen_driver.c (xenUnifiedDomainGetVcpusFlags): Likewise. ---
In response to: https://www.redhat.com/archives/libvir-list/2011-July/msg00967.html
ACK (including the docs you later squashed in)
Thanks; pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Laine Stump