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

[Jim Fehlig <jfehlig@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 1678 bytes --]

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.

Regards,
Jim


[-- Attachment #2: xen-net-prefix.patch --]
[-- Type: text/x-patch, Size: 2795 bytes --]

>From 8556bf5a116edf5989eac40f0a90feed6c003b38 Mon Sep 17 00:00:00 2001
From: Jim Fehlig <jfehlig@suse.com>
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@suse.com>
---
 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,
-- 
2.1.4


[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel