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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list