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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]