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

On 12/06/2015 10:04 AM, Jim Fehlig wrote: > 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 think this is actually handled on the destination when parsing the incoming XML in the Prepare phase. Something like the below patch may be a better solution.

>From 8556bf5a116edf5989eac40f0a90feed6c003b38 Mon Sep 17 00:00:00 2001 From: Jim Fehlig <jfehlig(a)suse.com&gt; Date: Sun, 6 Dec 2015 10:46:28 -0700 Subject: [PATCH] conf: add net device prefix for Xen Commit d2e5538b1 changes virDomainDef to include ifnames autogenerated by libxl. When migrating a domain, the autogenerated name is included in the XML sent to the destination host. 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. This patch defines another VIR_NET_GENERATED_PREFIX for Xen allowing the autogenerated names to be excluded when parsing and formatting inactive config. Signed-off-by: Jim Fehlig <jfehlig(a)suse.com&gt; --- src/conf/domain_conf.c | 6 ++++-- src/conf/domain_conf.h | 4 ++++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2f5c0ed..debcf4e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8428,7 +8428,8 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, ifname = virXMLPropString(cur, "dev"); if (ifname && (flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) && - STRPREFIX(ifname, VIR_NET_GENERATED_PREFIX)) { + (STRPREFIX(ifname, VIR_NET_GENERATED_PREFIX) || + STRPREFIX(ifname, VIR_NET_GENERATED_PREFIX_XEN))) { /* An auto-generated target name, blank it out */ VIR_FREE(ifname); } @@ -19790,7 +19791,8 @@ virDomainNetDefFormat(virBufferPtr buf, virBufferEscapeString(buf, "<backenddomain name='%s'/>\n", def->domain_name); if (def->ifname && !((flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE) && - (STRPREFIX(def->ifname, VIR_NET_GENERATED_PREFIX)))) { + (STRPREFIX(def->ifname, VIR_NET_GENERATED_PREFIX) || + STRPREFIX(def->ifname, VIR_NET_GENERATED_PREFIX_XEN)))) { /* Skip auto-generated target names for inactive config. */ virBufferEscapeString(buf, "<target dev='%s'/>\n", def->ifname); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 90d8e13..d2cc26f 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1085,6 +1085,10 @@ struct _virDomainNetDef { * by libvirt, and cannot be used for a persistent network name. */ # define VIR_NET_GENERATED_PREFIX "vnet" +/* Used for prefix of ifname of any network name generated dynamically + * by libvirt for Xen, and cannot be used for a persistent network name. */ +# define VIR_NET_GENERATED_PREFIX_XEN "vif" + typedef enum { VIR_DOMAIN_CHR_DEVICE_STATE_DEFAULT = 0, VIR_DOMAIN_CHR_DEVICE_STATE_CONNECTED,