4:54 a.m.
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
4:57 a.m.
New subject: [libvirt] [PATCH v3 1/2] cleanup conf/device_conf.h inclusion from util/virnetdev.h
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(a)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 {
11:33 a.m.
New subject: [libvirt] [PATCH v3 1/2] cleanup conf/device_conf.h inclusion from util/virnetdev.h
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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
5:05 a.m.
New subject: [libvirt] [PATCH v3 1/2] cleanup conf/device_conf.h inclusion from util/virnetdev.h
Hi Laine,
Thanks for the comments. Please find my reply inline.
On Tue, Mar 24, 2015 at 10:03 PM, Laine Stump <laine(a)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(a)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(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
I am sending v2 with the code movement as suggested.
Thanks,
Shiva
4:59 a.m.
New subject: [libvirt] [PATCH v3 2/2] network_conf: check if bridge exists on host for user created bridges
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(a)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;
}
10:12 a.m.
New subject: [libvirt] [PATCH v3 2/2] network_conf: check if bridge exists on host for user created bridges
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(a)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").
10:48 a.m.
New subject: [libvirt] [PATCH v3 2/2] network_conf: check if bridge exists on host for user created bridges
On 03/24/2015 11:12 AM, Laine Stump wrote:
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(a)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").
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).
8:18 a.m.
New subject: [libvirt] [PATCH v3 2/2] network_conf: check if bridge exists on host for user created bridges
On Tue, Mar 24, 2015 at 9:18 PM, Laine Stump <laine(a)laine.org> wrote:
On 03/24/2015 11:12 AM, Laine Stump wrote:
> 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(a)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").
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.
3531
days inactive
3532
days old
7 comments
3 participants
participants (3)
-
Laine Stump -
Shivaprasad bhat -
Shivaprasad G Bhat