Re: [libvirt] [PATCH 1/2] util: keep/use a bitmap of in-use macvtap devices
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!