[libvirt] [PATCH v4 0/2] network_conf: check if bridge exists on host for user created bridges
The patch fixes the below problem. ============================== If the bridge name is not mentioned in the <network> xml, the bridge name is auto generated from virNetworkAllocateBridge(). If the default template named bridge is created manually by a user, the bridge start will fail with "File exists". bash-4.3$ sudo brctl addbr virbr1 bash-4.3$ brctl show bridge name bridge id STP enabled interfaces br0 8000.000000000000 no virbr0 8000.525400a91d03 yes virbr0-nic virbr1 8000.000000000000 no bash-4.3$ sudo virsh net-list --all Name State Autostart Persistent ---------------------------------------------------------- default active no yes bash-4.3$ cat /tmp/isolated # Notice that the <bridge> intentionally not given. <network> <name>isolated</name> <forward/> <ip address="192.168.123.1" netmask="255.255.255.0"> <dhcp> <range start="192.168.123.2" end="192.168.123.254"/> </dhcp> </ip> </network> bash-4.3$ sudo virsh net-create /tmp/isolated error: Failed to create network from isolated error: Unable to create bridge virbr1: File exists =============================== --- Shivaprasad G Bhat (2): cleanup conf/device_conf.h inclusion from util/virnetdev.h network_conf: check if bridge exists on host for user created bridges src/conf/device_conf.h | 38 +------------------------------------- src/conf/network_conf.c | 8 +++++++- src/util/virnetdev.h | 38 +++++++++++++++++++++++++++++++++++++- 3 files changed, 45 insertions(+), 39 deletions(-) -- Signature
Move the struct and enum definitions from device_conf.h to virnetdev.h thus avoiding the file inclusion. The wrong reference of conf files from util/ is allowed in Makefile.am for now. The reason being, The change in Makefile.am for libvirt_util_la_CFLAGS to remove conf from the utils would require corresponding inclusion in util files to specify the paths explicitly. This would result in syntax-check failures (prohibit_cross_inclusion) which need to be worked around until there are patches reworking the incorrect inclusion. The syntax-check failure workaround seems dangerous as that might mask some easily resolvable failures. So dont touch the Makefile.am for now. Resolve the wrong inclusions in separate patches. Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> --- src/conf/device_conf.h | 38 +------------------------------------- src/util/virnetdev.h | 38 +++++++++++++++++++++++++++++++++++++- 2 files changed, 38 insertions(+), 38 deletions(-) diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h index 7ea90f6..a650189 100644 --- a/src/conf/device_conf.h +++ b/src/conf/device_conf.h @@ -31,19 +31,7 @@ # include "virutil.h" # include "virthread.h" # include "virbuffer.h" - -typedef enum { - VIR_INTERFACE_STATE_UNKNOWN = 1, - VIR_INTERFACE_STATE_NOT_PRESENT, - VIR_INTERFACE_STATE_DOWN, - VIR_INTERFACE_STATE_LOWER_LAYER_DOWN, - VIR_INTERFACE_STATE_TESTING, - VIR_INTERFACE_STATE_DORMANT, - VIR_INTERFACE_STATE_UP, - VIR_INTERFACE_STATE_LAST -} virInterfaceState; - -VIR_ENUM_DECL(virInterfaceState) +# include "virnetdev.h" typedef struct _virDevicePCIAddress virDevicePCIAddress; typedef virDevicePCIAddress *virDevicePCIAddressPtr; @@ -55,30 +43,6 @@ struct _virDevicePCIAddress { int multi; /* virTristateSwitch */ }; -typedef struct _virInterfaceLink virInterfaceLink; -typedef virInterfaceLink *virInterfaceLinkPtr; -struct _virInterfaceLink { - virInterfaceState state; /* link state */ - unsigned int speed; /* link speed in Mbits per second */ -}; - -typedef enum { - VIR_NET_DEV_FEAT_GRXCSUM, - VIR_NET_DEV_FEAT_GTXCSUM, - VIR_NET_DEV_FEAT_GSG, - VIR_NET_DEV_FEAT_GTSO, - VIR_NET_DEV_FEAT_GGSO, - VIR_NET_DEV_FEAT_GGRO, - VIR_NET_DEV_FEAT_LRO, - VIR_NET_DEV_FEAT_RXVLAN, - VIR_NET_DEV_FEAT_TXVLAN, - VIR_NET_DEV_FEAT_NTUPLE, - VIR_NET_DEV_FEAT_RXHASH, - VIR_NET_DEV_FEAT_LAST -} virNetDevFeature; - -VIR_ENUM_DECL(virNetDevFeature) - int virDevicePCIAddressIsValid(virDevicePCIAddressPtr addr); int virDevicePCIAddressParseXML(xmlNodePtr node, diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h index 856127b..8aab96b 100644 --- a/src/util/virnetdev.h +++ b/src/util/virnetdev.h @@ -30,7 +30,6 @@ # include "virnetlink.h" # include "virmacaddr.h" # include "virpci.h" -# include "device_conf.h" # ifdef HAVE_STRUCT_IFREQ typedef struct ifreq virIfreq; @@ -74,6 +73,43 @@ struct _virNetDevRxFilter { } vlan; }; +typedef enum { + VIR_NET_DEV_FEAT_GRXCSUM, + VIR_NET_DEV_FEAT_GTXCSUM, + VIR_NET_DEV_FEAT_GSG, + VIR_NET_DEV_FEAT_GTSO, + VIR_NET_DEV_FEAT_GGSO, + VIR_NET_DEV_FEAT_GGRO, + VIR_NET_DEV_FEAT_LRO, + VIR_NET_DEV_FEAT_RXVLAN, + VIR_NET_DEV_FEAT_TXVLAN, + VIR_NET_DEV_FEAT_NTUPLE, + VIR_NET_DEV_FEAT_RXHASH, + VIR_NET_DEV_FEAT_LAST +} virNetDevFeature; + +VIR_ENUM_DECL(virNetDevFeature) + +typedef enum { + VIR_INTERFACE_STATE_UNKNOWN = 1, + VIR_INTERFACE_STATE_NOT_PRESENT, + VIR_INTERFACE_STATE_DOWN, + VIR_INTERFACE_STATE_LOWER_LAYER_DOWN, + VIR_INTERFACE_STATE_TESTING, + VIR_INTERFACE_STATE_DORMANT, + VIR_INTERFACE_STATE_UP, + VIR_INTERFACE_STATE_LAST +} virInterfaceState; + +VIR_ENUM_DECL(virInterfaceState) + +typedef struct _virInterfaceLink virInterfaceLink; +typedef virInterfaceLink *virInterfaceLinkPtr; +struct _virInterfaceLink { + virInterfaceState state; /* link state */ + unsigned int speed; /* link speed in Mbits per second */ +}; + int virNetDevSetupControl(const char *ifname, virIfreq *ifr) ATTRIBUTE_RETURN_CHECK;
On Wed, Mar 25, 2015 at 07:33:41PM +0530, Shivaprasad G Bhat wrote:
Move the struct and enum definitions from device_conf.h to virnetdev.h thus avoiding the file inclusion.
The wrong reference of conf files from util/ is allowed in Makefile.am for now. The reason being, The change in Makefile.am for libvirt_util_la_CFLAGS to remove conf from the utils would require corresponding inclusion in util files to specify the paths explicitly. This would result in syntax-check failures (prohibit_cross_inclusion) which need to be worked around until there are patches reworking the incorrect inclusion.
The syntax-check failure workaround seems dangerous as that might mask some easily resolvable failures. So dont touch the Makefile.am for now. Resolve the wrong inclusions in separate patches.
Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> --- src/conf/device_conf.h | 38 +------------------------------------- src/util/virnetdev.h | 38 +++++++++++++++++++++++++++++++++++++- 2 files changed, 38 insertions(+), 38 deletions(-)
I like the cleanup, but it fails to build for me: In file included from ./util/virnetdev.h:30:0, from conf/device_conf.h:34, from conf/domain_conf.h:48, from conf/domain_capabilities.h:27, from conf/domain_capabilities.c:25: ./util/virnetlink.h:33:27: fatal error: netlink/msg.h: No such file or directory # include <netlink/msg.h> ^ compilation terminated. Makefile:7320: recipe for target 'conf/libvirt_conf_la-domain_capabilities.lo' failed It seems a change in the makefile will be necessary if we want to include virnetdev in conf/ Jan
diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h index 7ea90f6..a650189 100644 --- a/src/conf/device_conf.h +++ b/src/conf/device_conf.h @@ -31,19 +31,7 @@ # include "virutil.h" # include "virthread.h" # include "virbuffer.h" - -typedef enum { - VIR_INTERFACE_STATE_UNKNOWN = 1, - VIR_INTERFACE_STATE_NOT_PRESENT, - VIR_INTERFACE_STATE_DOWN, - VIR_INTERFACE_STATE_LOWER_LAYER_DOWN, - VIR_INTERFACE_STATE_TESTING, - VIR_INTERFACE_STATE_DORMANT, - VIR_INTERFACE_STATE_UP, - VIR_INTERFACE_STATE_LAST -} virInterfaceState; - -VIR_ENUM_DECL(virInterfaceState) +# include "virnetdev.h"
typedef struct _virDevicePCIAddress virDevicePCIAddress; typedef virDevicePCIAddress *virDevicePCIAddressPtr; @@ -55,30 +43,6 @@ struct _virDevicePCIAddress { int multi; /* virTristateSwitch */ };
-typedef struct _virInterfaceLink virInterfaceLink; -typedef virInterfaceLink *virInterfaceLinkPtr; -struct _virInterfaceLink { - virInterfaceState state; /* link state */ - unsigned int speed; /* link speed in Mbits per second */ -}; - -typedef enum { - VIR_NET_DEV_FEAT_GRXCSUM, - VIR_NET_DEV_FEAT_GTXCSUM, - VIR_NET_DEV_FEAT_GSG, - VIR_NET_DEV_FEAT_GTSO, - VIR_NET_DEV_FEAT_GGSO, - VIR_NET_DEV_FEAT_GGRO, - VIR_NET_DEV_FEAT_LRO, - VIR_NET_DEV_FEAT_RXVLAN, - VIR_NET_DEV_FEAT_TXVLAN, - VIR_NET_DEV_FEAT_NTUPLE, - VIR_NET_DEV_FEAT_RXHASH, - VIR_NET_DEV_FEAT_LAST -} virNetDevFeature; - -VIR_ENUM_DECL(virNetDevFeature) - int virDevicePCIAddressIsValid(virDevicePCIAddressPtr addr);
int virDevicePCIAddressParseXML(xmlNodePtr node, diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h index 856127b..8aab96b 100644 --- a/src/util/virnetdev.h +++ b/src/util/virnetdev.h @@ -30,7 +30,6 @@ # include "virnetlink.h" # include "virmacaddr.h" # include "virpci.h" -# include "device_conf.h"
# ifdef HAVE_STRUCT_IFREQ typedef struct ifreq virIfreq; @@ -74,6 +73,43 @@ struct _virNetDevRxFilter { } vlan; };
+typedef enum { + VIR_NET_DEV_FEAT_GRXCSUM, + VIR_NET_DEV_FEAT_GTXCSUM, + VIR_NET_DEV_FEAT_GSG, + VIR_NET_DEV_FEAT_GTSO, + VIR_NET_DEV_FEAT_GGSO, + VIR_NET_DEV_FEAT_GGRO, + VIR_NET_DEV_FEAT_LRO, + VIR_NET_DEV_FEAT_RXVLAN, + VIR_NET_DEV_FEAT_TXVLAN, + VIR_NET_DEV_FEAT_NTUPLE, + VIR_NET_DEV_FEAT_RXHASH, + VIR_NET_DEV_FEAT_LAST +} virNetDevFeature; + +VIR_ENUM_DECL(virNetDevFeature) + +typedef enum { + VIR_INTERFACE_STATE_UNKNOWN = 1, + VIR_INTERFACE_STATE_NOT_PRESENT, + VIR_INTERFACE_STATE_DOWN, + VIR_INTERFACE_STATE_LOWER_LAYER_DOWN, + VIR_INTERFACE_STATE_TESTING, + VIR_INTERFACE_STATE_DORMANT, + VIR_INTERFACE_STATE_UP, + VIR_INTERFACE_STATE_LAST +} virInterfaceState; + +VIR_ENUM_DECL(virInterfaceState) + +typedef struct _virInterfaceLink virInterfaceLink; +typedef virInterfaceLink *virInterfaceLinkPtr; +struct _virInterfaceLink { + virInterfaceState state; /* link state */ + unsigned int speed; /* link speed in Mbits per second */ +}; + int virNetDevSetupControl(const char *ifname, virIfreq *ifr) ATTRIBUTE_RETURN_CHECK;
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Thanks Jan.. On Thu, Mar 26, 2015 at 12:14 AM, Ján Tomko <jtomko@redhat.com> wrote:
On Wed, Mar 25, 2015 at 07:33:41PM +0530, Shivaprasad G Bhat wrote:
Move the struct and enum definitions from device_conf.h to virnetdev.h thus avoiding the file inclusion.
The wrong reference of conf files from util/ is allowed in Makefile.am for now. The reason being, The change in Makefile.am for libvirt_util_la_CFLAGS to remove conf from the utils would require corresponding inclusion in util files to specify the paths explicitly. This would result in syntax-check failures (prohibit_cross_inclusion) which need to be worked around until there are patches reworking the incorrect inclusion.
The syntax-check failure workaround seems dangerous as that might mask some easily resolvable failures. So dont touch the Makefile.am for now. Resolve the wrong inclusions in separate patches.
Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> --- src/conf/device_conf.h | 38 +------------------------------------- src/util/virnetdev.h | 38 +++++++++++++++++++++++++++++++++++++- 2 files changed, 38 insertions(+), 38 deletions(-)
I like the cleanup, but it fails to build for me: In file included from ./util/virnetdev.h:30:0, from conf/device_conf.h:34, from conf/domain_conf.h:48, from conf/domain_capabilities.h:27, from conf/domain_capabilities.c:25: ./util/virnetlink.h:33:27: fatal error: netlink/msg.h: No such file or directory # include <netlink/msg.h> ^ compilation terminated. Makefile:7320: recipe for target 'conf/libvirt_conf_la-domain_capabilities.lo' failed
It seems a change in the makefile will be necessary if we want to include virnetdev in conf/
It seems, the compilation fails only if the libnl-devel is not installed but libnl3-devel is installed. The /usr/include/netlink/msg.h part of libnl-devel helps the compilation with default include paths. Without that package it fails. Adding LIBNL_CFLAGS to AM_CFLAGS seems to fix it. Let me post the working changes in next version. Regards, Shiva
Jan
diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h index 7ea90f6..a650189 100644 --- a/src/conf/device_conf.h +++ b/src/conf/device_conf.h @@ -31,19 +31,7 @@ # include "virutil.h" # include "virthread.h" # include "virbuffer.h" - -typedef enum { - VIR_INTERFACE_STATE_UNKNOWN = 1, - VIR_INTERFACE_STATE_NOT_PRESENT, - VIR_INTERFACE_STATE_DOWN, - VIR_INTERFACE_STATE_LOWER_LAYER_DOWN, - VIR_INTERFACE_STATE_TESTING, - VIR_INTERFACE_STATE_DORMANT, - VIR_INTERFACE_STATE_UP, - VIR_INTERFACE_STATE_LAST -} virInterfaceState; - -VIR_ENUM_DECL(virInterfaceState) +# include "virnetdev.h"
typedef struct _virDevicePCIAddress virDevicePCIAddress; typedef virDevicePCIAddress *virDevicePCIAddressPtr; @@ -55,30 +43,6 @@ struct _virDevicePCIAddress { int multi; /* virTristateSwitch */ };
-typedef struct _virInterfaceLink virInterfaceLink; -typedef virInterfaceLink *virInterfaceLinkPtr; -struct _virInterfaceLink { - virInterfaceState state; /* link state */ - unsigned int speed; /* link speed in Mbits per second */ -}; - -typedef enum { - VIR_NET_DEV_FEAT_GRXCSUM, - VIR_NET_DEV_FEAT_GTXCSUM, - VIR_NET_DEV_FEAT_GSG, - VIR_NET_DEV_FEAT_GTSO, - VIR_NET_DEV_FEAT_GGSO, - VIR_NET_DEV_FEAT_GGRO, - VIR_NET_DEV_FEAT_LRO, - VIR_NET_DEV_FEAT_RXVLAN, - VIR_NET_DEV_FEAT_TXVLAN, - VIR_NET_DEV_FEAT_NTUPLE, - VIR_NET_DEV_FEAT_RXHASH, - VIR_NET_DEV_FEAT_LAST -} virNetDevFeature; - -VIR_ENUM_DECL(virNetDevFeature) - int virDevicePCIAddressIsValid(virDevicePCIAddressPtr addr);
int virDevicePCIAddressParseXML(xmlNodePtr node, diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h index 856127b..8aab96b 100644 --- a/src/util/virnetdev.h +++ b/src/util/virnetdev.h @@ -30,7 +30,6 @@ # include "virnetlink.h" # include "virmacaddr.h" # include "virpci.h" -# include "device_conf.h"
# ifdef HAVE_STRUCT_IFREQ typedef struct ifreq virIfreq; @@ -74,6 +73,43 @@ struct _virNetDevRxFilter { } vlan; };
+typedef enum { + VIR_NET_DEV_FEAT_GRXCSUM, + VIR_NET_DEV_FEAT_GTXCSUM, + VIR_NET_DEV_FEAT_GSG, + VIR_NET_DEV_FEAT_GTSO, + VIR_NET_DEV_FEAT_GGSO, + VIR_NET_DEV_FEAT_GGRO, + VIR_NET_DEV_FEAT_LRO, + VIR_NET_DEV_FEAT_RXVLAN, + VIR_NET_DEV_FEAT_TXVLAN, + VIR_NET_DEV_FEAT_NTUPLE, + VIR_NET_DEV_FEAT_RXHASH, + VIR_NET_DEV_FEAT_LAST +} virNetDevFeature; + +VIR_ENUM_DECL(virNetDevFeature) + +typedef enum { + VIR_INTERFACE_STATE_UNKNOWN = 1, + VIR_INTERFACE_STATE_NOT_PRESENT, + VIR_INTERFACE_STATE_DOWN, + VIR_INTERFACE_STATE_LOWER_LAYER_DOWN, + VIR_INTERFACE_STATE_TESTING, + VIR_INTERFACE_STATE_DORMANT, + VIR_INTERFACE_STATE_UP, + VIR_INTERFACE_STATE_LAST +} virInterfaceState; + +VIR_ENUM_DECL(virInterfaceState) + +typedef struct _virInterfaceLink virInterfaceLink; +typedef virInterfaceLink *virInterfaceLinkPtr; +struct _virInterfaceLink { + virInterfaceState state; /* link state */ + unsigned int speed; /* link speed in Mbits per second */ +}; + int virNetDevSetupControl(const char *ifname, virIfreq *ifr) ATTRIBUTE_RETURN_CHECK;
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
virNetworkBridgeInUse() doesn't check if the bridge is manually created outside of libvirt. Check if the bridge actually exist on host using the virNetDevExists(). Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> --- src/conf/network_conf.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index b334b64..8e9f3ac 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -3294,6 +3294,7 @@ int virNetworkBridgeInUse(virNetworkObjListPtr nets, const char *bridge, const char *skipname) { + int ret; virNetworkObjPtr obj; struct virNetworkBridgeInUseHelperData data = {bridge, skipname}; @@ -3301,7 +3302,12 @@ int virNetworkBridgeInUse(virNetworkObjListPtr nets, obj = virHashSearch(nets->objs, virNetworkBridgeInUseHelper, &data); virObjectUnlock(nets); - return obj != NULL; + if (obj) + ret = 1; + else /* Bridge might have been created by a user manually outside libvirt */ + ret = virNetDevExists(bridge) ? 1 : 0; + + return ret; } char *virNetworkAllocateBridge(virNetworkObjListPtr nets,
On Wed, Mar 25, 2015 at 07:35:12PM +0530, Shivaprasad G Bhat wrote:
virNetworkBridgeInUse() doesn't check if the bridge is manually created outside of libvirt. Check if the bridge actually exist on host using the virNetDevExists().
Signed-off-by: Shivaprasad G Bhat <sbhat@linux.vnet.ibm.com> --- src/conf/network_conf.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index b334b64..8e9f3ac 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -3294,6 +3294,7 @@ int virNetworkBridgeInUse(virNetworkObjListPtr nets, const char *bridge, const char *skipname) { + int ret; virNetworkObjPtr obj; struct virNetworkBridgeInUseHelperData data = {bridge, skipname};
@@ -3301,7 +3302,12 @@ int virNetworkBridgeInUse(virNetworkObjListPtr nets, obj = virHashSearch(nets->objs, virNetworkBridgeInUseHelper, &data); virObjectUnlock(nets);
- return obj != NULL; + if (obj) + ret = 1; + else /* Bridge might have been created by a user manually outside libvirt */ + ret = virNetDevExists(bridge) ? 1 : 0;
I don't think we should parse the network differently based on the host state. The existing check only looks at existing configs. Can this be checked in networkValidate instead? Jan
+ + return ret; }
char *virNetworkAllocateBridge(virNetworkObjListPtr nets,
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (3)
-
Ján Tomko -
Shivaprasad bhat -
Shivaprasad G Bhat