[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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




On 12/06/2015 06:11 PM, Jim Fehlig wrote:
> 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.
> 
Wasn't aware that virDomainNetDefParseXML took care of removing ifnames already
in Prepare phase. If so, your solution appears to be a better approach indeed.

Regards,
Joao

> 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@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel
> 

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.