Re: [PATCH] libxl: add memory attach support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
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"),
> 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]