Re: [libvirt] [PATCH] libxl: add memory attach support
Hey!
On 08/30/2016 11:00 AM, Bob Liu wrote:
Support for VIR_DOMAIN_DEVICE_MEMORY on domainAttachDeviceFlags API
in libxl
driver, using libxl_set_memory_target in xen libxl.
With "virsh attach-device" command we can then hotplug memory to a domain:
<memory model='dimm'>
<target>
<size unit='MiB'>128</size>
<node>0</node>
</target>
</memory>
$ virsh attach-device domain_name this.xml --live
Signed-off-by: Bob Liu <bob.liu(a)oracle.com>
See few very minor comments
below, but overall LGTM.
---
src/libxl/libxl_domain.c | 1 +
src/libxl/libxl_driver.c | 29 +++++++++++++++++++++++++++++
2 files changed, 30 insertions(+)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index f529a2e..3924ba0 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -414,6 +414,7 @@ virDomainDefParserConfig libxlDomainDefParserConfig = {
.macPrefix = { 0x00, 0x16, 0x3e },
.devicesPostParseCallback = libxlDomainDeviceDefPostParse,
.domainPostParseCallback = libxlDomainDefPostParse,
+ .features = VIR_DOMAIN_DEF_FEATURE_MEMORY_HOTPLUG
};
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index a34eb02..541ea3b 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -3085,6 +3085,30 @@ libxlDomainAttachHostUSBDevice(libxlDriverPrivatePtr driver,
#endif
static int
+libxlDomainAttachMemory(libxlDriverPrivatePtr driver,
+ virDomainObjPtr vm,
+ virDomainMemoryDefPtr mem)
+{
+ int res = 0;
I think you don't need to initialize the variable.
+ libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
+
+ if (mem->targetNode != 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("unsupport non-zero target node for memory device"));
Probably changing the message to "non-zero target node not supported"
instead? The
messages sounds like you are removing support for it. But english is not my
mothertongue so may be it's just my wrong reading :)
Should we fail with other error as this patch, or VIR_WARN the user and ignore
targetNode. AFAIK libxl doesn't yet support ballooning for a specific node. What do
others think?
+ return -1;
+ }
+
+ res = libxl_set_memory_target(cfg->ctx, vm->def->id, mem->size, 1, 1);
Should we unlock virDomainObj while ballooning, as similarly done in
libxlDomainSetMemory* ?
+ if (res < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Failed to attach %lluKB memory for domain %d"),
+ mem->size, vm->def->id);
+ return -1;
+ }
+ return 0;
+}
+
+static int
libxlDomainAttachHostDevice(libxlDriverPrivatePtr driver,
virDomainObjPtr vm,
virDomainHostdevDefPtr hostdev)
@@ -3284,6 +3308,11 @@ libxlDomainAttachDeviceLive(libxlDriverPrivatePtr driver,
dev->data.hostdev = NULL;
break;
+ case VIR_DOMAIN_DEVICE_MEMORY:
+ ret = libxlDomainAttachMemory(driver, vm, dev->data.memory);
+ dev->data.memory = NULL;
This should probably be:
ret = libxlDomainAttachMemory(driver, vm, dev->data.memory);
if (!ret)
dev->data.memory = NULL;
Right?
+ break;
+
default:
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("device type '%s' cannot be attached"),