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

[Joao Martins]

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 at 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?