[libvirt] [PATCH 0/3] setmem: add --current option to virsh setmem command
Hi all, This patchset adds --current option to virsh setmem command. This is based on the discussion at http://www.redhat.com/archives/libvir-list/2011-March/msg00564.html *[PATCH 1/3] setmem: introduce VIR_DOMAIN_MEM_CURRENT flag *[PATCH 2/3] setmem: add virDomainSetMemroyFlags(,,VIR_DOMAIN_MEM_CURRENT) support to qemu driver *[PATCH 3/3] setmem: add --current option to virsh setmem command Best regards, Taku Izumi
This patch introduces VIR_DOMAIN_MEM_CURRENT flag and modifies virDomainSetMemoryFlags function to support it. Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com> --- include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 13 +++++++------ 2 files changed, 8 insertions(+), 6 deletions(-) Index: libvirt/include/libvirt/libvirt.h.in =================================================================== --- libvirt.orig/include/libvirt/libvirt.h.in +++ libvirt/include/libvirt/libvirt.h.in @@ -854,6 +854,7 @@ int virDomainGetMemoryParameters(vir /* Memory size modification flags. */ typedef enum { + VIR_DOMAIN_MEM_CURRENT= 0, /* affect current domain */ VIR_DOMAIN_MEM_LIVE = (1 << 0), /* affect active domain */ VIR_DOMAIN_MEM_CONFIG = (1 << 1), /* affect next boot */ } virDomainMemoryModFlags; Index: libvirt/src/libvirt.c =================================================================== --- libvirt.orig/src/libvirt.c +++ libvirt/src/libvirt.c @@ -2862,10 +2862,12 @@ error: * to Domain0 i.e. the domain where the application runs. * This funcation may requires privileged access to the hypervisor. * - * @flags must include VIR_DOMAIN_MEM_LIVE to affect a running - * domain (which may fail if domain is not active), or - * VIR_DOMAIN_MEM_CONFIG to affect the next boot via the XML - * description of the domain. Both flags may be set. + * @flags may include VIR_DOMAIN_MEM_LIVE or VIR_DOMAIN_MEM_CONFIG. + * Both flags may be set. If VIR_DOMAIN_MEM_LIVE is set, the change affects + * a running domain and may fail if domain is not active. + * If VIR_DOMAIN_MEM_CONFIG is set, the change affects the next boot via + * the XML description on the domain. If neither flag is specified + * (=VIR_DOMAIN_MEM_CURRENT), behavior is different depending on hypervisor. * * Returns 0 in case of success, -1 in case of failure. */ @@ -2891,8 +2893,7 @@ virDomainSetMemoryFlags(virDomainPtr dom goto error; } - if (memory < 4096 || - (flags & (VIR_DOMAIN_MEM_LIVE | VIR_DOMAIN_MEM_CONFIG)) == 0) { + if (memory < 4096) { virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); goto error; }
On 03/22/2011 11:47 PM, Taku Izumi wrote:
This patch introduces VIR_DOMAIN_MEM_CURRENT flag and modifies virDomainSetMemoryFlags function to support it.
/* Memory size modification flags. */ typedef enum { + VIR_DOMAIN_MEM_CURRENT= 0, /* affect current domain */
I tweaked the spacing here.
VIR_DOMAIN_MEM_LIVE = (1 << 0), /* affect active domain */ VIR_DOMAIN_MEM_CONFIG = (1 << 1), /* affect next boot */ } virDomainMemoryModFlags; Index: libvirt/src/libvirt.c =================================================================== --- libvirt.orig/src/libvirt.c +++ libvirt/src/libvirt.c @@ -2862,10 +2862,12 @@ error: * to Domain0 i.e. the domain where the application runs. * This funcation may requires privileged access to the hypervisor.
Oops, let's fix that typo in "funcation" while we're here.
* - * @flags must include VIR_DOMAIN_MEM_LIVE to affect a running - * domain (which may fail if domain is not active), or - * VIR_DOMAIN_MEM_CONFIG to affect the next boot via the XML - * description of the domain. Both flags may be set. + * @flags may include VIR_DOMAIN_MEM_LIVE or VIR_DOMAIN_MEM_CONFIG. + * Both flags may be set. If VIR_DOMAIN_MEM_LIVE is set, the change affects + * a running domain and may fail if domain is not active. + * If VIR_DOMAIN_MEM_CONFIG is set, the change affects the next boot via + * the XML description on the domain. If neither flag is specified + * (=VIR_DOMAIN_MEM_CURRENT), behavior is different depending on hypervisor.
I tweaked this wording a bit.
* * Returns 0 in case of success, -1 in case of failure. */ @@ -2891,8 +2893,7 @@ virDomainSetMemoryFlags(virDomainPtr dom goto error; }
- if (memory < 4096 || - (flags & (VIR_DOMAIN_MEM_LIVE | VIR_DOMAIN_MEM_CONFIG)) == 0) { + if (memory < 4096) { virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); goto error; }
Here's what I squashed in before pushing: diff --git i/include/libvirt/libvirt.h.in w/include/libvirt/libvirt.h.in index c9129bc..d765412 100644 --- i/include/libvirt/libvirt.h.in +++ w/include/libvirt/libvirt.h.in @@ -854,9 +854,9 @@ int virDomainGetMemoryParameters(virDomainPtr domain, /* Memory size modification flags. */ typedef enum { - VIR_DOMAIN_MEM_CURRENT= 0, /* affect current domain */ - VIR_DOMAIN_MEM_LIVE = (1 << 0), /* affect active domain */ - VIR_DOMAIN_MEM_CONFIG = (1 << 1), /* affect next boot */ + VIR_DOMAIN_MEM_CURRENT = 0, /* affect current domain state */ + VIR_DOMAIN_MEM_LIVE = (1 << 0), /* affect active domain */ + VIR_DOMAIN_MEM_CONFIG = (1 << 1), /* affect next boot */ } virDomainMemoryModFlags; diff --git i/src/libvirt.c w/src/libvirt.c index ee11643..dde4bd4 100644 --- i/src/libvirt.c +++ w/src/libvirt.c @@ -2822,14 +2822,17 @@ error: * Dynamically change the target amount of physical memory allocated to a * domain. If domain is NULL, then this change the amount of memory reserved * to Domain0 i.e. the domain where the application runs. - * This funcation may requires privileged access to the hypervisor. + * This function may requires privileged access to the hypervisor. * * @flags may include VIR_DOMAIN_MEM_LIVE or VIR_DOMAIN_MEM_CONFIG. * Both flags may be set. If VIR_DOMAIN_MEM_LIVE is set, the change affects - * a running domain and may fail if domain is not active. - * If VIR_DOMAIN_MEM_CONFIG is set, the change affects the next boot via - * the XML description on the domain. If neither flag is specified - * (=VIR_DOMAIN_MEM_CURRENT), behavior is different depending on hypervisor. + * a running domain and will fail if domain is not active. + * If VIR_DOMAIN_MEM_CONFIG is set, the change affects persistent state, + * and will fail for transient domains. If neither flag is specified + * (that is, @flags is VIR_DOMAIN_MEM_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. + * Not all hypervisors can support all flag combinations. * * Returns 0 in case of success, -1 in case of failure. */ -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
This patch adds virDomainSetMemroyFlags(,,VIR_DOMAIN_MEM_CURRENT) support code to qemu driver. Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com> --- src/qemu/qemu_driver.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) Index: libvirt/src/qemu/qemu_driver.c =================================================================== --- libvirt.orig/src/qemu/qemu_driver.c +++ libvirt/src/qemu/qemu_driver.c @@ -1575,16 +1575,11 @@ static int qemudDomainSetMemoryFlags(vir qemuDomainObjPrivatePtr priv; virDomainObjPtr vm; virDomainDefPtr persistentDef = NULL; - int ret = -1, r; + int ret = -1, r, isActive; virCheckFlags(VIR_DOMAIN_MEM_LIVE | VIR_DOMAIN_MEM_CONFIG, -1); - if ((flags & (VIR_DOMAIN_MEM_LIVE | VIR_DOMAIN_MEM_CONFIG)) == 0) { - qemuReportError(VIR_ERR_INVALID_ARG, - _("invalid flag combination: (0x%x)"), flags); - } - qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); qemuDriverUnlock(driver); @@ -1605,7 +1600,16 @@ static int qemudDomainSetMemoryFlags(vir if (qemuDomainObjBeginJob(vm) < 0) goto cleanup; - if (!virDomainObjIsActive(vm) && (flags & VIR_DOMAIN_MEM_LIVE)) { + isActive = virDomainObjIsActive(vm); + + if (flags == VIR_DOMAIN_MEM_CURRENT) { + if (isActive) + flags = VIR_DOMAIN_MEM_LIVE; + else + flags = VIR_DOMAIN_MEM_CONFIG; + } + + if (!isActive && (flags & VIR_DOMAIN_MEM_LIVE)) { qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain is not running")); goto endjob;
Long subject line. 'git shortlog -20' should give you a feel for how to shorten this; I went with: setmem: add VIR_DOMAIN_MEM_CURRENT support to qemu On 03/22/2011 11:48 PM, Taku Izumi wrote:
This patch adds virDomainSetMemroyFlags(,,VIR_DOMAIN_MEM_CURRENT) support
s/Memroy/Memory/
code to qemu driver.
Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com> --- src/qemu/qemu_driver.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)
Index: libvirt/src/qemu/qemu_driver.c =================================================================== --- libvirt.orig/src/qemu/qemu_driver.c +++ libvirt/src/qemu/qemu_driver.c @@ -1575,16 +1575,11 @@ static int qemudDomainSetMemoryFlags(vir qemuDomainObjPrivatePtr priv; virDomainObjPtr vm; virDomainDefPtr persistentDef = NULL; - int ret = -1, r; + int ret = -1, r, isActive;
Hmm, based on the name, I would have guessed isActive is better as a bool.
virCheckFlags(VIR_DOMAIN_MEM_LIVE | VIR_DOMAIN_MEM_CONFIG, -1);
- if ((flags & (VIR_DOMAIN_MEM_LIVE | VIR_DOMAIN_MEM_CONFIG)) == 0) { - qemuReportError(VIR_ERR_INVALID_ARG, - _("invalid flag combination: (0x%x)"), flags); - } - qemuDriverLock(driver); vm = virDomainFindByUUID(&driver->domains, dom->uuid); qemuDriverUnlock(driver); @@ -1605,7 +1600,16 @@ static int qemudDomainSetMemoryFlags(vir if (qemuDomainObjBeginJob(vm) < 0) goto cleanup;
- if (!virDomainObjIsActive(vm) && (flags & VIR_DOMAIN_MEM_LIVE)) { + isActive = virDomainObjIsActive(vm);
Surprisingly, virDomainObjIsActive returns int instead of bool. Well, no longer :)
+ + if (flags == VIR_DOMAIN_MEM_CURRENT) { + if (isActive) + flags = VIR_DOMAIN_MEM_LIVE; + else + flags = VIR_DOMAIN_MEM_CONFIG; + } + + if (!isActive && (flags & VIR_DOMAIN_MEM_LIVE)) { qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", _("domain is not running")); goto endjob;
I squashed this in, then pushed: diff --git i/src/conf/domain_conf.h w/src/conf/domain_conf.h index 10e73cb..95bd11e 100644 --- i/src/conf/domain_conf.h +++ w/src/conf/domain_conf.h @@ -1172,7 +1172,7 @@ struct _virDomainObjList { virHashTable *objs; }; -static inline int +static inline bool virDomainObjIsActive(virDomainObjPtr dom) { return dom->def->id != -1; diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index c0e706c..6a0bf24 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -1576,7 +1576,8 @@ static int qemudDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, qemuDomainObjPrivatePtr priv; virDomainObjPtr vm; virDomainDefPtr persistentDef = NULL; - int ret = -1, r, isActive; + int ret = -1, r; + bool isActive; virCheckFlags(VIR_DOMAIN_MEM_LIVE | VIR_DOMAIN_MEM_CONFIG, -1); -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
This patch adds the new option (--current) to the "virsh setmem" command. When --current option is specified, it affects a "current" domain. The word "current" denotes that if a domain is running, it affects a running domain only; otherwise it affects a persistent domain. Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com> --- tools/virsh.c | 20 +++++++++++++++----- tools/virsh.pod | 7 +++++-- 2 files changed, 20 insertions(+), 7 deletions(-) Index: libvirt/tools/virsh.c =================================================================== --- libvirt.orig/tools/virsh.c +++ libvirt/tools/virsh.c @@ -2920,6 +2920,7 @@ static const vshCmdOptDef opts_setmem[] {"kilobytes", VSH_OT_INT, VSH_OFLAG_REQ, N_("number of kilobytes of memory")}, {"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} }; @@ -2932,14 +2933,23 @@ cmdSetmem(vshControl *ctl, const vshCmd int ret = TRUE; int config = vshCommandOptBool(cmd, "config"); int live = vshCommandOptBool(cmd, "live"); + int current = vshCommandOptBool(cmd, "current"); int flags = 0; - /* Need to use flags if config was specified, but prefer older api - * for live-only behavior otherwise */ - if (config) { - flags = VIR_DOMAIN_MEM_CONFIG; + if (current) { + if (live || config) { + vshError(ctl, "%s", _("--current must be specified exclusively")); + return FALSE; + } + flags = VIR_DOMAIN_MEM_CURRENT; + } else { + if (config) + flags |= VIR_DOMAIN_MEM_CONFIG; if (live) flags |= VIR_DOMAIN_MEM_LIVE; + /* neither option is specified */ + if (!live && !config) + flags = -1; } if (!vshConnectionUsability(ctl, ctl->conn)) @@ -2972,7 +2982,7 @@ cmdSetmem(vshControl *ctl, const vshCmd return FALSE; } - if (!flags) { + if (flags == -1) { if (virDomainSetMemory(dom, kilobytes) != 0) { ret = FALSE; } Index: libvirt/tools/virsh.pod =================================================================== --- libvirt.orig/tools/virsh.pod +++ libvirt/tools/virsh.pod @@ -578,12 +578,15 @@ Therefore, -1 is a useful shorthand for B<Note>: The weight and cap parameters are defined only for the XEN_CREDIT scheduler and are now I<DEPRECATED>. -=item B<setmem> I<domain-id> B<kilobytes> optional I<--config> I<--live> +=item B<setmem> I<domain-id> B<kilobytes> optional I<--config> I<--live> I<--current> Change the memory allocation for a guest domain. If I<--live> is specified, perform a memory balloon of a running guest. If I<--config> is specified, affect the next boot of a persistent guest. -Both flags may be given. If neither flag is given, I<--live> is assumed. +If I<--current> is specified, affect a current guest. +Both I<--live> and I<--config> flags may be given, but I<--current> is +exclusive. If neither flag is specified, behavior is different depending +on hypervisor. Some hypervisors require a larger granularity than kilobytes, and requests that are not an even multiple will be rounded up. For example, vSphere/ESX
On 03/22/2011 11:49 PM, Taku Izumi wrote:
This patch adds the new option (--current) to the "virsh setmem" command. When --current option is specified, it affects a "current" domain. The word "current" denotes that if a domain is running, it affects a running domain only; otherwise it affects a persistent domain.
Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com> --- tools/virsh.c | 20 +++++++++++++++----- tools/virsh.pod | 7 +++++--
All right - you remembered to update the docs at the same time!
+ if (current) { + if (live || config) { + vshError(ctl, "%s", _("--current must be specified exclusively")); + return FALSE; + } + flags = VIR_DOMAIN_MEM_CURRENT; + } else { + if (config) + flags |= VIR_DOMAIN_MEM_CONFIG; if (live) flags |= VIR_DOMAIN_MEM_LIVE; + /* neither option is specified */ + if (!live && !config) + flags = -1; }
if (!vshConnectionUsability(ctl, ctl->conn)) @@ -2972,7 +2982,7 @@ cmdSetmem(vshControl *ctl, const vshCmd return FALSE; }
- if (!flags) { + if (flags == -1) { if (virDomainSetMemory(dom, kilobytes) != 0) {
Looks reasonable.
+++ libvirt/tools/virsh.pod @@ -578,12 +578,15 @@ Therefore, -1 is a useful shorthand for B<Note>: The weight and cap parameters are defined only for the XEN_CREDIT scheduler and are now I<DEPRECATED>.
-=item B<setmem> I<domain-id> B<kilobytes> optional I<--config> I<--live> +=item B<setmem> I<domain-id> B<kilobytes> optional I<--config> I<--live> I<--current>
I wrapped this at 80 columns.
Change the memory allocation for a guest domain. If I<--live> is specified, perform a memory balloon of a running guest. If I<--config> is specified, affect the next boot of a persistent guest. -Both flags may be given. If neither flag is given, I<--live> is assumed. +If I<--current> is specified, affect a current guest.
I tweaked this to 'current guest state (active or persistent)'.
+Both I<--live> and I<--config> flags may be given, but I<--current> is +exclusive. If neither flag is specified, behavior is different depending +on hypervisor.
ACK and applied. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Taku Izumi