[libvirt] [PATCH] RFC: virsh setmem: configure inactive domains' memory size
Currently "virsh setmem" is not allowed to use against inactive domains. By applying this patch, we can change the memory size of inactive domains by "virsh setmem". Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com> --- tools/virsh.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 62 insertions(+), 8 deletions(-) Index: libvirt-git/tools/virsh.c =================================================================== --- libvirt-git.orig/tools/virsh.c +++ libvirt-git/tools/virsh.c @@ -2919,39 +2919,93 @@ cmdSetmem(vshControl *ctl, const vshCmd virDomainPtr dom; virDomainInfo info; unsigned long kilobytes; - int ret = TRUE; + int ret = FALSE; + + char *xmlStr; + virBuffer memBuf = VIR_BUFFER_INITIALIZER; + xmlDocPtr xmlDocObj = NULL; + xmlXPathContextPtr ctxt = NULL; + xmlXPathObjectPtr result = NULL; + xmlChar *newXml = NULL; + int newXmlSize; if (!vshConnectionUsability(ctl, ctl->conn)) - return FALSE; + return ret; if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) - return FALSE; + return ret; kilobytes = vshCommandOptUL(cmd, "kilobytes", NULL); if (kilobytes <= 0) { virDomainFree(dom); vshError(ctl, _("Invalid value of %lu for memory size"), kilobytes); - return FALSE; + return ret; } if (virDomainGetInfo(dom, &info) != 0) { virDomainFree(dom); vshError(ctl, "%s", _("Unable to verify MaxMemorySize")); - return FALSE; + return ret; } if (kilobytes > info.maxMem) { virDomainFree(dom); vshError(ctl, _("Requested memory size %lu kb is larger than maximum of %lu kb"), kilobytes, info.maxMem); - return FALSE; + return ret; } - if (virDomainSetMemory(dom, kilobytes) != 0) { - ret = FALSE; + if (virDomainIsActive(dom) == 1) { + if (virDomainSetMemory(dom, kilobytes) == 0) + ret = TRUE; + virDomainFree(dom); + return ret; } + /* domain is inactive */ + + xmlStr = virDomainGetXMLDesc(dom, VIR_DOMAIN_XML_SECURE | + VIR_DOMAIN_XML_INACTIVE); + if (!xmlStr) + goto cleanup; + + xmlDocObj = xmlReadDoc((const xmlChar *)xmlStr, "domain.xml", NULL, + XML_PARSE_NOENT | XML_PARSE_NONET | + XML_PARSE_NOWARNING); + if (!xmlDocObj) + goto cleanup; + + ctxt = xmlXPathNewContext(xmlDocObj); + if (!ctxt) + goto cleanup; + + result = xmlXPathEvalExpression((xmlChar *)"/domain/currentMemory", ctxt); + if (xmlXPathNodeSetIsEmpty(result->nodesetval) || + result->nodesetval->nodeNr != 1 || + result->nodesetval->nodeTab[0]->type != XML_ELEMENT_NODE) { + goto cleanup; + } + + virBufferVSprintf(&memBuf, "%lu", kilobytes); + xmlNodeSetContent(result->nodesetval->nodeTab[0], + (const xmlChar *)virBufferContentAndReset(&memBuf)); + xmlDocDumpMemory(xmlDocObj, &newXml, &newXmlSize); + if (!newXml) + goto cleanup; + virDomainFree(dom); + dom = virDomainDefineXML(ctl->conn, (char *)newXml); + if (dom) + ret = TRUE; + +cleanup: + xmlFree(newXml); + xmlXPathFreeContext(ctxt); + xmlFreeDoc(xmlDocObj); + VIR_FREE(xmlStr); + if (dom) + virDomainFree(dom); + return ret; }
On 02/24/2011 03:25 AM, Taku Izumi wrote:
Currently "virsh setmem" is not allowed to use against inactive domains. By applying this patch, we can change the memory size of inactive domains by "virsh setmem".
- if (virDomainSetMemory(dom, kilobytes) != 0) { - ret = FALSE; + if (virDomainIsActive(dom) == 1) { + if (virDomainSetMemory(dom, kilobytes) == 0)
This is slightly racy - in between the time that you probe if the domain is active and actually request to set memory, the domain may have gone inactive (or vice versa, in between the time you see that it is inactive and start modifying the xml, it may have become active). I think the better fix here would be to add a new API, virDomainSetMemoryFlags, similar to virDomainUpdateDeviceFlags, where the flags can request whether you want to affect running, persistent, or both configurations all from a single API. Falling back to XML editing when the new API is not present is reasonable, but not an excuse for not providing the right interface to begin with. So, I'm debating whether to apply this now or whether to add the better API first; anyone else want to chime in? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Hi Eric, Thank you for reply. (2011/02/25 6:43), Eric Blake wrote:
On 02/24/2011 03:25 AM, Taku Izumi wrote:
Currently "virsh setmem" is not allowed to use against inactive domains. By applying this patch, we can change the memory size of inactive domains by "virsh setmem".
- if (virDomainSetMemory(dom, kilobytes) != 0) { - ret = FALSE; + if (virDomainIsActive(dom) == 1) { + if (virDomainSetMemory(dom, kilobytes) == 0)
This is slightly racy - in between the time that you probe if the domain is active and actually request to set memory, the domain may have gone inactive (or vice versa, in between the time you see that it is inactive and start modifying the xml, it may have become active).
I think the better fix here would be to add a new API, virDomainSetMemoryFlags, similar to virDomainUpdateDeviceFlags, where the flags can request whether you want to affect running, persistent, or both configurations all from a single API. Falling back to XML editing when the new API is not present is reasonable, but not an excuse for not providing the right interface to begin with.
So, I'm debating whether to apply this now or whether to add the better API first; anyone else want to chime in?
I also want to use not only "virsh setmem" command, but the following commands against inactive domains: - virsh setmaxmem - virsh vcpupin - virsh attach-device, detach-device - virsh setvcpus (already done) However, virDomainSetMemory, virDomainSetMaxMemory and virDomainPinVcpu are not allowed to use against inactive domains, so we need to extend those APIs like virDomainSetMemoryFlags. But although new APIs obsoletes old APIs, we can't of-course clear old APIs away for compatibility. I am concerned the number of APIs increases and the source codes become complex (and a little ugly), so I selected the method like this patch instead. If you say new APIs should be introduced, I have no objection. Taku Izumi
On Thu, Feb 24, 2011 at 02:43:03PM -0700, Eric Blake wrote:
On 02/24/2011 03:25 AM, Taku Izumi wrote:
Currently "virsh setmem" is not allowed to use against inactive domains. By applying this patch, we can change the memory size of inactive domains by "virsh setmem".
- if (virDomainSetMemory(dom, kilobytes) != 0) { - ret = FALSE; + if (virDomainIsActive(dom) == 1) { + if (virDomainSetMemory(dom, kilobytes) == 0)
This is slightly racy - in between the time that you probe if the domain is active and actually request to set memory, the domain may have gone inactive (or vice versa, in between the time you see that it is inactive and start modifying the xml, it may have become active).
I think the better fix here would be to add a new API, virDomainSetMemoryFlags, similar to virDomainUpdateDeviceFlags, where the flags can request whether you want to affect running, persistent, or both configurations all from a single API. Falling back to XML editing when the new API is not present is reasonable, but not an excuse for not providing the right interface to begin with.
So, I'm debating whether to apply this now or whether to add the better API first; anyone else want to chime in?
If we want this kind of functionality in virsh, then IMHO we should have an API for it. The resulting code will be much simpler than what this does in virsh. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Taku Izumi