[libvirt] [PATCH v1 03/11] bandwidth: Create (un)plug functions
Michal Privoznik
mprivozn at redhat.com
Mon Dec 3 15:55:41 UTC 2012
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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
More information about the libvir-list
mailing list