On 08/31/2016 08:56 AM, Bob Liu wrote:
On 08/30/2016 11:20 PM, Joao Martins wrote:
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@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
Agree to use VIR_WARN(), will be updated.
Ah sorry, I forgot to add a "?" in this sentence, and I am not sure what would the correct behavior here. I think the current is correct, and I assume user wouldn't try to memory balloon to a node other than 0 as it can't create a guest with multiple nodes either. So probably it's reasonable to left it as is, unless someone raises a flag to loose the restriction?