[libvirt] [PATCH] libxl: support interface type=network

Add support for <interface type='network'> in the libxl driver. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_conf.c | 41 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 39 insertions(+), 2 deletions(-) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index cec37d6..6efcea6 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -908,7 +908,44 @@ libxlMakeNic(virDomainDefPtr def, if (VIR_STRDUP(x_nic->script, l_nic->script) < 0) return -1; break; - default: + case VIR_DOMAIN_NET_TYPE_NETWORK: + { + bool error = true; + char *brname = NULL; + virNetworkPtr network = NULL; + virConnectPtr conn; + + if (!(conn = virConnectOpen("xen:///system"))) + return -1; + + if (!(network = + virNetworkLookupByName(conn, l_nic->data.network.name))) + goto cleanup_net; + + if (!(brname = virNetworkGetBridgeName(network))) + goto cleanup_net; + + if (VIR_STRDUP(x_nic->bridge, brname) < 0) + goto cleanup_net; + + error = false; + + cleanup_net: + VIR_FREE(brname); + virNetworkFree(network); + virObjectUnref(conn); + if (error) + return -1; + break; + } + case VIR_DOMAIN_NET_TYPE_USER: + case VIR_DOMAIN_NET_TYPE_SERVER: + case VIR_DOMAIN_NET_TYPE_CLIENT: + case VIR_DOMAIN_NET_TYPE_MCAST: + case VIR_DOMAIN_NET_TYPE_INTERNAL: + case VIR_DOMAIN_NET_TYPE_DIRECT: + case VIR_DOMAIN_NET_TYPE_HOSTDEV: + case VIR_DOMAIN_NET_TYPE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, _("libxenlight does not support network device type %s"), virDomainNetTypeToString(l_nic->type)); @@ -919,7 +956,7 @@ libxlMakeNic(virDomainDefPtr def, } static int -libxlMakeNicList(virDomainDefPtr def, libxl_domain_config *d_config) +libxlMakeNicList(virDomainDefPtr def, libxl_domain_config *d_config) { virDomainNetDefPtr *l_nics = def->nets; size_t nnics = def->nnets; -- 1.8.0.1

Jim Fehlig wrote:
Add support for <interface type='network'> in the libxl driver.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_conf.c | 41 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 39 insertions(+), 2 deletions(-)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index cec37d6..6efcea6 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -908,7 +908,44 @@ libxlMakeNic(virDomainDefPtr def, if (VIR_STRDUP(x_nic->script, l_nic->script) < 0) return -1; break; - default: + case VIR_DOMAIN_NET_TYPE_NETWORK: + { + bool error = true; + char *brname = NULL; + virNetworkPtr network = NULL; + virConnectPtr conn; + + if (!(conn = virConnectOpen("xen:///system"))) + return -1; + + if (!(network = + virNetworkLookupByName(conn, l_nic->data.network.name))) + goto cleanup_net; + + if (!(brname = virNetworkGetBridgeName(network))) + goto cleanup_net; + + if (VIR_STRDUP(x_nic->bridge, brname) < 0) + goto cleanup_net; + + error = false; + + cleanup_net: + VIR_FREE(brname); + virNetworkFree(network);
While testing this patch a bit more, noticed that freeing the virNetworkPtr here swallowed any previous errors. E.g. when specifying a non-existent network, I got the following error error: invalid network pointer in virNetworkFree With the below squashed in, I get the more reasonable error: Network not found: no network with matching name 'foobar' Regards, Jim diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 6efcea6..195bacc 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -910,31 +910,37 @@ libxlMakeNic(virDomainDefPtr def, break; case VIR_DOMAIN_NET_TYPE_NETWORK: { - bool error = true; + bool fail = false; char *brname = NULL; - virNetworkPtr network = NULL; + virNetworkPtr network; virConnectPtr conn; + virErrorPtr errobj; if (!(conn = virConnectOpen("xen:///system"))) return -1; if (!(network = - virNetworkLookupByName(conn, l_nic->data.network.name))) - goto cleanup_net; - - if (!(brname = virNetworkGetBridgeName(network))) - goto cleanup_net; - - if (VIR_STRDUP(x_nic->bridge, brname) < 0) - goto cleanup_net; + virNetworkLookupByName(conn, l_nic->data.network.name))) { + virObjectUnref(conn); + return -1; + } - error = false; + if ((brname = virNetworkGetBridgeName(network))) { + if (VIR_STRDUP(x_nic->bridge, brname) < 0) + fail = true; + } else { + fail = true; + } - cleanup_net: VIR_FREE(brname); + + /* Preserve any previous failure */ + errobj = virSaveLastError(); virNetworkFree(network); + virSetError(errobj); + virFreeError(errobj); virObjectUnref(conn); - if (error) + if (fail) return -1; break; }

On 06/07/2014 12:30 AM, Jim Fehlig wrote:
Add support for <interface type='network'> in the libxl driver.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_conf.c | 41 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 39 insertions(+), 2 deletions(-)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index cec37d6..6efcea6 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -908,7 +908,44 @@ libxlMakeNic(virDomainDefPtr def, if (VIR_STRDUP(x_nic->script, l_nic->script) < 0) return -1; break; - default: + case VIR_DOMAIN_NET_TYPE_NETWORK: + { + bool error = true; + char *brname = NULL; + virNetworkPtr network = NULL; + virConnectPtr conn; + + if (!(conn = virConnectOpen("xen:///system"))) + return -1; + + if (!(network = + virNetworkLookupByName(conn, l_nic->data.network.name))) + goto cleanup_net; + + if (!(brname = virNetworkGetBridgeName(network))) + goto cleanup_net;
This only accounts for the traditional libvirt-managed networks (the ones with <forward mode='nat|route'> or no <forward> at all, which use a transient bridge device created by libvirt). As long as you're adding this support, you may as well add it in a way that you can take advantage of the libvirt networks which are just thinly veiled coveres over existing bridges, e.g.: http://www.libvirt.org/formatnetwork.html#examplesBridge That can be done by following the example of qemunetworkIfaceConnect(). In short, instead of looking at l_nic->type, you look at virDomainNetGetActualType(l_nic), and do the above code only if *that* is VIR_DOMAIN_NET_TYPE_NETWORK. Otherwise, if actualType is VIR_DOMAIN_NET_TYPE_BRIDGE, you will want to VIR_STRDUP(x_nic->bridge, virDomainNetGetActualBridgeName(l_nic)) (note that this will end up accounting for both the case of an <interface type='bridge'> *AND* an <interface type='network'> where the network is a wrapper over a system-created bridge device). BTW, I notice that because you added the new case at the end, none of these network-based connections will use the script from the definition as is done with bridge-based connections (yet no error will be logged if one exists). Was this intentional?
+ + if (VIR_STRDUP(x_nic->bridge, brname) < 0) + goto cleanup_net; + + error = false; + + cleanup_net: + VIR_FREE(brname); + virNetworkFree(network); + virObjectUnref(conn); + if (error) + return -1; + break; + } + case VIR_DOMAIN_NET_TYPE_USER: + case VIR_DOMAIN_NET_TYPE_SERVER: + case VIR_DOMAIN_NET_TYPE_CLIENT: + case VIR_DOMAIN_NET_TYPE_MCAST: + case VIR_DOMAIN_NET_TYPE_INTERNAL: + case VIR_DOMAIN_NET_TYPE_DIRECT: + case VIR_DOMAIN_NET_TYPE_HOSTDEV: + case VIR_DOMAIN_NET_TYPE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, _("libxenlight does not support network device type %s"), virDomainNetTypeToString(l_nic->type)); @@ -919,7 +956,7 @@ libxlMakeNic(virDomainDefPtr def, }
static int -libxlMakeNicList(virDomainDefPtr def, libxl_domain_config *d_config) +libxlMakeNicList(virDomainDefPtr def, libxl_domain_config *d_config) { virDomainNetDefPtr *l_nics = def->nets; size_t nnics = def->nnets;

Laine Stump wrote:
On 06/07/2014 12:30 AM, Jim Fehlig wrote:
Add support for <interface type='network'> in the libxl driver.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_conf.c | 41 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 39 insertions(+), 2 deletions(-)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index cec37d6..6efcea6 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -908,7 +908,44 @@ libxlMakeNic(virDomainDefPtr def, if (VIR_STRDUP(x_nic->script, l_nic->script) < 0) return -1; break; - default: + case VIR_DOMAIN_NET_TYPE_NETWORK: + { + bool error = true; + char *brname = NULL; + virNetworkPtr network = NULL; + virConnectPtr conn; + + if (!(conn = virConnectOpen("xen:///system"))) + return -1; + + if (!(network = + virNetworkLookupByName(conn, l_nic->data.network.name))) + goto cleanup_net; + + if (!(brname = virNetworkGetBridgeName(network))) + goto cleanup_net;
This only accounts for the traditional libvirt-managed networks (the ones with <forward mode='nat|route'> or no <forward> at all, which use a transient bridge device created by libvirt). As long as you're adding this support, you may as well add it in a way that you can take advantage of the libvirt networks which are just thinly veiled coveres over existing bridges, e.g.:
http://www.libvirt.org/formatnetwork.html#examplesBridge
That can be done by following the example of qemunetworkIfaceConnect(). In short, instead of looking at l_nic->type, you look at virDomainNetGetActualType(l_nic), and do the above code only if *that* is VIR_DOMAIN_NET_TYPE_NETWORK. Otherwise, if actualType is VIR_DOMAIN_NET_TYPE_BRIDGE, you will want to VIR_STRDUP(x_nic->bridge, virDomainNetGetActualBridgeName(l_nic)) (note that this will end up accounting for both the case of an <interface type='bridge'> *AND* an <interface type='network'> where the network is a wrapper over a system-created bridge device).
Thanks for the pointer! I've adjusted the patch as you've suggested.
BTW, I notice that because you added the new case at the end, none of these network-based connections will use the script from the definition as is done with bridge-based connections (yet no error will be logged if one exists). Was this intentional?
Yes. We discussed supporting <script> in the libxl driver a while back and agreed to only support it for interface types 'ethernet' and 'bridge' http://www.redhat.com/archives/libvir-list/2013-April/msg00775.html I've sent a V2 with a second patch that enforces that http://www.redhat.com/archives/libvir-list/2014-June/msg00460.html And while writing this, realized I should have just sent patch 2 separately instead of creating the series. Regards, Jim
participants (2)
-
Jim Fehlig
-
Laine Stump