Re: [PATCH v2] libxl: free ifname on libxlDomainMigrationPrepareDef

* Re: [PATCH v2] libxl: free ifname on libxlDomainMigrationPrepareDef
       [not found] <1449258301-7335-1-git-send-email-joao.m.martins@oracle.com>
@ 2015-12-06 17:04 ` Jim Fehlig
       [not found] ` <56646A92.7040102@suse.com>
  1 sibling, 0 replies; 4+ messages in thread
From: Jim Fehlig @ 2015-12-06 17:04 UTC (permalink / raw)
  To: Joao Martins, libvir-list; +Cc: Jiri Denemark, xen-devel

On 12/04/2015 12:45 PM, Joao Martins wrote:
> Commit d2e5538b1 changes virDomainDef to include ifnames
> that autogenerated by libxl, and that are also cleared
> on domain cleanup. One place that's missing is on
> migration, when domain xml is sent to dst libvirtd and
> would contain ifnames from the source libvirtd. This
> would lead to erronous behaviour (as seen in osstest CI)
> such as failing to migrate when a vif with the same name
> existed (belonging to another domain) on destination.

Your patch is certainly one way to fix this issue, but I wonder if we should be
adding the generated ifname to the XML that is sent to the destination.
virDomainNetDefFormat() has this interesting piece of logic

    if (def->ifname &&
        !((flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE) &&
          (STRPREFIX(def->ifname, VIR_NET_GENERATED_PREFIX)))) {
        /* Skip auto-generated target names for inactive config. */
        virBufferEscapeString(buf, "<target dev='%s'/>\n", def->ifname);
    }

In the Begin phase, we format the XML that will be sent to the destination based
on current vm->def of the machine running on the source. Calling
virDomainDefFormat() with VIR_DOMAIN_DEF_FORMAT_INACTIVE doesn't seem right. I
tried to see how this is handled in the qemu driver, but couldn't quite figure
it out. Jirka is one of the experts of the qemu migration code, adding him to cc
to see if he has any insights.

I'd like to get this resolved and a fix pushed before 1.3.0 is released since it
causes a migration regression.

Regards,
Jim

>
> This patch adds a helper libxlDomainFreeIfaceNames for
> clearing ifnames on both migration prepare phase and domain
> cleanup. It is done on prepare phase so that we don't affect
> source domain (specially in cases when the migration fails),
> while still allowing interface stats to be consulted during
> the migration.
>
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
> Changes since v1:
>  - Commit message is changed
>  - Clear ifnames on prepare phase as opposed to begin phase.
> ---
>  src/libxl/libxl_domain.c    | 32 ++++++++++++++++++++++----------
>  src/libxl/libxl_domain.h    |  3 +++
>  src/libxl/libxl_migration.c |  2 ++
>  3 files changed, 27 insertions(+), 10 deletions(-)
>
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index ef92974..487589d 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -728,16 +728,7 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
>          }
>      }
>  
> -    if ((vm->def->nnets)) {
> -        size_t i;
> -
> -        for (i = 0; i < vm->def->nnets; i++) {
> -            virDomainNetDefPtr net = vm->def->nets[i];
> -
> -            if (STRPREFIX(net->ifname, "vif"))
> -                VIR_FREE(net->ifname);
> -        }
> -    }
> +    libxlDomainFreeIfaceNames(vm->def);
>  
>      if (virAsprintf(&file, "%s/%s.xml", cfg->stateDir, vm->def->name) > 0) {
>          if (unlink(file) < 0 && errno != ENOENT && errno != ENOTDIR)
> @@ -923,6 +914,27 @@ libxlDomainCreateIfaceNames(virDomainDefPtr def, libxl_domain_config *d_config)
>      }
>  }
>  
> +/*
> + * Removes autogenerated interface names for the network devices in
> + * parameter def. User-provided interface names are skipped.
> + */
> +void
> +libxlDomainFreeIfaceNames(virDomainDefPtr def)
> +{
> +    size_t i;
> +
> +    for (i = 0; i < def->nnets; i++) {
> +        virDomainNetDefPtr net = def->nets[i];
> +
> +        if (!net->ifname)
> +            continue;
> +
> +        if (STRPREFIX(net->ifname, "vif"))
> +            VIR_FREE(net->ifname);
> +    }
> +}
> +
> +
>  
>  /*
>   * Start a domain through libxenlight.
> diff --git a/src/libxl/libxl_domain.h b/src/libxl/libxl_domain.h
> index 44b3e0b..70c139c 100644
> --- a/src/libxl/libxl_domain.h
> +++ b/src/libxl/libxl_domain.h
> @@ -135,6 +135,9 @@ int
>  libxlDomainSetVcpuAffinities(libxlDriverPrivatePtr driver,
>                               virDomainObjPtr vm);
>  
> +void
> +libxlDomainFreeIfaceNames(virDomainDefPtr def);
> +
>  int
>  libxlDomainStart(libxlDriverPrivatePtr driver,
>                   virDomainObjPtr vm,
> diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
> index 0d23e5f..1ca31ab 100644
> --- a/src/libxl/libxl_migration.c
> +++ b/src/libxl/libxl_migration.c
> @@ -288,6 +288,8 @@ libxlDomainMigrationPrepareDef(libxlDriverPrivatePtr driver,
>                                          VIR_DOMAIN_DEF_PARSE_INACTIVE)))
>          goto cleanup;
>  
> +    libxlDomainFreeIfaceNames(def);
> +
>      if (dname) {
>          name = def->name;
>          if (VIR_STRDUP(def->name, dname) < 0) {

^ permalink raw reply	[flat|nested] 4+ messages in thread