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@xxxxxxxxxx>
>> 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?
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list