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

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.

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(a)redhat.com&gt; >> --- >> 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(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list >