Re: [PATCH RFC 1/2] conf: add net device prefix to capabilities

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

 




On 01/22/2016 03:46 PM, Joao Martins wrote:
> 
> 
> On 01/22/2016 02:50 PM, Daniel P. Berrange wrote:
>> On Wed, Jan 20, 2016 at 11:41:26PM +0000, Joao Martins wrote:
>>> In the reverted commit d2e5538b1, the libxl driver was changed to copy
>>> interface names autogenerated by libxl to the corresponding network def
>>> in the domain's virDomainDef object. The copied name is freed when the
>>> domain transitions to the shutoff state. But when migrating a domain,
>>> the autogenerated name is included in the XML sent to the destination
>>> host.  It is possible an interface with the same name already exists on
>>> the destination host, causing migration to fail.
>>>
>>> This patch defines a new capability for setting the network device
>>> prefix that will be used in the driver. This prefix will be then copied
>>> when domain is created as def->netprefix, which will be then used by
>>> virDomainNetDefFormat(). Valid prefixes are VIR_NET_GENERATED_PREFIX or
>>> the one announced by the driver.
>>>
>>> Signed-off-by: Joao Martins <joao.m.martins@xxxxxxxxxx>
>>> ---
>>>  src/conf/capabilities.c     | 21 +++++++++++++++++++++
>>>  src/conf/capabilities.h     |  4 ++++
>>>  src/conf/domain_conf.c      | 20 +++++++++++++++-----
>>>  src/conf/domain_conf.h      |  2 ++
>>>  src/libvirt_private.syms    |  1 +
>>>  src/network/bridge_driver.c |  2 +-
>>>  6 files changed, 44 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c
>>> index 86ea212..eafa7a9 100644
>>> --- a/src/conf/capabilities.c
>>> +++ b/src/conf/capabilities.c
>>> @@ -269,6 +269,23 @@ virCapabilitiesAddHostMigrateTransport(virCapsPtr caps,
>>>      return 0;
>>>  }
>>>  
>>> +/**
>>> + * virCapabilitiesSetNetPrefix:
>>> + * @caps: capabilities to extend
>>> + * @name: prefix for host generated network interfaces
>>> + *
>>> + * Registers the prefix that is used for generated network interfaces
>>> + */
>>> +int
>>> +virCapabilitiesSetNetPrefix(virCapsPtr caps,
>>> +                            const char *prefix)
>>> +{
>>> +    if (VIR_STRDUP(caps->host.netprefix, prefix) < 0)
>>> +        return -1;
>>> +
>>> +    return 0;
>>> +}
>>
>> You'll need to update virCapabilitiesDispose to free this string
>> too.
>>
> OK.
> 
>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>> index a9706b0..70d5556 100644
>>> --- a/src/conf/domain_conf.c
>>> +++ b/src/conf/domain_conf.c
>>> @@ -8403,6 +8403,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>>>                          xmlNodePtr node,
>>>                          xmlXPathContextPtr ctxt,
>>>                          virHashTablePtr bootHash,
>>> +                        char *prefix,
>>>                          unsigned int flags)
>>>  {
>>>      virDomainNetDefPtr def;
>>> @@ -8569,7 +8570,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) ||
>>> +                      (prefix && STRPREFIX(ifname, prefix)))) {
>>>                      /* An auto-generated target name, blank it out */
>>>                      VIR_FREE(ifname);
>>>                  }
>>> @@ -12525,6 +12527,7 @@ virDomainDeviceDefParse(const char *xmlStr,
>>>      xmlNodePtr node;
>>>      xmlXPathContextPtr ctxt = NULL;
>>>      virDomainDeviceDefPtr dev = NULL;
>>> +    char *prefix;
>>>  
>>>      if (!(xml = virXMLParseStringCtxt(xmlStr, _("(device_definition)"), &ctxt)))
>>>          goto error;
>>> @@ -12567,8 +12570,9 @@ virDomainDeviceDefParse(const char *xmlStr,
>>>              goto error;
>>>          break;
>>>      case VIR_DOMAIN_DEVICE_NET:
>>> +        prefix = caps->host.netprefix;
>>>          if (!(dev->data.net = virDomainNetDefParseXML(xmlopt, node, ctxt,
>>> -                                                      NULL, flags)))
>>> +                                                      NULL, prefix, flags)))
>>>              goto error;
>>>          break;
>>>      case VIR_DOMAIN_DEVICE_INPUT:
>>> @@ -15885,11 +15889,15 @@ virDomainDefParseXML(xmlDocPtr xml,
>>>          goto error;
>>>      if (n && VIR_ALLOC_N(def->nets, n) < 0)
>>>          goto error;
>>> +    if (caps->host.netprefix &&
>>> +        VIR_STRDUP(def->netprefix, caps->host.netprefix) < 0)
>>> +        goto error;
>>>      for (i = 0; i < n; i++) {
>>>          virDomainNetDefPtr net = virDomainNetDefParseXML(xmlopt,
>>>                                                           nodes[i],
>>>                                                           ctxt,
>>>                                                           bootHash,
>>> +                                                         def->netprefix,
>>>                                                           flags);
>>>          if (!net)
>>>              goto error;
>>> @@ -19880,6 +19888,7 @@ virDomainVirtioNetDriverFormat(char **outstr,
>>>  int
>>>  virDomainNetDefFormat(virBufferPtr buf,
>>>                        virDomainNetDefPtr def,
>>> +                      char *prefix,
>>>                        unsigned int flags)
>>>  {
>>>      unsigned int actualType = virDomainNetGetActualType(def);
>>> @@ -20057,7 +20066,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) ||
>>> +           (prefix && STRPREFIX(def->ifname, prefix))))) {
>>>          /* Skip auto-generated target names for inactive config. */
>>>          virBufferEscapeString(buf, "<target dev='%s'/>\n", def->ifname);
>>>      }
>>> @@ -22310,7 +22320,7 @@ virDomainDefFormatInternal(virDomainDefPtr def,
>>>              goto error;
>>>  
>>>      for (n = 0; n < def->nnets; n++)
>>> -        if (virDomainNetDefFormat(buf, def->nets[n], flags) < 0)
>>> +        if (virDomainNetDefFormat(buf, def->nets[n], def->netprefix, flags) < 0)
>>>              goto error;
>>>  
>>>      for (n = 0; n < def->nsmartcards; n++)
>>> @@ -23535,7 +23545,7 @@ virDomainDeviceDefCopy(virDomainDeviceDefPtr src,
>>>          rc = virDomainFSDefFormat(&buf, src->data.fs, flags);
>>>          break;
>>>      case VIR_DOMAIN_DEVICE_NET:
>>> -        rc = virDomainNetDefFormat(&buf, src->data.net, flags);
>>> +        rc = virDomainNetDefFormat(&buf, src->data.net, def->netprefix, flags);
>>>          break;
>>>      case VIR_DOMAIN_DEVICE_INPUT:
>>>          rc = virDomainInputDefFormat(&buf, src->data.input, flags);
>>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>>> index 0141009..9e085a7 100644
>>> --- a/src/conf/domain_conf.h
>>> +++ b/src/conf/domain_conf.h
>>> @@ -2254,6 +2254,7 @@ struct _virDomainDef {
>>>  
>>>      virDomainOSDef os;
>>>      char *emulator;
>>> +    char *netprefix;
>>>      /* These three options are of type virTristateSwitch,
>>>       * except VIR_DOMAIN_FEATURE_CAPABILITIES that is of type
>>>       * virDomainCapabilitiesPolicy */
>>> @@ -2748,6 +2749,7 @@ int virDomainDiskSourceFormat(virBufferPtr buf,
>>>  
>>>  int virDomainNetDefFormat(virBufferPtr buf,
>>>                            virDomainNetDefPtr def,
>>> +                          char *prefix,
>>>                            unsigned int flags);
>>
>> It will result in a ripple effect across the drivers, but rather than
>> storing 'netprefix' in virDomainDef, I think we should probably change
>> virDomainDefFormat to pass in the virCapsPtr object, so we can directly
>> access the canonical source for this data.
>>
>> It is probably best to add virCapsPtr to virDomainDefFormat as a separate
>> patch to this one to keep it clean
>>
Iit looks like the ripple effect is rather big: there are a couple of other
functions in which this requires interfaces to be changed such as
virDomainSaveConfig, virSnapshotDefFormat. Caching netprefix in the virDomainDef
makes the changes significantly smaller, which makes it a bit more attractive
(but perhaps less correct?). I have submitted it as "RFC v2" [0], and kept
driver changes together in the relevant patches to help bisectability.

Thanks!

[0] https://www.redhat.com/archives/libvir-list/2016-February/msg00187.html

> Got it.
> 
> Thanks for the review!
> 
>> Regards,
>> Daniel
>>

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]