On Thu, Jan 21, 2016 at 02:54:10PM -0500, Laine Stump wrote: [...]
src/libvirt_private.syms | 2 + src/qemu/qemu_process.c | 10 +- src/util/virnetdevmacvlan.c | 438 +++++++++++++++++++++++++++++++++++++------- src/util/virnetdevmacvlan.h | 5 +- 4 files changed, 390 insertions(+), 65 deletions(-)
[...]
diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index 496416e..20a821a 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c
[...]
+ * virNetDevMacVLanReserveID: + * + * @id: id 0 - MACVLAN_MAX_ID+1 to reserve (or -1 for "first free") + * @flags: set VIR_NETDEV_MACVLAN_CREATE_WITH_TAP for macvtapN else macvlanN + * @quietfail: don't log an error if this name is already in-use + * + * Reserve the indicated ID in the appropriate bitmap, or find the + * first free ID if @id is -1. + * + * returns id or -1 to indicate failure + */ +static int +virNetDevMacVLanReserveID(int id, unsigned int flags, bool quietfail) +{ + virBitmapPtr bitmap; + + if (virNetDevMacVLanInitialize() < 0) + return -1; + + bitmap = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? + macvtapIDs : macvlanIDs; + + if (id > MACVLAN_MAX_ID) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("can't use name %s%d - out of range 0-%d"), + (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? + MACVTAP_NAME_PREFIX : MACVLAN_NAME_PREFIX, + id, MACVLAN_MAX_ID); + return -1; + } + + if (id < 0 && + (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; + } + + if (virBitmapIsBitSet(bitmap, id)) { + if (quietfail) { + VIR_INFO("couldn't reserve name %s%d - already in use", + (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? + MACVTAP_NAME_PREFIX : MACVLAN_NAME_PREFIX, id); + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("couldn't reserve name %s%d - already in use"), + (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? + MACVTAP_NAME_PREFIX : MACVLAN_NAME_PREFIX, id); + } + return -1; + } + + if (virBitmapSetBit(bitmap, id) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("couldn't mark %s%d as used"), + (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? + MACVTAP_NAME_PREFIX : MACVLAN_NAME_PREFIX, id); + return -1; + } + VIR_INFO("reserving device %s%d", + (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? + MACVTAP_NAME_PREFIX : MACVLAN_NAME_PREFIX, id); + return id; +} + + +/** + * 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. 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; } [...] }
+ * returns id or -1 to indicate failure + */ +static int +virNetDevMacVLanReserveNextFreeID(int id, unsigned int flags) +{ + virBitmapPtr bitmap; + + if (virNetDevMacVLanInitialize() < 0) + return -1; + + bitmap = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? + macvtapIDs : macvlanIDs; + + if (id > MACVLAN_MAX_ID) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("can't use name %s%d - out of range 0-%d"), + (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? + MACVTAP_NAME_PREFIX : MACVLAN_NAME_PREFIX, + id, MACVLAN_MAX_ID); + return -1; + } + + if ((id = virBitmapNextClearBit(bitmap, id)) < 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; + } + + if (virBitmapIsBitSet(bitmap, id)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("couldn't reserve name %s%d - already in use"), + (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? + MACVTAP_NAME_PREFIX : MACVLAN_NAME_PREFIX, id); + return -1; + } + + if (virBitmapSetBit(bitmap, id) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("couldn't mark %s%d as used"), + (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? + MACVTAP_NAME_PREFIX : MACVLAN_NAME_PREFIX, id); + return -1; + } + VIR_INFO("reserving device %s%d", + (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? + MACVTAP_NAME_PREFIX : MACVLAN_NAME_PREFIX, id); + return id; +} +
[...]
@@ -724,14 +1002,20 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char *ifname, * virNetDevMacVLanCreateWithVPortProfile: * Create an instance of a macvtap device and open its tap character * device. - * @tgifname: Interface name that the macvtap is supposed to have. May - * be NULL if this function is supposed to choose a name + + * @ifnameRequested: Interface name that the caller wants the macvtap + * device to have, or NULL to pick the first available name + * appropriate for the type (macvlan%d or macvtap%d). If the + * suggested name fits one of those patterns, but is already in + * use, we will fallback to finding the first available. If the + * suggested name *doesn't* fit a pattern and the name is in use, + * we will fail. * @macaddress: The MAC address for the macvtap device * @linkdev: The interface name of the NIC to connect to the external bridge - * @mode: int describing the mode for 'bridge', 'vepa', 'private' or 'passthru'. + * @mode: macvtap mode (VIR_NETDEV_MACVLAN_MODE_(BRIDGE|VEPA|PRIVATE|PASSTHRU) * @vmuuid: The UUID of the VM the macvtap belongs to * @virtPortProfile: pointer to object holding the virtual port profile data - * @res_ifname: Pointer to a string pointer where the actual name of the + * @ifnameResult: Pointer to a string pointer where the actual name of the * interface will be stored into if everything succeeded. It is up * to the caller to free the string. * @tapfd: array of file descriptor return value for the new tap device @@ -744,39 +1028,36 @@ virNetDevMacVLanVPortProfileRegisterCallback(const char *ifname, * * Return 0 on success, -1 on error. */ -int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, - const virMacAddr *macaddress, - const char *linkdev, - virNetDevMacVLanMode mode, - const unsigned char *vmuuid, - virNetDevVPortProfilePtr virtPortProfile, - char **res_ifname, - virNetDevVPortProfileOp vmOp, - char *stateDir, - int *tapfd, - size_t tapfdSize, - unsigned int flags) +int +virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested, + const virMacAddr *macaddress, + const char *linkdev, + virNetDevMacVLanMode mode, + const unsigned char *vmuuid, + virNetDevVPortProfilePtr virtPortProfile, + char **ifnameResult, + virNetDevVPortProfileOp vmOp, + char *stateDir, + int *tapfd, + size_t tapfdSize, + unsigned int flags) { const char *type = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? "macvtap" : "macvlan"; - const char *prefix = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? - MACVTAP_NAME_PREFIX : MACVLAN_NAME_PREFIX; const char *pattern = (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) ? MACVTAP_NAME_PATTERN : MACVLAN_NAME_PATTERN; - int c, rc; + int rc, reservedID = -1; char ifname[IFNAMSIZ]; int retries, do_retry = 0; uint32_t macvtapMode; - const char *cr_ifname = NULL; + const char *ifnameCreated = NULL; int ret; int vf = -1; bool vnet_hdr = flags & VIR_NETDEV_MACVLAN_VNET_HDR;
macvtapMode = modeMap[mode];
- *res_ifname = NULL; - - VIR_DEBUG("%s: VM OPERATION: %s", __FUNCTION__, virNetDevVPortProfileOpTypeToString(vmOp)); + *ifnameResult = NULL;
/** Note: When using PASSTHROUGH mode with MACVTAP devices the link * device's MAC address must be set to the VMs MAC address. In @@ -803,52 +1084,81 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname, } }
- if (tgifname) { - if ((ret = virNetDevExists(tgifname)) < 0) - return -1; + if (ifnameRequested) { + bool isAutoName + = (STRPREFIX(ifnameRequested, MACVTAP_NAME_PREFIX) || + STRPREFIX(ifnameRequested, MACVLAN_NAME_PREFIX)); + + VIR_INFO("Requested macvtap device name: %s", ifnameRequested); + virMutexLock(&virNetDevMacVLanCreateMutex);
+ if ((ret = virNetDevExists(ifnameRequested)) < 0) { + virMutexUnlock(&virNetDevMacVLanCreateMutex); + return -1; + } if (ret) { - if (STRPREFIX(tgifname, prefix)) + if (isAutoName) goto create_name; 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.
return -1; } - cr_ifname = tgifname; - rc = virNetDevMacVLanCreate(tgifname, type, macaddress, linkdev, - macvtapMode, &do_retry); - if (rc < 0) + if (isAutoName && + (reservedID = virNetDevMacVLanReserveName(ifnameRequested, true)) < 0) { + reservedID = -1; + goto create_name; + } + + if (virNetDevMacVLanCreate(ifnameRequested, type, macaddress, + linkdev, macvtapMode, &do_retry) < 0) { + if (isAutoName) { + virNetDevMacVLanReleaseName(ifnameRequested); + reservedID = -1; + goto create_name; + } + virMutexUnlock(&virNetDevMacVLanCreateMutex); return -1; - } else { + } + /* virNetDevMacVLanCreate() was successful - use this name */ + ifnameCreated = ifnameRequested; create_name: - retries = 5; + virMutexUnlock(&virNetDevMacVLanCreateMutex); + } + + retries = 8192; + while (!ifnameCreated && retries) { virMutexLock(&virNetDevMacVLanCreateMutex); - for (c = 0; c < 8192; c++) { - snprintf(ifname, sizeof(ifname), pattern, c); - if ((ret = virNetDevExists(ifname)) < 0) { - 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().