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

On 12/06/2015 10:59 AM, Jim Fehlig wrote: > 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. Sorry for spamming with all the self-replies... > >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, If this is acceptable, the below diff should be squashed in to use the #define instead of hardcoded "vif" in the libxl driver.

Regards, Jim diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index ef92974..c5d84a4 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -734,7 +734,7 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, for (i = 0; i < vm->def->nnets; i++) { virDomainNetDefPtr net = vm->def->nets[i]; - if (STRPREFIX(net->ifname, "vif")) + if (STRPREFIX(net->ifname, VIR_NET_GENERATED_PREFIX_XEN)) VIR_FREE(net->ifname); } } @@ -918,7 +918,8 @@ libxlDomainCreateIfaceNames(virDomainDefPtr def, libxl_domain_config *d_config) if (net->ifname) continue; - ignore_value(virAsprintf(&net->ifname, "vif%d.%d%s", + ignore_value(virAsprintf(&net->ifname, + VIR_NET_GENERATED_PREFIX_XEN "%d.%d%s", def->id, x_nic->devid, suffix)); } } _______________________________________________ Xen-devel mailing list Xen-devel(a)lists.xen.org http://lists.xen.org/xen-devel