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

[Shanzhi Yu]

On 03/29/2016 12:23 AM, Laine Stump wrote:
> On 03/28/2016 10:42 AM, Laine Stump wrote:
>> 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.
>
> Sigh. The above is incorrect, as I was trying to trace through the code
> in my mind after mentally applying the patch, rather than actually
> trying it.
>

Thanks for your explanations, actually your fix is really what I want, I 
should go through virNetDevMacVLanReserveID carefully.


> As a matter of fact, the reality is worse than the above description.
> With this patch the device name is constructed by using the id number
> that we *want* to reserve rather than the one that we actually do
> reserve, but the bug in virNetDevMacVLanReserveID() is still there, so
> in fact what will happen is that the newly patched loop will
>
>   1) format the name to use as "macvtap0"
>   2) ask for macvtap0
>   3) get back macvtap2, but ignore that
>   4) try/fail to create macvtap0
>   5) *NOT* release the id that it just allocated (which was macvtap2)
>   6) loop again
>   7) format "macvtap2"
>   8) ask for macvtap2
>   9) get back macvtap3, but ignore that
> 10) try/fail to create macvtap2
> 11) *NOT release the id that was just reserved (macvtap3)
> 12) loop again
> 13) format macvtap3
> 14) ask for macvtap3
> 15) get back macvtap4 (even though nobody is using macvtap3,
>      we reserved it in the last iteration, didn't use it,
>      and didn't free it!)
> 16) try/succeed to create macvtap3
>
> So at this point, we have reserved macvtap2, macvtap3, and macvtap4,
> even though libvirt is only using macvtap3 (and something outside of
> libvirt is using macvtap2). The next time someone wants a macvtap
> device, we'll again start out at 0, get back 5, try/fail to create 0,
> ask for 1, get back 6, try/fail to create 1, ask for 2, get back 7,
> try/fail to create 2, ask for 3, get back 8, try/fail to create 3, ask
> for 4, get back 9, and finally succeed with macvtap4. So *now* we've
> reserved all names up through macvtap9, but are only using macvtap0,
> macvtap1 (these from previous), macvtap3, and macvtap4. We'll continue
> like this, reserving n-1 extra names for each new macvtap device "n",
> thus running out much more quickly.
>
> Anyway, that's probably much more analysis than necessary. The main
> points (in addition to the fact that it doesn't fix the root cause) are:
>
> a) it creates the macvtap name based on the desired id# rather than the
> one that was actually reserved
> b) it never frees the id's that it ends up not using because the create
> failed.
>
>
>>
>> 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;
>>
>> --
>> libvir-list mailing list
>> libvir-list at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
>>
>

-- 
Regards
shyu