[libvirt] [PATCH v3 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 | 15 ++++++++++++--- src/util/virnetdev.h | 38 +++++++++++++++++++++++++++++++++++++- 3 files changed, 50 insertions(+), 41 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..daba0bb 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; @@ -47,6 +46,43 @@ typedef enum { } virNetDevRxFilterMode; VIR_ENUM_DECL(virNetDevRxFilterMode) +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 */ +}; + +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 struct _virNetDevRxFilter virNetDevRxFilter; typedef virNetDevRxFilter *virNetDevRxFilterPtr; struct _virNetDevRxFilter {
On 03/24/2015 05:57 AM, 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.
I agree that we should eliminate this #include of something from conf in util (and definitely that is a more egregious problem than calling a platform specific function from conf), but disagree with using this patch as the method of assuring that virNetDevExists() is declared for network_conf.c (so that patch 2/2 of this series compiles). if virnetdev.h is required by something in network_conf.c, then it should be #included by network_conf.c. (yes, I understand that the code movement in this patch makes it necessary to #include virnetdev.h in device_conf.h anyway) I see that there are a few other cases of *_conf.h files being included from .h files in the util directory. Is this patch perhaps part of a patch series to fix that? (BTW, the util directory was clean of any #includes from the conf directory until last July, when management of "close callbacks" was moved from the qemu driver into util/virclosecallbacks.h. The problem is that one of the args to the a callback is a virDomainObj. So I think the proper solution to this problem would require making that arg to all the virCloseCallback functions an opaque pointer, show real nature would be known only to the callback function itself.)
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.
Maybe that's what you're referring to here?
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..daba0bb 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; @@ -47,6 +46,43 @@ typedef enum { } virNetDevRxFilterMode; VIR_ENUM_DECL(virNetDevRxFilterMode)
+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 */ +}; + +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) +
It's a bit odd that you've moved this unrelated stuff in between virNetDevRxFilterMode and virNetDevRxFilter. Those should probably be left together.
typedef struct _virNetDevRxFilter virNetDevRxFilter; typedef virNetDevRxFilter *virNetDevRxFilterPtr; struct _virNetDevRxFilter {
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Hi Laine, Thanks for the comments. Please find my reply inline. On Tue, Mar 24, 2015 at 10:03 PM, Laine Stump <laine@laine.org> wrote:
On 03/24/2015 05:57 AM, 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.
I agree that we should eliminate this #include of something from conf in util (and definitely that is a more egregious problem than calling a platform specific function from conf), but disagree with using this patch as the method of assuring that virNetDevExists() is declared for network_conf.c (so that patch 2/2 of this series compiles).
if virnetdev.h is required by something in network_conf.c, then it should be #included by network_conf.c. (yes, I understand that the code movement in this patch makes it necessary to #include virnetdev.h in device_conf.h anyway)
I agree with you. The virInterfaceState and virInterfaceState being the only virInterface* definitions and having no functions starting with virInterface*, I decided to move to virnetdev.h file, given that is the only utils/*.c/h file using them. That was the reason behind choosing this file.
I see that there are a few other cases of *_conf.h files being included from .h files in the util directory. Is this patch perhaps part of a patch series to fix that?
Yes. This patch is supposed to be the part of such a series. I am still working on them. Need some more time posting them.
(BTW, the util directory was clean of any #includes from the conf directory until last July, when management of "close callbacks" was moved from the qemu driver into util/virclosecallbacks.h. The problem is that one of the args to the a callback is a virDomainObj. So I think the proper solution to this problem would require making that arg to all the virCloseCallback functions an opaque pointer, show real nature would be known only to the callback function itself.)
I was thinking for changing the virDomainObj to virObject instead, as per Dan's suggestion in IRC. That is still work a in progress.
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.
Maybe that's what you're referring to here?
Yes. I was referring to that.
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..daba0bb 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; @@ -47,6 +46,43 @@ typedef enum { } virNetDevRxFilterMode; VIR_ENUM_DECL(virNetDevRxFilterMode)
+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 */ +}; + +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) +
It's a bit odd that you've moved this unrelated stuff in between virNetDevRxFilterMode and virNetDevRxFilter. Those should probably be left together.
I'll move them downwards to make it clean.
typedef struct _virNetDevRxFilter virNetDevRxFilter; typedef virNetDevRxFilter *virNetDevRxFilterPtr; struct _virNetDevRxFilter {
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
I am sending v2 with the code movement as suggested. Thanks, Shiva
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 | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index d7c5dec..c3ae2e4 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -3227,13 +3227,22 @@ virNetworkBridgeInUseHelper(const void *payload, int ret = 0; virNetworkObjPtr net = (virNetworkObjPtr) payload; const struct virNetworkBridgeInUseHelperData *data = opaque; + bool defined_bridge = false; virObjectLock(net); if (net->def->bridge && - STREQ(net->def->bridge, data->bridge) && - !(data->skipname && STREQ(net->def->name, data->skipname))) - ret = 1; + STREQ(net->def->bridge, data->bridge)) { + defined_bridge = true; + if (!(data->skipname && STREQ(net->def->name, data->skipname))) + ret = 1; + } + virObjectUnlock(net); + + /* Bridge might have been created by a user manually outside libvirt */ + if (!defined_bridge && !ret) + ret = virNetDevExists(data->bridge) ? 1 : 0; + return ret; }
On 03/24/2015 05:59 AM, 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 | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index d7c5dec..c3ae2e4 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -3227,13 +3227,22 @@ virNetworkBridgeInUseHelper(const void *payload, int ret = 0; virNetworkObjPtr net = (virNetworkObjPtr) payload; const struct virNetworkBridgeInUseHelperData *data = opaque; + bool defined_bridge = false;
virObjectLock(net); if (net->def->bridge && - STREQ(net->def->bridge, data->bridge) && - !(data->skipname && STREQ(net->def->name, data->skipname))) - ret = 1; + STREQ(net->def->bridge, data->bridge)) { + defined_bridge = true; + if (!(data->skipname && STREQ(net->def->name, data->skipname))) + ret = 1; + } + virObjectUnlock(net); + + /* Bridge might have been created by a user manually outside libvirt */ + if (!defined_bridge && !ret) + ret = virNetDevExists(data->bridge) ? 1 : 0; + return ret; }
This function is intended to be called once for each defined network on the host, with data->bridge being the same each time, but net->def->bridge being different, so adding the check for data->bridge existence here may work, but it's a bit convoluted. Instead, you should leave this function alone, and just add the extra check directly in the function virNetworkBridgeInUse() (either before locking, or after unlocking "nets").
On 03/24/2015 11:12 AM, Laine Stump 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 | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index d7c5dec..c3ae2e4 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -3227,13 +3227,22 @@ virNetworkBridgeInUseHelper(const void *payload, int ret = 0; virNetworkObjPtr net = (virNetworkObjPtr) payload; const struct virNetworkBridgeInUseHelperData *data = opaque; + bool defined_bridge = false;
virObjectLock(net); if (net->def->bridge && - STREQ(net->def->bridge, data->bridge) && - !(data->skipname && STREQ(net->def->name, data->skipname))) - ret = 1; + STREQ(net->def->bridge, data->bridge)) { + defined_bridge = true; + if (!(data->skipname && STREQ(net->def->name, data->skipname))) + ret = 1; + } + virObjectUnlock(net); + + /* Bridge might have been created by a user manually outside libvirt */ + if (!defined_bridge && !ret) + ret = virNetDevExists(data->bridge) ? 1 : 0; + return ret; } This function is intended to be called once for each defined network on
On 03/24/2015 05:59 AM, Shivaprasad G Bhat wrote: the host, with data->bridge being the same each time, but net->def->bridge being different, so adding the check for data->bridge existence here may work, but it's a bit convoluted.
Instead, you should leave this function alone, and just add the extra check directly in the function virNetworkBridgeInUse() (either before locking, or after unlocking "nets").
You know, there's another problem with this - adding this call to virNetDevExists() would be the first case of anything in the conf directory (which is supposed to be platform-independent parsing/formatting of XML and maintenance of lists of config objects) that calls a virNetDev*() function which is dependent on the current platform (and having root privileges). For the current uses of virNetworkBridgeInUse() it ends up not being a problem, because the only caller of virNetworkBridgeInUse() will already be verified as running on a platform that supports the virNetDev* functions and having root privileges (and/or certain to fail if we made this call on return), but it still leaves a bad taste in my mouth to be calling a device ioctl from a function in the conf directory. The trouble of course is that doing it differently turns this into a much bigger problem - as it is network_conf.c mostly isolates the network driver (bridge_driver.c) from the idea that a network could be defined without a bridge name specified, or that there might be a conflicting bridge name, by calling virNetworkSetBridgeName() which is in network_conf.c (it also calls virNetworkSetBridgeName() on its own when loading the network configs from disk, thus not even giving the network driver a chance to intervene). So I don't want to be the one to NACK based on this cross pollination, but thought I should point it out in case anyone with a stronger opinion did (and even if not, so that we see that even if we accept a patch like the current one to fix the bug now, we may still want to plan a different fix in the long run. Or maybe not - maybe I'm being too pedantic).
On Tue, Mar 24, 2015 at 9:18 PM, Laine Stump <laine@laine.org> wrote:
On 03/24/2015 11:12 AM, Laine Stump 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 | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index d7c5dec..c3ae2e4 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -3227,13 +3227,22 @@ virNetworkBridgeInUseHelper(const void *payload, int ret = 0; virNetworkObjPtr net = (virNetworkObjPtr) payload; const struct virNetworkBridgeInUseHelperData *data = opaque; + bool defined_bridge = false;
virObjectLock(net); if (net->def->bridge && - STREQ(net->def->bridge, data->bridge) && - !(data->skipname && STREQ(net->def->name, data->skipname))) - ret = 1; + STREQ(net->def->bridge, data->bridge)) { + defined_bridge = true; + if (!(data->skipname && STREQ(net->def->name, data->skipname))) + ret = 1; + } + virObjectUnlock(net); + + /* Bridge might have been created by a user manually outside libvirt */ + if (!defined_bridge && !ret) + ret = virNetDevExists(data->bridge) ? 1 : 0; + return ret; } This function is intended to be called once for each defined network on
On 03/24/2015 05:59 AM, Shivaprasad G Bhat wrote: the host, with data->bridge being the same each time, but net->def->bridge being different, so adding the check for data->bridge existence here may work, but it's a bit convoluted.
Instead, you should leave this function alone, and just add the extra check directly in the function virNetworkBridgeInUse() (either before locking, or after unlocking "nets").
Ok. Will do as suggested in the next version.
You know, there's another problem with this - adding this call to virNetDevExists() would be the first case of anything in the conf directory (which is supposed to be platform-independent parsing/formatting of XML and maintenance of lists of config objects) that calls a virNetDev*() function which is dependent on the current platform (and having root privileges). For the current uses of virNetworkBridgeInUse() it ends up not being a problem, because the only caller of virNetworkBridgeInUse() will already be verified as running on a platform that supports the virNetDev* functions and having root privileges (and/or certain to fail if we made this call on return), but it still leaves a bad taste in my mouth to be calling a device ioctl from a function in the conf directory.
The trouble of course is that doing it differently turns this into a much bigger problem - as it is network_conf.c mostly isolates the network driver (bridge_driver.c) from the idea that a network could be defined without a bridge name specified, or that there might be a conflicting bridge name, by calling virNetworkSetBridgeName() which is in network_conf.c (it also calls virNetworkSetBridgeName() on its own when loading the network configs from disk, thus not even giving the network driver a chance to intervene).
So I don't want to be the one to NACK based on this cross pollination, but thought I should point it out in case anyone with a stronger opinion did (and even if not, so that we see that even if we accept a patch like the current one to fix the bug now, we may still want to plan a different fix in the long run. Or maybe not - maybe I'm being too pedantic).
Hmm.. I too am not sure whats best here. Is it better to move the function call virSetBridgeName from virNetworkLoadConfig() to networkAutostartConfig() ? and virNetworkSetBridgeName, virNetworkBridgeInUse, virNetworkAllocateBridge and virNetworkBridgeInUseHelper function definitions to bridge_driver.c ? That way the calls will be from the driver only instead of conf.
participants (3)
-
Laine Stump -
Shivaprasad bhat -
Shivaprasad G Bhat