Re: [libvirt] [PATCH 5/7] virnetdevbandwidth.c: Separate tc filter creation to a function

Not only this simplifies the code a bit, it prepares the environment for upcoming patches. The new virNetDevBandwidthManipulateFilter() function is capable of both removing a filter and adding a new one. At the same time! Yeah, this is not currently used anywhere but look at the next commit where you'll see it. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/util/virnetdevbandwidth.c | 142 +++++++++++++++++++++++++++++++----------- 1 file changed, 106 insertions(+), 36 deletions(-) diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c index 943178b..c57c8c0 100644 --- a/src/util/virnetdevbandwidth.c +++ b/src/util/virnetdevbandwidth.c @@ -43,6 +43,107 @@ virNetDevBandwidthFree(virNetDevBandwidthPtr def) VIR_FREE(def); } +/** + * virNetDevBandwidthManipulateFilter: + * @ifname: interface to operate on + * @ifmac_ptr: MAC of the interface to create filter over + * @id: filter ID + * @class_id: where to place traffic + * @remove_old: whether to remove the filter + * @create_new: whether to create the filter + * + * TC filters are as crucial for traffic shaping as QDiscs. While + * QDisc acts like black boxes deciding which packets should be

+ * held up and which should be sent immediately, it's filter who

+ * places a packet into the box. So, we may end up constructing a + * set of filters on a single device (e.g. a bridge) and filter + * the traffic into QDiscs based on the originating vNET device. + * + * Long story short, @ifname is the interface where the filter + * should be created. The @ifmac_ptr is the MAC address for which + * the filter should be created (usually different to the MAC + * address of @ifname). Then, like everything - even filters have + * an @id which should be unique (per @ifname). And @class_id + * tells into which QDisc should filter place the traffic.

+ * + * This function can be used for both, removing stale filter + * (@remove_old set to true) and creating new one (@create_new + * set to true). Both at once for the same price!

+ * + * Returns: 0 on success, + * -1 otherwise (with error reported). + */ +static int ATTRIBUTE_NONNULL(1) +virNetDevBandwidthManipulateFilter(const char *ifname, + const virMacAddr *ifmac_ptr, + unsigned int id, + const char *class_id, + bool remove_old, + bool create_new)

+{ + int ret = -1; + char *filter_id = NULL; + virCommandPtr cmd = NULL; + unsigned char ifmac[VIR_MAC_BUFLEN]; + char *mac[2] = {NULL, NULL}; + + if (!(remove_old || create_new)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("filter creation API error")); + goto cleanup; + }

+ + /* u32 filters must have 800:: prefix. Don't ask. */ + if (virAsprintf(&filter_id, "800::%u", id) < 0) + goto cleanup; + + if (remove_old) { + int cmd_ret = 0; + + cmd = virCommandNew(TC); + virCommandAddArgList(cmd, "filter", "del", "dev", ifname, + "prio", "2", "handle", filter_id, "u32", NULL); + + if (virCommandRun(cmd, &cmd_ret) < 0) + goto cleanup; + + } + + if (create_new) { + virMacAddrGetRaw(ifmac_ptr, ifmac); + + if (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) + goto cleanup; + + virCommandFree(cmd); + cmd = virCommandNew(TC); + /* Okay, this not nice. But since libvirt does not necessarily track + * 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", ifname, "protocol", "ip", + "prio", "2", "handle", 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(filter_id); + virCommandFree(cmd); + return ret; +} + /** * virNetDevBandwidthSet: @@ -416,19 +517,15 @@ virNetDevBandwidthPlug(const char *brname, 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 (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) { @@ -441,10 +538,6 @@ virNetDevBandwidthPlug(const char *brname, 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 : @@ -468,31 +561,15 @@ virNetDevBandwidthPlug(const char *brname, 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) + if (virNetDevBandwidthManipulateFilter(brname, ifmac_ptr, id, + class_id, false, true) < 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); @@ -517,7 +594,6 @@ virNetDevBandwidthUnplug(const char *brname, 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); @@ -525,8 +601,7 @@ virNetDevBandwidthUnplug(const char *brname, } if (virAsprintf(&class_id, "1:%x", id) < 0 || - virAsprintf(&qdisc_id, "%x:", id) < 0 || - virAsprintf(&filter_id, "%u", id) < 0) + virAsprintf(&qdisc_id, "%x:", id) < 0) goto cleanup; cmd = virCommandNew(TC); @@ -538,12 +613,8 @@ virNetDevBandwidthUnplug(const char *brname, 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) + if (virNetDevBandwidthManipulateFilter(brname, NULL, id, + NULL, true, false) < 0) goto cleanup; virCommandFree(cmd); @@ -557,7 +628,6 @@ virNetDevBandwidthUnplug(const char *brname, ret = 0; cleanup: - VIR_FREE(filter_id); VIR_FREE(qdisc_id); VIR_FREE(class_id); virCommandFree(cmd);