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

2014-06-10 Thread Jim Fehlig 
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 jfeh...@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

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

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

2014-06-09 Thread Laine Stump 
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 jfeh...@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;

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

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

2014-06-06 Thread Jim Fehlig 
Add support for interface type='network' in the libxl driver.

Signed-off-by: Jim Fehlig jfeh...@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

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

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

2014-06-06 Thread Jim Fehlig 
Jim Fehlig wrote:
 Add support for interface type='network' in the libxl driver.

 Signed-off-by: Jim Fehlig jfeh...@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;
 }

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