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