[libvirt] [PATCH] util: generate correct macvtap name

[Laine Stump]

On 03/28/2016 06:43 AM, Shanzhi Yu wrote:
> in commit 370608b, bitmap of in-used macvtap devices was introduced.
> if there is already macvtap device created not by libvirt,
> virNetDevMacVLanCreateWithVPortProfile will failed after try
> MACVLAN_MAX_ID times call virNetDevMacVLanReleaseID
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1321546

NACK.

This is a bonafide bug, but not the right fix. Your patch would go back 
and pick up the unused "macvtap0" name rather than trying macvtap2, but 
once macvtap0 was in use, the next call (for yet another macvtap device) 
would attempt to use macvtap2 (since macvtap0 and macvtap1 were in use) 
and would then end up looping in the same manner as it did without your 
patch (MACVLAN_MAX_ID times == 8191) and then still reporting a failure.

The proper fix is to call virBitmapNextClearBit() (inside 
virNetDevMacVLanReserveID()) with the most recently failed id (or -1) 
rather than with 0. This is what I've done in the following patch:

https://www.redhat.com/archives/libvir-list/2016-March/msg01286.html


>
> Signed-off-by: Shanzhi Yu <shyu at redhat.com>
> ---
>   src/util/virnetdevmacvlan.c | 16 +++++++++-------
>   1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
> index 2409113..973363e 100644
> --- a/src/util/virnetdevmacvlan.c
> +++ b/src/util/virnetdevmacvlan.c
> @@ -988,7 +988,8 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested,
>           MACVTAP_NAME_PREFIX : MACVLAN_NAME_PREFIX;
>       const char *pattern = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ?
>           MACVTAP_NAME_PATTERN : MACVLAN_NAME_PATTERN;
> -    int rc, reservedID = -1;
> +    int rc = -1;
> +    int reservedID = 0;
>       char ifname[IFNAMSIZ];
>       int retries, do_retry = 0;
>       uint32_t macvtapMode;
> @@ -1072,16 +1073,17 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested,
>       retries = MACVLAN_MAX_ID;
>       while (!ifnameCreated && retries) {
>           virMutexLock(&virNetDevMacVLanCreateMutex);
> -        reservedID = virNetDevMacVLanReserveID(reservedID, flags, false, true);
> -        if (reservedID < 0) {
> -            virMutexUnlock(&virNetDevMacVLanCreateMutex);
> -            return -1;
> -        }
>           snprintf(ifname, sizeof(ifname), pattern, reservedID);
>           rc = virNetDevMacVLanCreate(ifname, type, macaddress, linkdev,
>                                       macvtapMode, &do_retry);
>           if (rc < 0) {
> -            virNetDevMacVLanReleaseID(reservedID, flags);
> +            reservedID++;
> +            reservedID = virNetDevMacVLanReserveID(reservedID, flags, false, true);
> +            if (reservedID < 0) {
> +                virMutexUnlock(&virNetDevMacVLanCreateMutex);
> +                return -1;
> +            }
> +
>               virMutexUnlock(&virNetDevMacVLanCreateMutex);
>               if (!do_retry)
>                   return -1;