Re: [libvirt] [PATCH] libxl: add memory attach support

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(a)oracle.com&gt; > 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.