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().
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list