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@xxxxxxxx>
> 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@xxxxxxxx>
> ---
> 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));
}
}
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list