On Thu, Feb 28, 2013 at 04:25:30PM +0100, Michal Privoznik wrote:
Currently, after we removed the qemu driver lock, it may happen
that two or more threads will start up a machine with macvlan and
race over virNetDevMacVLanCreateWithVPortProfile(). However,
there's a racy section in which we are generating a sequence of
possible device names and detecting if they exits. If we found
one which doesn't we try to create a device with that name.
However, the other thread is doing just the same. Assume it will
succeed and we must therefore fail. If this happens more than 5
times (which in massive parallel startup surely will) we return
-1 without any error reported. This patch is a simple hack to
both of these problems. It introduces a mutex, so only one thread
will enter the section, and if it runs out of possibilities,
error is reported. Moreover, the number of retries is raised to 20.
---
This is just a quick hack which aim is to be as small as possible in order to
be well understood and hence included in the release. After the release, this
should be totally dropped in flavour of virNetDevNameAllocator.
src/util/virnetdevmacvlan.c | 38 ++++++++++++++++++++++++++++++++------
1 file changed, 32 insertions(+), 6 deletions(-)
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
index a74db1e..bf5e7e0 100644
--- a/src/util/virnetdevmacvlan.c
+++ b/src/util/virnetdevmacvlan.c
@@ -31,6 +31,7 @@
#include "virmacaddr.h"
#include "virutil.h"
#include "virerror.h"
+#include "virthread.h"
#define VIR_FROM_THIS VIR_FROM_NET
@@ -71,6 +72,15 @@ VIR_ENUM_IMPL(virNetDevMacVLanMode, VIR_NETDEV_MACVLAN_MODE_LAST,
# define MACVLAN_NAME_PREFIX "macvlan"
# define MACVLAN_NAME_PATTERN "macvlan%d"
+virMutex virNetDevMacVLanCreateMutex;
+
+static int virNetDevMacVLanCreateMutexOnceInit(void)
+{
+ return virMutexInit(&virNetDevMacVLanCreateMutex);
+}
+
+VIR_ONCE_GLOBAL_INIT(virNetDevMacVLanCreateMutex);
+
/**
* virNetDevMacVLanCreate:
*
@@ -829,7 +839,7 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname,
char ifname[IFNAMSIZ];
int retries, do_retry = 0;
uint32_t macvtapMode;
- const char *cr_ifname;
+ const char *cr_ifname = NULL;
int ret;
int vf = -1;
@@ -870,23 +880,39 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname,
return -1;
} else {
create_name:
- retries = 5;
+ if (virNetDevMacVLanCreateMutexInitialize() < 0) {
+ virReportSystemError(errno, "%s", _("unable to init
mutext"));
+ return -1;
+ }
This is flawed - the way the global initializers work is that you
must report the error in the initializer. It preserves it in a
dedicated virERrorPtr and then copies it to the current virErrorPtr
thread local back in the calling code.
+
+ retries = 20;
I think this is not required now we are using a mutex to serialize
everything. The only race is between libvirtd and non-libvirtd apps
creating macvtap devices, which basically doesn't exist.
Daniel
--
|:
http://berrange.com -o-
http://www.flickr.com/photos/dberrange/ :|
|:
http://libvirt.org -o-
http://virt-manager.org :|
|:
http://autobuild.org -o-
http://search.cpan.org/~danberr/ :|
|:
http://entangle-photo.org -o-
http://live.gnome.org/gtk-vnc :|