[libvirt] [PATCH] libxl: Disallow save or migrate when host devices are assigned to a guest.

[Jim Fehlig]

Konrad Rzeszutek Wilk wrote:
> It is unhealthy. If the device is not doing any DMA operations
> it would work - but if you are saving and there are DMA operations
> happening the chance of corruption (outstanding DMAs) increase.
>
> As such re-use the check migration used.
>   

s/used// ?

> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk at oracle.com>
> ---
>  src/libxl/libxl_domain.c    | 19 +++++++++++++++++++
>  src/libxl/libxl_domain.h    |  3 +++
>  src/libxl/libxl_driver.c    |  6 ++++++
>  src/libxl/libxl_migration.c | 15 +--------------
>  4 files changed, 29 insertions(+), 14 deletions(-)
>
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index 5e0ab56..c038989 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -178,6 +178,25 @@ libxlDomainObjEndJob(libxlDriverPrivatePtr driver ATTRIBUTE_UNUSED,
>      return virObjectUnref(obj);
>  }
>  
> +/*
> + * Checks whether the domain has host devices (PCIe) to disallow
> + * migration or saving of guest - unless they are detached.
> + *
> + * Returns true if the domain has host devices, otherwise false.
> + */
> +bool
> +libxlDomainHasHostDevices(virDomainDefPtr def)
> +{
> +    /* Migration is not allowed if definition contains any hostdevs */
> +    if (def->nhostdevs > 0) {
> +        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> +                       _("domain has assigned host devices"));
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
>  static void *
>  libxlDomainObjPrivateAlloc(void)
>  {
> diff --git a/src/libxl/libxl_domain.h b/src/libxl/libxl_domain.h
> index aa647b8..76c0c8d 100644
> --- a/src/libxl/libxl_domain.h
> +++ b/src/libxl/libxl_domain.h
> @@ -95,6 +95,9 @@ char *
>  libxlDomainManagedSavePath(libxlDriverPrivatePtr driver,
>                             virDomainObjPtr vm);
>  
> +bool
> +libxlDomainHasHostDevices(virDomainDefPtr def);
> +
>  int
>  libxlDomainSaveImageOpen(libxlDriverPrivatePtr driver,
>                           libxlDriverConfigPtr cfg,
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 9eb071e..b9faba8 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -1650,6 +1650,9 @@ libxlDomainSaveFlags(virDomainPtr dom, const char *to, const char *dxml,
>          goto endjob;
>      }
>  
> +    if (libxlDomainHasHostDevices(vm->def))
> +        goto endjob;
> +
>      if (libxlDoDomainSave(driver, vm, to) < 0)
>          goto endjob;
>  
> @@ -1876,6 +1879,9 @@ libxlDomainManagedSave(virDomainPtr dom, unsigned int flags)
>          goto endjob;
>      }
>  
> +    if (libxlDomainHasHostDevices(vm->def))
> +        goto endjob;
> +
>      name = libxlDomainManagedSavePath(driver, vm);
>      if (name == NULL)
>          goto endjob;
> diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
> index 4010506..aaed448c 100644
> --- a/src/libxl/libxl_migration.c
> +++ b/src/libxl/libxl_migration.c
> @@ -209,19 +209,6 @@ libxlDoMigrateSend(libxlDriverPrivatePtr driver,
>      return ret;
>  }
>  
> -static bool
> -libxlDomainMigrationIsAllowed(virDomainDefPtr def)
> -{
> -    /* Migration is not allowed if definition contains any hostdevs */
> -    if (def->nhostdevs > 0) {
> -        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> -                       _("domain has assigned host devices"));
> -        return false;
> -    }
> -
> -    return true;
> -}
> -

This function was created with the intention of adding more checks, e.g.
is the cpu and numa configuration migratable?  Are there pending
snapshot or block jobs (once those are supported)? ...

I'd like to keep libxlDomainMigrationIsAllowed even though it currently
only checks for assigned hostdevs.  I think it improves readability of
the migration code.  For the time it could simply call
libxlDomainHasHostDevices.

Regards,
Jim