[libvirt] [PATCH 1/2] util: keep/use a bitmap of in-use macvtap devices

[Laine Stump]

On 01/22/2016 07:47 AM, Pavel Hrdina wrote:
> On Thu, Jan 21, 2016 at 02:54:10PM -0500, Laine Stump wrote:
>
>
>>   
>> +/**
>> + * virNetDevMacVLanReserveNextFreeID:
>> + *
>> + *  @id: id to start scanning at - return first free ID *after* this
>> + *       id (use -1 to start looking at the beginning)
>> + *  @flags: set VIR_NETDEV_MACVLAN_CREATE_WITH_TAP for macvtapN else macvlanN
>> + *
>> + *  Reserve the indicated ID in the appropriate bitmap, or find the
>> + *  first free ID if @id is -1.
>> + *
> This comment isn't true.  The function takes the id, pass it to the
> virBitmanNextClearBit and get a next free ID even if original id >= 0.  It
> always tries to find next free id starting from the position specified by id.


Yes, you're right. This function was made by copying the other, and in 
the heat of development I forgot to revise the description :-)


>
> The second issue I have with those function is that they are almost identical
> and I think they can be easily merged together extending the
> virNetDevMacVLanReserveID() like this:
>
> static int
> virNetDevMacVLanReserverID(int id,
>                             unsigned int flags,
>                             bool quietfail,
>                             bool nextFree)
> {
>
> [...]
>
>      if ((id < 0 || nextFree ) &&
>          (id = virBitmapNextClearBit(bitmap, 0)) < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("no unused %s names available"),
>                         (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ?
>                         MACVTAP_NAME_PREFIX : MACVLAN_NAME_PREFIX);
>          return -1;
>      }


Yeah, that seems reasonable (although I dislike long strings of bools in 
arg lists, and also dislike creating new enums for flags). Truthfully I 
was doing "stream of consciousness programming" (think Jack Kerouac "On 
the Road" in C), which led to the copy-paste of 
virNetDevMacVLanReserveID() to virNetDevMacVlanReserveNextFreeID() 
(because I didn't want to destroy the original in the process of making 
the new one work), then forgot about going back to consolidate things 
once it was actually working.

I'll revisit and recombine as appropriate.



>> +            virReportSystemError(EEXIST,
>> -                                 _("Unable to create macvlan device %s"), tgifname);
>> +                                 _("Unable to create macvlan device %s"), ifnameRequested);
>> +            virMutexUnlock(&virNetDevMacVLanCreateMutex);
> Maybe use %s instead of macvlan as you've done in the new functions.


Yep, since I'm already touching the line anyway, might as well make the 
message more accurate.


>> -                virMutexUnlock(&virNetDevMacVLanCreateMutex);
>> +        if ((reservedID = virNetDevMacVLanReserveNextFreeID(reservedID, flags)) < 0) {
>> +            virMutexUnlock(&virNetDevMacVLanCreateMutex);
>> +            virReportSystemError(EEXIST, "%s",
>> +                                 _("All macvlan device names are already being used"));
>> +            return -1;
> Isn't this error message redundant?  There will already be an error reported
> by virNetDevMacVLanReserveNextFreeID().


You are correct!

Thanks for the review. V2 coming up!