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@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/libvir-list > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list