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
