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).
> +{
> + 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.
> + 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
