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@redhat.com>

src/util/virnetdevbandwidth.c

@@ -43,6 +43,108 @@ 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
+ * QDiscs act like black boxes deciding which packets should be
+ * held up and which should be sent immediately, it's the filter
+ * that 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 +518,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 +539,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 +562,15 @@ virNetDevBandwidthPlug(const char *brname,
     if (virCommandRun(cmd, NULL) < 0)
         goto cleanup;
 
-    virCommandFree(cmd);
+    if (virNetDevBandwidthManipulateFilter(brname, ifmac_ptr, id,
-    cmd = virCommandNew(TC);
+                                           class_id, false, true) < 0)
-    /* 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)
         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 +595,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 +602,7 @@ virNetDevBandwidthUnplug(const char *brname,
     }
 
     if (virAsprintf(&class_id, "1:%x", id) < 0 ||
-        virAsprintf(&qdisc_id, "%x:", id) < 0 ||
+        virAsprintf(&qdisc_id, "%x:", id) < 0)
-        virAsprintf(&filter_id, "%u", id) < 0)
         goto cleanup;
 
     cmd = virCommandNew(TC);
@@ -538,12 +614,8 @@ virNetDevBandwidthUnplug(const char *brname,
     if (virCommandRun(cmd, &cmd_ret) < 0)
         goto cleanup;
 
-    virCommandFree(cmd);
+    if (virNetDevBandwidthManipulateFilter(brname, NULL, id,
-    cmd = virCommandNew(TC);
+                                           NULL, true, false) < 0)
-    virCommandAddArgList(cmd, "filter", "del", "dev", brname,
-                         "prio", filter_id, NULL);
-
-    if (virCommandRun(cmd, &cmd_ret) < 0)
         goto cleanup;
 
     virCommandFree(cmd);
@@ -557,7 +629,6 @@ virNetDevBandwidthUnplug(const char *brname,
     ret = 0;
 
  cleanup:
-    VIR_FREE(filter_id);
     VIR_FREE(qdisc_id);
     VIR_FREE(class_id);
     virCommandFree(cmd);