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

Re: [libvirt] [PATCH v2 4/8] bandwidth: Create (un)plug functions



On 12/04/2012 02:18 PM, Michal Privoznik wrote:
> These set bridge part of QoS when bringing domain's interface up.
> Long story short, if there's a 'floor' set, a new QoS class is created.
> ClassID MUST be unique within the bridge and should be kept for
> unplug phase.
> ---
>  po/POTFILES.in                |    1 +
>  src/util/virnetdevbandwidth.c |  188 +++++++++++++++++++++++++++++++++++++++++
>  src/util/virnetdevbandwidth.h |   13 +++
>  3 files changed, 202 insertions(+), 0 deletions(-)
>
> diff --git a/po/POTFILES.in b/po/POTFILES.in
> index 586aa2b..d47f445 100644
> --- a/po/POTFILES.in
> +++ b/po/POTFILES.in
> @@ -158,6 +158,7 @@ src/util/virinitctl.c
>  src/util/virkeyfile.c
>  src/util/virlockspace.c
>  src/util/virnetdev.c
> +src/util/virnetdevbandwidth.c
>  src/util/virnetdevbridge.c
>  src/util/virnetdevmacvlan.c
>  src/util/virnetdevopenvswitch.c
> diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c
> index 71c272e..6c4920b 100644
> --- a/src/util/virnetdevbandwidth.c
> +++ b/src/util/virnetdevbandwidth.c
> @@ -352,3 +352,191 @@ virNetDevBandwidthEqual(virNetDevBandwidthPtr a,
>  
>      return true;
>  }
> +
> +/*
> + * virNetDevBandwidthPlug:
> + * @brname: name of the bridge
> + * @net_bandwidth: QoS settings on @brname
> + * @ifmac: MAC of interface
> + * @bandwidth: QoS settings for interface
> + * @id: unique ID (MUST be greater than 2)
> + *
> + * Set bridge part of interface QoS settings, e.g. guaranteed
> + * bandwidth.  @id is an unique ID (among @brname) from which
> + * other identifiers for class, qdisc and filter are derived.
> + * However, two classes were already set up (by
> + * virNetDevBandwidthSet). That's why this @id MUST be greater
> + * than 2. You may want to keep passed @id, as it is used later
> + * by virNetDevBandwidthUnplug.
> + *
> + * Returns:
> + * 1 if there is nothing to set
> + * 0 if QoS set successfully
> + * -1 otherwise.
> + */
> +int
> +virNetDevBandwidthPlug(const char *brname,
> +                       virNetDevBandwidthPtr net_bandwidth,
> +                       const virMacAddrPtr ifmac_ptr,
> +                       virNetDevBandwidthPtr bandwidth,
> +                       unsigned int id)
> +{
> +    int ret = -1;
> +    virCommandPtr cmd = NULL;
> +    char *class_id = NULL;
> +    char *qdisc_id = NULL;
> +    char *filter_id = NULL;
> +    char *floor = NULL;
> +    char *ceil = NULL;
> +    unsigned char ifmac[VIR_MAC_BUFLEN];
> +    char ifmacStr[VIR_MAC_STRING_BUFLEN];
> +    char *mac[2] = {NULL, NULL};
> +
> +    if (!bandwidth || !bandwidth->in || !bandwidth->in->floor) {
> +        /* nothing to set */
> +        return 1;
> +    }

I find this "return 1 when nothing needs to be done" a bit confusing, in
particular the bit in 6/8 where you log the warning about "maybe you
should upgrade libvirt". It seems like what you're checking for should
only be possible if there's an inconsistency/error among your functions,
is that right? Is there a real danger of that, or are you just being
paranoid? :-) (at some point you have to be able to make assumptions
about the correctness of other functions in the same subsystem, or your
code ends up spending most of its energy protecting against itself)



> +
> +    if (id <= 2) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid class ID %d"), id);
> +        return -1;
> +    }
> +
> +    virMacAddrGetRaw(ifmac_ptr, ifmac);
> +    virMacAddrFormat(ifmac_ptr, ifmacStr);
> +
> +    if (!net_bandwidth || !net_bandwidth->in) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                       _("Bridge '%s' has no QoS set, therefore "
> +                         "unable to set 'floor' on '%s'"),
> +                       brname, ifmacStr);
> +        return -1;
> +    }
> +
> +    if (virAsprintf(&class_id, "1:%x", id) < 0 ||
> +        virAsprintf(&qdisc_id, "%x:", id) < 0 ||
> +        virAsprintf(&filter_id, "%u", id) < 0 ||
> +        virAsprintf(&mac[0], "0x%02x%02x%02x%02x", ifmac[2],
> +                    ifmac[3], ifmac[4], ifmac[5]) < 0 ||
> +        virAsprintf(&mac[1], "0x%02x%02x", ifmac[0], ifmac[1]) < 0 ||
> +        virAsprintf(&floor, "%llukbps", bandwidth->in->floor) < 0 ||
> +        virAsprintf(&ceil, "%llukbps", net_bandwidth->in->peak ?
> +                    net_bandwidth->in->peak :
> +                    net_bandwidth->in->average) < 0) {
> +        virReportOOMError();
> +        goto cleanup;
> +    }
> +
> +    cmd = virCommandNew(TC);
> +    virCommandAddArgList(cmd, "class", "add", "dev", brname, "parent", "1:1",
> +                         "classid", class_id, "htb", "rate", floor,
> +                         "ceil", ceil, NULL);
> +
> +    if (virCommandRun(cmd, NULL) < 0)
> +        goto cleanup;
> +
> +    virCommandFree(cmd);
> +    cmd = virCommandNew(TC);
> +    virCommandAddArgList(cmd, "qdisc", "add", "dev", brname, "parent",
> +                         class_id, "handle", qdisc_id, "sfq", "perturb",
> +                         "10", NULL);
> +
> +    if (virCommandRun(cmd, NULL) < 0)
> +        goto cleanup;
> +
> +    virCommandFree(cmd);
> +    cmd = virCommandNew(TC);
> +    /* Okay, this not nice. But since libvirt does not know anything about
> +     * interface IP address(es), and tc fw filter simply refuse to use ebtables
> +     * marks, we need to use u32 selector to match MAC address.
> +     * If libvirt will ever know something, remove this FIXME
> +     */
> +    virCommandAddArgList(cmd, "filter", "add", "dev", brname, "protocol", "ip",
> +                         "prio", filter_id, "u32",
> +                         "match", "u16", "0x0800", "0xffff", "at", "-2",
> +                         "match", "u32", mac[0], "0xffffffff", "at", "-12",
> +                         "match", "u16", mac[1], "0xffff", "at", "-14",
> +                         "flowid", class_id, NULL);
> +
> +    if (virCommandRun(cmd, NULL) < 0)
> +        goto cleanup;
> +
> +    ret = 0;
> +
> +cleanup:
> +    VIR_FREE(mac[1]);
> +    VIR_FREE(mac[0]);
> +    VIR_FREE(ceil);
> +    VIR_FREE(floor);
> +    VIR_FREE(filter_id);
> +    VIR_FREE(qdisc_id);
> +    VIR_FREE(class_id);
> +    virCommandFree(cmd);
> +    return ret;
> +}
> +
> +/*
> + * virNetDevBandwidthUnplug:
> + * @brname: from which bridge are we unplugging
> + * @id: unique identifier (MUST be greater than 2)
> + *
> + * Remove QoS settings from bridge.
> + *
> + * Returns 0 on success, -1 otherwise.
> + */
> +int
> +virNetDevBandwidthUnplug(const char *brname,
> +                         unsigned int id)
> +{
> +    int ret = -1;
> +    int cmd_ret = 0;
> +    virCommandPtr cmd = NULL;
> +    char *class_id = NULL;
> +    char *qdisc_id = NULL;
> +    char *filter_id = NULL;
> +
> +    if (id <= 2) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid class ID %d"), id);
> +        return -1;
> +    }
> +
> +    if (virAsprintf(&class_id, "1:%x", id) < 0 ||
> +        virAsprintf(&qdisc_id, "%x:", id) < 0 ||
> +        virAsprintf(&filter_id, "%u", id) < 0) {
> +        virReportOOMError();
> +        goto cleanup;
> +    }
> +
> +    cmd = virCommandNew(TC);
> +    virCommandAddArgList(cmd, "qdisc", "del", "dev", brname,
> +                         "handle", qdisc_id, NULL);
> +
> +    /* Don't threat tc errors as fatal, but
> +     * try to remove as much as possible */
> +    if (virCommandRun(cmd, &cmd_ret) < 0)
> +        goto cleanup;
> +
> +    virCommandFree(cmd);
> +    cmd = virCommandNew(TC);
> +    virCommandAddArgList(cmd, "filter", "del", "dev", brname,
> +                         "prio", filter_id, NULL);
> +
> +    if (virCommandRun(cmd, &cmd_ret) < 0)
> +        goto cleanup;
> +
> +    cmd = virCommandNew(TC);
> +    virCommandAddArgList(cmd, "class", "del", "dev", brname,
> +                         "classid", class_id, NULL);
> +
> +    if (virCommandRun(cmd, &cmd_ret) < 0)
> +        goto cleanup;
> +
> +    ret = 0;
> +
> +cleanup:
> +    VIR_FREE(filter_id);
> +    VIR_FREE(qdisc_id);
> +    VIR_FREE(class_id);
> +    virCommandFree(cmd);
> +    return ret;
> +}
> diff --git a/src/util/virnetdevbandwidth.h b/src/util/virnetdevbandwidth.h
> index d308ab2..3d4072d 100644
> --- a/src/util/virnetdevbandwidth.h
> +++ b/src/util/virnetdevbandwidth.h
> @@ -24,6 +24,7 @@
>  # define __VIR_NETDEV_BANDWIDTH_H__
>  
>  # include "internal.h"
> +# include "virmacaddr.h"
>  
>  typedef struct _virNetDevBandwidthRate virNetDevBandwidthRate;
>  typedef virNetDevBandwidthRate *virNetDevBandwidthRatePtr;
> @@ -53,4 +54,16 @@ int virNetDevBandwidthCopy(virNetDevBandwidthPtr *dest, const virNetDevBandwidth
>  
>  bool virNetDevBandwidthEqual(virNetDevBandwidthPtr a, virNetDevBandwidthPtr b);
>  
> +int virNetDevBandwidthPlug(const char *brname,
> +                           virNetDevBandwidthPtr net_bandwidth,
> +                           const virMacAddrPtr ifmac_ptr,
> +                           virNetDevBandwidthPtr bandwidth,
> +                           unsigned int id)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
> +    ATTRIBUTE_RETURN_CHECK;
> +
> +int virNetDevBandwidthUnplug(const char *brname,
> +                             unsigned int id)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
> +
>  #endif /* __VIR_NETDEV_BANDWIDTH_H__ */

ACK.


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