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

Re: [libvirt] [PATCH v1 03/11] bandwidth: Create (un)plug functions



On 30.11.2012 18:53, Laine Stump wrote:
> On 11/19/2012 11:51 AM, 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 |  178 +++++++++++++++++++++++++++++++++++++++++
>>  src/util/virnetdevbandwidth.h |   14 +++
>>  3 files changed, 193 insertions(+), 0 deletions(-)
>>
>> diff --git a/po/POTFILES.in b/po/POTFILES.in
>> index 9768528..f51e2c7 100644
>> --- a/po/POTFILES.in
>> +++ b/po/POTFILES.in
>> @@ -154,6 +154,7 @@ src/util/virhash.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 fcc6b56..9597dcd 100644
>> --- a/src/util/virnetdevbandwidth.c
>> +++ b/src/util/virnetdevbandwidth.c
>> @@ -285,3 +285,181 @@ virNetDevBandwidthEqual(virNetDevBandwidthPtr a,
>>  
>>      return true;
>>  }
>> +
>> +/*
>> + * virNetDevBandwidthPlug:
>> + * @brname: name of the bridge
>> + * @net_bandwidth: QoS settings on @brname
>> + * @ifname: interface being plugged into @brname
>> + * @ifmac: MAC of @ifname
>> + * @bandwidth: QoS settings for @ifname
>> + * @id: unique ID (MUST be greater than 2)
> 
> * If it must be > 2, then you need to check for that at the top of the
> function.
> 
>> + *
>> + * 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 char *ifname,
>> +                       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 *mac[2];
>> +
>> +    if (!bandwidth || !bandwidth->in || !bandwidth->in->floor) {
>> +        /* nothing to set */
>> +        return 1;
>> +    }
>> +
>> +    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, ifname);
>> +        return -1;
>> +    }
>> +
>> +    virMacAddrGetRaw(ifmac_ptr, ifmac);
>> +
>> +    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
> 
> Heck, maybe not even then - don't forget about IPv6, along with the fact
> that I'm not convinced the host can ever know all the IP addresses used
> by an untrusted guest with 100% certainty (unless we filter for them I
> suppose).
> 
>> +     */
>> +    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(ceil);
>> +    VIR_FREE(floor);
>> +    VIR_FREE(mac[1]);
>> +    VIR_FREE(mac[0]);
> 
> * If the expression in the if() statement setting mac[0] and mac[1]
> short circuits before setting them, they will be uninitialized. (that
> would only happen in case of OOM, but it's still not nice).
> You should initialize them with "= {NULL, NULL}"
> 
> 
>> +    VIR_FREE(filter_id);
>> +    VIR_FREE(qdisc_id);
>> +    VIR_FREE(class_id);
>> +    virCommandFree(cmd);
> 
> It would be simpler to visually verify everything is being cleaned up if
> these were in either exactly the same order, or exactly the opposite
> order they were defined in.
> 
> 
>> +    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)
> 
> As I'm looking at all these uses of id's, I'm wondering if there's any
> danger of namespace conflict with other users of tc. (This isn't any
> critique, just curiousity).

No, class ID is specific within an NIC. That is, classID of 3 on eth0
doesn't interfere with class on vnet0. In fact, they have nothing in
common. What we could interfere with is - if user sets something
afterwards on fully libvirt managed interface. But I guess we don't
support this, right? It's all or nothing approach to me.

> 
>> +{
>> +    int ret = -1;
>> +    int cmd_ret = 0;
>> +    virCommandPtr cmd = NULL;
>> +    char *class_id = NULL;
>> +    char *qdisc_id = NULL;
>> +    char *filter_id = NULL;
>> +
>> +    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 */
> 
> What's your definition of "fatal"? In this case, if tc fails you return
> -1, not 0.

No, the return value of tc is stored into cmd_ret. Since we are not
passing a NULL here, virCommandRun should fail if something went really
wrong - e.g. OOM, or a pipe couldn't be created, and so on.

> 
>> +    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;
> 
> same here
> 
>> +
>> +    cmd = virCommandNew(TC);
>> +    virCommandAddArgList(cmd, "class", "del", "dev", brname,
>> +                         "classid", class_id, NULL);
>> +
>> +    if (virCommandRun(cmd, &cmd_ret) < 0)
>> +        goto cleanup;
> 
> and here. I don't see you being forgiving at all (is it maybe in the
> caller that you ignore the return status?)
> 
>> +
>> +    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..19eb418 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,17 @@ int virNetDevBandwidthCopy(virNetDevBandwidthPtr *dest, const virNetDevBandwidth
>>  
>>  bool virNetDevBandwidthEqual(virNetDevBandwidthPtr a, virNetDevBandwidthPtr b);
>>  
>> +int virNetDevBandwidthPlug(const char *brname,
>> +                           virNetDevBandwidthPtr net_bandwidth,
>> +                           const char *ifname,
>> +                           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 with the points marked as "*" fixed (other suggestions you can take
> or leave)
> 
> --
> libvir-list mailing list
> libvir-list redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 


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