[libvirt] [PATCH] virnetdevmacvlan.c: Introduce mutex for macvlan creation

[Daniel P. Berrange]

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 :|