From: Daniel P. Berrangé <berrange@redhat.com>
Allow virCommand to find 'tc' in $PATH. This command is only used
when running privileged in which case both 'bin' and 'sbin' dirs will
be in $PATH, so virFindFileInPath will do the right thing to find it.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
meson.build | 10 ++----
src/util/virfirewall.h | 1 +
src/util/virnetdevbandwidth.c | 36 ++++++++++-----------
tests/virnetdevbandwidthtest.c | 58 +++++++++++++++++-----------------
4 files changed, 50 insertions(+), 55 deletions(-)
diff --git a/meson.build b/meson.build
index 0515820cdd..4223f5ab86 100644
--- a/meson.build
+++ b/meson.build
@@ -845,7 +845,7 @@ endforeach
# optional programs
-optional_test_programs = [
+optional_programs = [
'augparse',
'black',
'flake8',
@@ -853,10 +853,6 @@ optional_test_programs = [
'pytest',
]
-optional_programs = [
- 'tc',
-] + optional_test_programs
-
missing_optional_programs = []
foreach name : optional_programs
prog = find_program(name, required: false, dirs: libvirt_sbin_path)
@@ -865,9 +861,7 @@ foreach name : optional_programs
prog_path = prog.full_path()
else
prog_path = name
- if name in optional_test_programs
- missing_optional_programs += [ name ]
- endif
+ missing_optional_programs += [ name ]
endif
conf.set_quoted(varname.to_upper(), prog_path)
diff --git a/src/util/virfirewall.h b/src/util/virfirewall.h
index d42e60884b..07391bea67 100644
--- a/src/util/virfirewall.h
+++ b/src/util/virfirewall.h
@@ -30,6 +30,7 @@
#define IPTABLES "iptables"
#define IP6TABLES "ip6tables"
#define NFT "nft"
+#define TC "tc"
typedef struct _virFirewall virFirewall;
diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c
index 5c6a65528c..b141a38b10 100644
--- a/src/util/virnetdevbandwidth.c
+++ b/src/util/virnetdevbandwidth.c
@@ -126,7 +126,7 @@ virNetDevBandwidthManipulateFilter(const char *ifname,
filter_id = g_strdup_printf("800::%u", 800 + id);
if (remove_old) {
- g_autoptr(virCommand) cmd = virCommandNew(TC);
+ g_autoptr(virCommand) cmd = virCommandNew("tc");
int cmd_ret = 0;
virCommandAddArgList(cmd, "filter", "del", "dev", ifname,
@@ -138,7 +138,7 @@ virNetDevBandwidthManipulateFilter(const char *ifname,
}
if (create_new) {
- g_autoptr(virCommand) cmd = virCommandNew(TC);
+ g_autoptr(virCommand) cmd = virCommandNew("tc");
virMacAddrGetRaw(ifmac_ptr, ifmac);
mac[0] = g_strdup_printf("0x%02x%02x%02x%02x", ifmac[2],
@@ -334,7 +334,7 @@ virNetDevBandwidthSet(const char *ifname,
*/
if (hierarchical_class) {
virCommandFree(cmd);
- cmd = virCommandNew(TC);
+ cmd = virCommandNew("tc");
virCommandAddArgList(cmd, "class", "add", "dev", ifname, "parent",
"1:", "classid", "1:1", "htb", "rate", average,
"ceil", peak ? peak : average, NULL);
@@ -343,7 +343,7 @@ virNetDevBandwidthSet(const char *ifname,
goto cleanup;
}
virCommandFree(cmd);
- cmd = virCommandNew(TC);
+ cmd = virCommandNew("tc");
virCommandAddArgList(cmd, "class", "add", "dev", ifname, "parent",
hierarchical_class ? "1:1" : "1:", "classid",
hierarchical_class ? "1:2" : "1:1", "htb",
@@ -359,7 +359,7 @@ virNetDevBandwidthSet(const char *ifname,
goto cleanup;
virCommandFree(cmd);
- cmd = virCommandNew(TC);
+ cmd = virCommandNew("tc");
virCommandAddArgList(cmd, "qdisc", "add", "dev", ifname, "parent",
hierarchical_class ? "1:2" : "1:1",
"handle", "2:", "sfq", "perturb",
@@ -369,7 +369,7 @@ virNetDevBandwidthSet(const char *ifname,
goto cleanup;
virCommandFree(cmd);
- cmd = virCommandNew(TC);
+ cmd = virCommandNew("tc");
virCommandAddArgList(cmd, "filter", "add", "dev", ifname, "parent",
"1:0", "protocol", "all", "prio", "1", "handle",
"1", "fw", "flowid", "1", NULL);
@@ -397,7 +397,7 @@ virNetDevBandwidthSet(const char *ifname,
}
virCommandFree(cmd);
- cmd = virCommandNew(TC);
+ cmd = virCommandNew("tc");
virCommandAddArgList(cmd, "qdisc", "add", "dev", ifname,
"ingress", NULL);
@@ -405,7 +405,7 @@ virNetDevBandwidthSet(const char *ifname,
goto cleanup;
virCommandFree(cmd);
- cmd = virCommandNew(TC);
+ cmd = virCommandNew("tc");
/* Set filter to match all ingress traffic */
virCommandAddArgList(cmd, "filter", "add", "dev", ifname, "parent",
"ffff:", "protocol", "all", "u32", "match", "u32",
@@ -448,13 +448,13 @@ virNetDevBandwidthClear(const char *ifname)
if (!ifname)
return 0;
- rootcmd = virCommandNew(TC);
+ rootcmd = virCommandNew("tc");
virCommandAddArgList(rootcmd, "qdisc", "del", "dev", ifname, "root", NULL);
if (virCommandRun(rootcmd, &dummy) < 0)
ret = -1;
- ingresscmd = virCommandNew(TC);
+ ingresscmd = virCommandNew("tc");
virCommandAddArgList(ingresscmd, "qdisc", "del", "dev", ifname, "ingress", NULL);
if (virCommandRun(ingresscmd, &dummy) < 0)
@@ -593,7 +593,7 @@ virNetDevBandwidthPlug(const char *brname,
net_bandwidth->in->peak :
net_bandwidth->in->average);
- cmd1 = virCommandNew(TC);
+ cmd1 = virCommandNew("tc");
virCommandAddArgList(cmd1, "class", "add", "dev", brname, "parent", "1:1",
"classid", class_id, "htb", "rate", floor,
"ceil", ceil, NULL);
@@ -602,7 +602,7 @@ virNetDevBandwidthPlug(const char *brname,
if (virCommandRun(cmd1, NULL) < 0)
return -1;
- cmd2 = virCommandNew(TC);
+ cmd2 = virCommandNew("tc");
virCommandAddArgList(cmd2, "qdisc", "add", "dev", brname, "parent",
class_id, "handle", qdisc_id, "sfq", "perturb",
"10", NULL);
@@ -644,7 +644,7 @@ virNetDevBandwidthUnplug(const char *brname,
class_id = g_strdup_printf("1:%x", id);
qdisc_id = g_strdup_printf("%x:", id);
- cmd1 = virCommandNew(TC);
+ cmd1 = virCommandNew("tc");
virCommandAddArgList(cmd1, "qdisc", "del", "dev", brname,
"handle", qdisc_id, NULL);
@@ -657,7 +657,7 @@ virNetDevBandwidthUnplug(const char *brname,
NULL, true, false) < 0)
return -1;
- cmd2 = virCommandNew(TC);
+ cmd2 = virCommandNew("tc");
virCommandAddArgList(cmd2, "class", "del", "dev", brname,
"classid", class_id, NULL);
@@ -698,7 +698,7 @@ virNetDevBandwidthUpdateRate(const char *ifname,
bandwidth->in->peak :
bandwidth->in->average);
- cmd = virCommandNew(TC);
+ cmd = virCommandNew("tc");
virCommandAddArgList(cmd, "class", "change", "dev", ifname,
"classid", class_id, "htb", "rate", rate,
"ceil", ceil, NULL);
@@ -772,7 +772,7 @@ virNetDevBandwidthSetRootQDisc(const char *ifname,
/* Ideally, we would have a netlink implementation and just
* call it here. But honestly, I tried and failed miserably.
* Fallback to spawning tc. */
- cmd = virCommandNewArgList(TC, "qdisc", "add", "dev", ifname,
+ cmd = virCommandNewArgList("tc", "qdisc", "add", "dev", ifname,
"root", "handle", "0:", qdisc,
NULL);
@@ -811,7 +811,7 @@ virNetDevBandWidthAddTxFilterParentQdisc(const char *ifname,
/* first check it the qdisc with handle 1: was already added for
* this interface by someone else
*/
- testCmd = virCommandNew(TC);
+ testCmd = virCommandNew("tc");
virCommandAddArgList(testCmd, "qdisc", "show", "dev", ifname,
"handle", "1:", NULL);
virCommandSetOutputBuffer(testCmd, &testResult);
@@ -826,7 +826,7 @@ virNetDevBandWidthAddTxFilterParentQdisc(const char *ifname,
*/
if (!(testResult && strstr(testResult, "qdisc") && strstr(testResult, " 1: "))) {
/* didn't find qdisc in output, so we need to add one */
- g_autoptr(virCommand) addCmd = virCommandNew(TC);
+ g_autoptr(virCommand) addCmd = virCommandNew("tc");
virCommandAddArgList(addCmd, "qdisc", "add", "dev", ifname, "root",
"handle", "1:", "htb", "default",
diff --git a/tests/virnetdevbandwidthtest.c b/tests/virnetdevbandwidthtest.c
index b7cf546d9f..9624da9b96 100644
--- a/tests/virnetdevbandwidthtest.c
+++ b/tests/virnetdevbandwidthtest.c
@@ -145,13 +145,13 @@ mymain(void)
DO_TEST_SET("<bandwidth>"
" <inbound average='1024'/>"
"</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"
- TC " filter add dev eth0 parent 1:0 protocol all prio 1 handle 1 fw flowid 1\n",
+ "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"
+ "tc filter add dev eth0 parent 1:0 protocol all prio 1 handle 1 fw flowid 1\n",
"ovs-vsctl --timeout=5 --no-heading --columns=_uuid find queue 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n"
"ovs-vsctl --timeout=5 --no-heading --columns=_uuid find qos 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n"
"ovs-vsctl --timeout=5 set port eth0 qos=@qos1 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"' --"
@@ -163,10 +163,10 @@ mymain(void)
DO_TEST_SET("<bandwidth>"
" <outbound average='1024'/>"
"</bandwidth>",
- TC " qdisc del dev eth0 root\n"
- TC " qdisc del dev eth0 ingress\n"
- TC " qdisc add dev eth0 ingress\n"
- TC " filter add dev eth0 parent ffff: protocol all u32 match u32 0 0"
+ "tc qdisc del dev eth0 root\n"
+ "tc qdisc del dev eth0 ingress\n"
+ "tc qdisc add dev eth0 ingress\n"
+ "tc filter add dev eth0 parent ffff: protocol all u32 match u32 0 0"
" police rate 1024kbps burst 1024kb mtu 64kb drop flowid :1\n",
"ovs-vsctl --timeout=5 --no-heading --columns=_uuid find queue 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n"
"ovs-vsctl --timeout=5 --no-heading --columns=_uuid find qos 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n"
@@ -176,15 +176,15 @@ mymain(void)
" <inbound average='1' peak='2' floor='3' burst='4'/>"
" <outbound average='5' peak='6' burst='7'/>"
"</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"
- TC " filter add dev eth0 parent 1:0 protocol all prio 1 handle 1 fw flowid 1\n"
- TC " qdisc add dev eth0 ingress\n"
- TC " filter add dev eth0 parent ffff: protocol all u32 match u32 0 0"
+ "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"
+ "tc filter add dev eth0 parent 1:0 protocol all prio 1 handle 1 fw flowid 1\n"
+ "tc qdisc add dev eth0 ingress\n"
+ "tc filter add dev eth0 parent ffff: protocol all u32 match u32 0 0"
" police rate 5kbps burst 7kb mtu 64kb drop flowid :1\n",
"ovs-vsctl --timeout=5 --no-heading --columns=_uuid find queue 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n"
"ovs-vsctl --timeout=5 --no-heading --columns=_uuid find qos 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n"
@@ -199,15 +199,15 @@ mymain(void)
" <inbound average='4294967295'/>"
" <outbound average='4294967295'/>"
"</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"
- TC " filter add dev eth0 parent 1:0 protocol all prio 1 handle 1 fw flowid 1\n"
- TC " qdisc add dev eth0 ingress\n"
- TC " filter add dev eth0 parent ffff: protocol all u32 match"
+ "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"
+ "tc filter add dev eth0 parent 1:0 protocol all prio 1 handle 1 fw flowid 1\n"
+ "tc qdisc add dev eth0 ingress\n"
+ "tc filter add dev eth0 parent ffff: protocol all u32 match"
" u32 0 0 police rate 4294967295kbps burst 4194303kb mtu 64kb"
" drop flowid :1\n",
"ovs-vsctl --timeout=5 --no-heading --columns=_uuid find queue 'external-ids:vm-id=\"" VMUUID "\"' 'external-ids:ifname=\"eth0\"'\n"
--
2.49.0On Tue, Apr 29, 2025 at 12:14:16 +0100, Daniel P. Berrangé via Devel wrote: > From: Daniel P. Berrangé <berrange@redhat.com> > > Allow virCommand to find 'tc' in $PATH. This command is only used > when running privileged in which case both 'bin' and 'sbin' dirs will > be in $PATH, so virFindFileInPath will do the right thing to find it. > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > --- > meson.build | 10 ++---- > src/util/virfirewall.h | 1 + > src/util/virnetdevbandwidth.c | 36 ++++++++++----------- > tests/virnetdevbandwidthtest.c | 58 +++++++++++++++++----------------- > 4 files changed, 50 insertions(+), 55 deletions(-) > > diff --git a/meson.build b/meson.build > index 0515820cdd..4223f5ab86 100644 > --- a/meson.build > +++ b/meson.build > @@ -845,7 +845,7 @@ endforeach > > # optional programs > > -optional_test_programs = [ > +optional_programs = [ > 'augparse', > 'black', > 'flake8', > @@ -853,10 +853,6 @@ optional_test_programs = [ > 'pytest', > ] > > -optional_programs = [ > - 'tc', > -] + optional_test_programs > - > missing_optional_programs = [] > foreach name : optional_programs > prog = find_program(name, required: false, dirs: libvirt_sbin_path) > @@ -865,9 +861,7 @@ foreach name : optional_programs > prog_path = prog.full_path() > else > prog_path = name > - if name in optional_test_programs > - missing_optional_programs += [ name ] > - endif > + missing_optional_programs += [ name ] > endif I had to dig a bit what happened here. Please mention in the commit message that as there are no optional programs for runtime we're left with optional programs for testing (which are also moaned about in the configure output). > > conf.set_quoted(varname.to_upper(), prog_path) > diff --git a/src/util/virfirewall.h b/src/util/virfirewall.h > index d42e60884b..07391bea67 100644 > --- a/src/util/virfirewall.h > +++ b/src/util/virfirewall.h > @@ -30,6 +30,7 @@ > #define IPTABLES "iptables" > #define IP6TABLES "ip6tables" > #define NFT "nft" > +#define TC "tc" There are just two places referencing the TC macro after this. Do we still need it? src/util/virfirewall.c=VIR_ENUM_IMPL(virFirewallLayerCommand, src/util/virfirewall.c: TC, src/util/virfirewall.c=virFirewallCmdNftablesApply(virFirewall *firewall G_GNUC_UNUSED, src/util/virfirewall.c: cmd = virCommandNew(TC); The rest looks good. If you justify/fix the thing above and mention the build system change in the commit message: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
On Tue, Apr 29, 2025 at 02:38:45PM +0200, Peter Krempa wrote: > On Tue, Apr 29, 2025 at 12:14:16 +0100, Daniel P. Berrangé via Devel wrote: > > From: Daniel P. Berrangé <berrange@redhat.com> > > > > Allow virCommand to find 'tc' in $PATH. This command is only used > > when running privileged in which case both 'bin' and 'sbin' dirs will > > be in $PATH, so virFindFileInPath will do the right thing to find it. > > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > > --- > > meson.build | 10 ++---- > > src/util/virfirewall.h | 1 + > > src/util/virnetdevbandwidth.c | 36 ++++++++++----------- > > tests/virnetdevbandwidthtest.c | 58 +++++++++++++++++----------------- > > 4 files changed, 50 insertions(+), 55 deletions(-) > > > > diff --git a/src/util/virfirewall.h b/src/util/virfirewall.h > > index d42e60884b..07391bea67 100644 > > --- a/src/util/virfirewall.h > > +++ b/src/util/virfirewall.h > > @@ -30,6 +30,7 @@ > > #define IPTABLES "iptables" > > #define IP6TABLES "ip6tables" > > #define NFT "nft" > > +#define TC "tc" > > There are just two places referencing the TC macro after this. Do we > still need it? > > src/util/virfirewall.c=VIR_ENUM_IMPL(virFirewallLayerCommand, > src/util/virfirewall.c: TC, > src/util/virfirewall.c=virFirewallCmdNftablesApply(virFirewall *firewall G_GNUC_UNUSED, > src/util/virfirewall.c: cmd = virCommandNew(TC); I was borderline on this - in the end since we have NFT/IPTABLES/IP6TABLES constants I went for consistency within virfirewall.c 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 Tue, Apr 29, 2025 at 13:40:31 +0100, Daniel P. Berrangé wrote: > On Tue, Apr 29, 2025 at 02:38:45PM +0200, Peter Krempa wrote: > > On Tue, Apr 29, 2025 at 12:14:16 +0100, Daniel P. Berrangé via Devel wrote: > > > From: Daniel P. Berrangé <berrange@redhat.com> > > > > > > Allow virCommand to find 'tc' in $PATH. This command is only used > > > when running privileged in which case both 'bin' and 'sbin' dirs will > > > be in $PATH, so virFindFileInPath will do the right thing to find it. > > > > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > > > --- > > > meson.build | 10 ++---- > > > src/util/virfirewall.h | 1 + > > > src/util/virnetdevbandwidth.c | 36 ++++++++++----------- > > > tests/virnetdevbandwidthtest.c | 58 +++++++++++++++++----------------- > > > 4 files changed, 50 insertions(+), 55 deletions(-) > > > > > > > diff --git a/src/util/virfirewall.h b/src/util/virfirewall.h > > > index d42e60884b..07391bea67 100644 > > > --- a/src/util/virfirewall.h > > > +++ b/src/util/virfirewall.h > > > @@ -30,6 +30,7 @@ > > > #define IPTABLES "iptables" > > > #define IP6TABLES "ip6tables" > > > #define NFT "nft" > > > +#define TC "tc" > > > > There are just two places referencing the TC macro after this. Do we > > still need it? > > > > src/util/virfirewall.c=VIR_ENUM_IMPL(virFirewallLayerCommand, > > src/util/virfirewall.c: TC, > > src/util/virfirewall.c=virFirewallCmdNftablesApply(virFirewall *firewall G_GNUC_UNUSED, > > src/util/virfirewall.c: cmd = virCommandNew(TC); > > I was borderline on this - in the end since we have NFT/IPTABLES/IP6TABLES > constants I went for consistency within virfirewall.c Fair enough keep it as is.
© 2016 - 2025 Red Hat, Inc.