[libvirt] [PATCH RFC 0/2] conf: add net device prefix capability
Hey, Back in the pre 1.3.0 release [0] we had a regression when migrating domains in libxl (introduced by the reverted commit d2e5538b1). The chosen solution to address this problem was to introduce a capability to be used by the virDomainNetDefFormat() and virDomainNetDefParseXML() routines. This series implements this, and I would ask if it's in line with what you had in mind by exposing as a hypervisor capability? For the purposes of RFC and compability with other drivers I left the hardcoded checking of VIR_NET_GENERATED_PREFIX plus checking a prefix registered by the driver. Having this series and the reverted commit applied, I can successfully migrate a domain without seeing the same interface name on both source and destination node. Thoughts? Thanks! Joao [0] https://www.redhat.com/archives/libvir-list/2015-December/msg00279.html Joao Martins (2): conf: add net device prefix to capabilities libxl: set net device prefix 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/libxl/libxl_conf.c | 3 +++ src/libxl/libxl_conf.h | 4 ++++ src/network/bridge_driver.c | 2 +- 8 files changed, 51 insertions(+), 6 deletions(-) -- 2.1.4
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@oracle.com> --- 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; +} + /** * virCapabilitiesAddHostNUMACell: @@ -913,6 +930,10 @@ virCapabilitiesFormatXML(virCapsPtr caps) virBufferAddLit(&buf, "</migration_features>\n"); } + if (caps->host.netprefix) + virBufferAsprintf(&buf, "<netprefix>%s</netprefix>\n", + caps->host.netprefix); + if (caps->host.nnumaCell && virCapabilitiesFormatNUMATopology(&buf, caps->host.nnumaCell, caps->host.numaCell) < 0) diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h index 1754b13..2767f48 100644 --- a/src/conf/capabilities.h +++ b/src/conf/capabilities.h @@ -160,6 +160,7 @@ struct _virCapsHost { size_t nsecModels; virCapsHostSecModelPtr secModels; + char *netprefix; virCPUDefPtr cpu; int nPagesSize; /* size of pagesSize array */ unsigned int *pagesSize; /* page sizes support on the system */ @@ -219,6 +220,9 @@ extern int virCapabilitiesAddHostMigrateTransport(virCapsPtr caps, const char *name); +extern int +virCapabilitiesSetNetPrefix(virCapsPtr caps, + const char *prefix); extern int virCapabilitiesAddHostNUMACell(virCapsPtr caps, 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); typedef enum { diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 83f6e2c..80d6cc5 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -59,6 +59,7 @@ virCapabilitiesGetCpusForNodemask; virCapabilitiesHostSecModelAddBaseLabel; virCapabilitiesNew; virCapabilitiesSetHostCPU; +virCapabilitiesSetNetPrefix; # conf/cpu_conf.h diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 362e294..5b0a7f1 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -200,7 +200,7 @@ networkRunHook(virNetworkObjPtr network, virBufferAddLit(&buf, "<hookData>\n"); virBufferAdjustIndent(&buf, 2); - if (iface && virDomainNetDefFormat(&buf, iface, 0) < 0) + if (iface && virDomainNetDefFormat(&buf, iface, NULL, 0) < 0) goto cleanup; if (virNetworkDefFormatBuf(&buf, network->def, 0) < 0) goto cleanup; -- 2.1.4
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@oracle.com> --- 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.
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 Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
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@oracle.com> --- 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
Got it. Thanks for the review!
Regards, Daniel
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@oracle.com> --- 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
Use the newly added virCapabilitiesSetNetPrefix to set the network prefix for the driver. This in return will be use by NetDefFormat() and NetDefParseXML() routines to free any interface name that start with the registered prefix. Signed-off-by: Joao Martins <joao.m.martins@oracle.com> --- src/libxl/libxl_conf.c | 3 +++ src/libxl/libxl_conf.h | 4 ++++ 2 files changed, 7 insertions(+) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 6320421..d7fb533 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -183,6 +183,9 @@ libxlCapsInitHost(libxl_ctx *ctx, virCapsPtr caps) virCapabilitiesAddHostFeature(caps, "pae") < 0) return -1; + if (virCapabilitiesSetNetPrefix(caps, LIBXL_GENERATED_PREFIX_XEN) < 0) + return -1; + return 0; } diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h index 7c68b2b..6ad9ad3 100644 --- a/src/libxl/libxl_conf.h +++ b/src/libxl/libxl_conf.h @@ -47,6 +47,10 @@ # define LIBXL_MIGRATION_PORT_MIN 49152 # define LIBXL_MIGRATION_PORT_MAX 49216 +/* 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 LIBXL_GENERATED_PREFIX_XEN "vif" + # define LIBXL_CONFIG_BASE_DIR SYSCONFDIR "/libvirt" # define LIBXL_CONFIG_DIR SYSCONFDIR "/libvirt/libxl" # define LIBXL_AUTOSTART_DIR LIBXL_CONFIG_DIR "/autostart" -- 2.1.4
On Wed, Jan 20, 2016 at 11:41:27PM +0000, Joao Martins wrote:
Use the newly added virCapabilitiesSetNetPrefix to set the network prefix for the driver. This in return will be use by NetDefFormat() and NetDefParseXML() routines to free any interface name that start with the registered prefix.
Signed-off-by: Joao Martins <joao.m.martins@oracle.com> --- src/libxl/libxl_conf.c | 3 +++ src/libxl/libxl_conf.h | 4 ++++ 2 files changed, 7 insertions(+)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 6320421..d7fb533 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -183,6 +183,9 @@ libxlCapsInitHost(libxl_ctx *ctx, virCapsPtr caps) virCapabilitiesAddHostFeature(caps, "pae") < 0) return -1;
+ if (virCapabilitiesSetNetPrefix(caps, LIBXL_GENERATED_PREFIX_XEN) < 0) + return -1; + return 0; }
diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h index 7c68b2b..6ad9ad3 100644 --- a/src/libxl/libxl_conf.h +++ b/src/libxl/libxl_conf.h @@ -47,6 +47,10 @@ # define LIBXL_MIGRATION_PORT_MIN 49152 # define LIBXL_MIGRATION_PORT_MAX 49216
+/* 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 LIBXL_GENERATED_PREFIX_XEN "vif" + # define LIBXL_CONFIG_BASE_DIR SYSCONFDIR "/libvirt" # define LIBXL_CONFIG_DIR SYSCONFDIR "/libvirt/libxl" # define LIBXL_AUTOSTART_DIR LIBXL_CONFIG_DIR "/autostart"
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (2)
-
Daniel P. Berrange -
Joao Martins