There will soon be two separate users of tc on virtual networks, and
both will use the "qdisc root handle 1: htb" to add tx filters. One or the
other could get the first chance to add the qdisc, and then if at a
later time the other decides to use it, we need to prevent the 2nd
user from attempting to re-add the qdisc (because that just generates
an error).

We do this by running "tc qdisc show dev $bridge handle 1:" then
checking if the output of that command starts with "qdisc htb 1:
root". If it does then the qdisc is already there. If not then we need
to add it now.

Signed-off-by: Laine Stump <laine@redhat.com>
---
 src/util/virnetdevbandwidth.c  | 33 +++++++++++++++++++++++++++------
 tests/virnetdevbandwidthtest.c |  3 +++
 2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c
index 09c10e9a15..ae7214a9d5 100644
--- a/src/util/virnetdevbandwidth.c
+++ b/src/util/virnetdevbandwidth.c
@@ -797,12 +797,33 @@ int
 virNetDevBandWidthAddTxFilterParentQdisc(const char *ifname,
                                          bool hierarchical_class)
 {
-    g_autoptr(virCommand) cmd = NULL;
+    g_autoptr(virCommand) testCmd = NULL;
+    g_autofree char *testResult = NULL;
 
-    cmd = virCommandNew(TC);
-    virCommandAddArgList(cmd, "qdisc", "add", "dev", ifname, "root",
-                         "handle", "1:", "htb", "default",
-                         hierarchical_class ? "2" : "1", NULL);
+    /* first check it the qdisc with handle 1: was already added for
+     * this interface by someone else
+     */
+    testCmd = virCommandNew(TC);
+    virCommandAddArgList(testCmd, "qdisc", "show", "dev", ifname,
+                         "handle", "1:", NULL);
+    virCommandSetOutputBuffer(testCmd, &testResult);
 
-    return virCommandRun(cmd, NULL);
+    if (virCommandRun(testCmd, NULL) < 0)
+        return -1;
+
+    /* output will be something like: "qdisc htb 1: root refcnt ..."
+     * if the qdisc was already added.
+     */
+    if (!(testResult && STRPREFIX(testResult, "qdisc htb 1: root"))) {
+        /* didn't find qdisc in output, so we need to add one */
+        g_autoptr(virCommand) addCmd = virCommandNew(TC);
+
+        virCommandAddArgList(addCmd, "qdisc", "add", "dev", ifname, "root",
+                             "handle", "1:", "htb", "default",
+                             hierarchical_class ? "2" : "1", NULL);
+
+        return virCommandRun(addCmd, NULL);
+    }
+
+    return 0;
 }
diff --git a/tests/virnetdevbandwidthtest.c b/tests/virnetdevbandwidthtest.c
index 75f960e402..8ee57ccc24 100644
--- a/tests/virnetdevbandwidthtest.c
+++ b/tests/virnetdevbandwidthtest.c
@@ -140,6 +140,7 @@ mymain(void)
                 "</bandwidth>",
                 TC " qdisc del dev eth0 root\n"
                 TC " qdisc del dev eth0 ingress\n"
+                TC " qdisc show dev eth0 handle 1:\n"
                 TC " qdisc add dev eth0 root handle 1: htb default 1\n"
                 TC " class add dev eth0 parent 1: classid 1:1 htb rate 1024kbps quantum 87\n"
                 TC " qdisc add dev eth0 parent 1:1 handle 2: sfq perturb 10\n"
@@ -170,6 +171,7 @@ mymain(void)
                 "</bandwidth>",
                 TC " qdisc del dev eth0 root\n"
                 TC " qdisc del dev eth0 ingress\n"
+                TC " qdisc show dev eth0 handle 1:\n"
                 TC " qdisc add dev eth0 root handle 1: htb default 1\n"
                 TC " class add dev eth0 parent 1: classid 1:1 htb rate 1kbps ceil 2kbps burst 4kb quantum 1\n"
                 TC " qdisc add dev eth0 parent 1:1 handle 2: sfq perturb 10\n"
@@ -192,6 +194,7 @@ mymain(void)
                 "</bandwidth>",
                 TC " qdisc del dev eth0 root\n"
                 TC " qdisc del dev eth0 ingress\n"
+                TC " qdisc show dev eth0 handle 1:\n"
                 TC " qdisc add dev eth0 root handle 1: htb default 1\n"
                 TC " class add dev eth0 parent 1: classid 1:1 htb rate 4294967295kbps quantum 366503875\n"
                 TC " qdisc add dev eth0 parent 1:1 handle 2: sfq perturb 10\n"
-- 
2.47.0

On Fri, Nov 22, 2024 at 04:16:37PM -0500, Laine Stump wrote:
> There will soon be two separate users of tc on virtual networks, and
> both will use the "qdisc root handle 1: htb" to add tx filters. One or the
> other could get the first chance to add the qdisc, and then if at a
> later time the other decides to use it, we need to prevent the 2nd
> user from attempting to re-add the qdisc (because that just generates
> an error).
> 
> We do this by running "tc qdisc show dev $bridge handle 1:" then
> checking if the output of that command starts with "qdisc htb 1:
> root". If it does then the qdisc is already there. If not then we need
> to add it now.
> 


> diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c
> index 09c10e9a15..ae7214a9d5 100644
> --- a/src/util/virnetdevbandwidth.c
> +++ b/src/util/virnetdevbandwidth.c

> +
> +    /* output will be something like: "qdisc htb 1: root refcnt ..."
> +     * if the qdisc was already added.
> +     */
> +    if (!(testResult && STRPREFIX(testResult, "qdisc htb 1: root"))) {

I wonder a little how stable "tc qdisc show" output format has been
over time ? This is a very exact match we're performing. Don't
suppose there's some way to detect this scenario without parsing
human output from 'tc' ?

> +        /* didn't find qdisc in output, so we need to add one */
> +        g_autoptr(virCommand) addCmd = virCommandNew(TC);
> +
> +        virCommandAddArgList(addCmd, "qdisc", "add", "dev", ifname, "root",
> +                             "handle", "1:", "htb", "default",
> +                             hierarchical_class ? "2" : "1", NULL);
> +
> +        return virCommandRun(addCmd, NULL);
> +    }
> +
> +    return 0;
>  }

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On 11/25/24 5:41 AM, Daniel P. Berrangé wrote:
> On Fri, Nov 22, 2024 at 04:16:37PM -0500, Laine Stump wrote:
>> There will soon be two separate users of tc on virtual networks, and
>> both will use the "qdisc root handle 1: htb" to add tx filters. One or the
>> other could get the first chance to add the qdisc, and then if at a
>> later time the other decides to use it, we need to prevent the 2nd
>> user from attempting to re-add the qdisc (because that just generates
>> an error).
>>
>> We do this by running "tc qdisc show dev $bridge handle 1:" then
>> checking if the output of that command starts with "qdisc htb 1:
>> root". If it does then the qdisc is already there. If not then we need
>> to add it now.
>>
> 
> 
>> diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c
>> index 09c10e9a15..ae7214a9d5 100644
>> --- a/src/util/virnetdevbandwidth.c
>> +++ b/src/util/virnetdevbandwidth.c
> 
>> +
>> +    /* output will be something like: "qdisc htb 1: root refcnt ..."
>> +     * if the qdisc was already added.
>> +     */
>> +    if (!(testResult && STRPREFIX(testResult, "qdisc htb 1: root"))) {
> 
> I wonder a little how stable "tc qdisc show" output format has been
> over time ? This is a very exact match we're performing. Don't
> suppose there's some way to detect this scenario without parsing
> human output from 'tc' ?

I talked to Phil Sutter about this (when I mentioned I was thinking of 
using a bool in the virNetworkObj to keep track of whether or not the 
rule had been added[*] and he suggested parsing the output of "tc qdisc 
show" instead). We both had the same concern as you that the effect of a 
"minor" change to the output format of the command could potentially 
cause failure of that method. He mentioned that iptables had a --check 
option for this reason (it changes the exit code based on existence of a 
rule),but didn't say anything about a similar tc option, and I couldn't 
find any such option in a search of "man tc" (also the exit code of tc 
show doesn't change).

If it would make you more comfortable, I can search for a shorter 
string, and look for it as a substring anywhere within the output rather 
than requiring it to start at character 0 though. How about if I redo it 
looking for just "qdisc" anywhere in the output?

([*] I went most of the way down the "keep track with a bool in the 
virNetworkObj" method, and it turned out to not be foolproof, especially 
when upgrading libvirt or switching from iptables to nftables backend. 
In the end the code would have been much more complicated and it still 
wouldn't have covered 100% of the possibilities)

> 
>> +        /* didn't find qdisc in output, so we need to add one */
>> +        g_autoptr(virCommand) addCmd = virCommandNew(TC);
>> +
>> +        virCommandAddArgList(addCmd, "qdisc", "add", "dev", ifname, "root",
>> +                             "handle", "1:", "htb", "default",
>> +                             hierarchical_class ? "2" : "1", NULL);
>> +
>> +        return virCommandRun(addCmd, NULL);
>> +    }
>> +
>> +    return 0;
>>   }
> 
> With regards,
> Daniel