Having two bools in the arg list is on the borderline of being
confusing to anyone trying to read the code, but we're about to add a
3rd. This patch replaces the two bools with a single flags argument
which will instead have one or more bits from virNetDevBandwidthFlags
set.
Signed-off-by: Laine Stump <laine@redhat.com>
---
src/lxc/lxc_driver.c | 8 ++++++--
src/lxc/lxc_process.c | 8 ++++++--
src/network/bridge_driver.c | 10 ++++++++--
src/qemu/qemu_command.c | 11 ++++++++---
src/qemu/qemu_driver.c | 29 ++++++++++++++-------------
src/qemu/qemu_hotplug.c | 22 +++++++++++++++------
src/util/virnetdevbandwidth.c | 36 ++++++++++++++++++++--------------
src/util/virnetdevbandwidth.h | 11 ++++++++---
tests/virnetdevbandwidthtest.c | 8 +++++++-
9 files changed, 95 insertions(+), 48 deletions(-)
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 0e31e5e4b9..a64cfef1a0 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -3570,8 +3570,12 @@ lxcDomainAttachDeviceNetLive(virLXCDriver *driver,
actualBandwidth = virDomainNetGetActualBandwidth(net);
if (actualBandwidth) {
if (virNetDevSupportsBandwidth(actualType)) {
- if (virNetDevBandwidthSet(net->ifname, actualBandwidth, false,
- !virDomainNetTypeSharesHostView(net)) < 0)
+ unsigned int flags = 0;
+
+ if (!virDomainNetTypeSharesHostView(net))
+ flags |= VIR_NETDEV_BANDWIDTH_SET_DIR_SWAPPED;
+
+ if (virNetDevBandwidthSet(net->ifname, actualBandwidth, flags) < 0)
goto cleanup;
} else {
VIR_WARN("setting bandwidth on interfaces of "
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index 083ab83ec6..7eba7a2c46 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -609,8 +609,12 @@ virLXCProcessSetupInterfaces(virLXCDriver *driver,
actualBandwidth = virDomainNetGetActualBandwidth(net);
if (actualBandwidth) {
if (virNetDevSupportsBandwidth(type)) {
- if (virNetDevBandwidthSet(net->ifname, actualBandwidth, false,
- !virDomainNetTypeSharesHostView(net)) < 0)
+ unsigned int flags = 0;
+
+ if (!virDomainNetTypeSharesHostView(net))
+ flags |= VIR_NETDEV_BANDWIDTH_SET_DIR_SWAPPED;
+
+ if (virNetDevBandwidthSet(net->ifname, actualBandwidth, flags) < 0)
goto cleanup;
} else {
VIR_WARN("setting bandwidth on interfaces of "
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index d408f17de7..e700a614a9 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -2109,8 +2109,11 @@ networkStartNetworkVirtual(virNetworkDriverState *driver,
}
}
- if (virNetDevBandwidthSet(def->bridge, def->bandwidth, true, true) < 0)
+ if (virNetDevBandwidthSet(def->bridge, def->bandwidth,
+ VIR_NETDEV_BANDWIDTH_SET_HIERARCHICAL_CLASS
+ | VIR_NETDEV_BANDWIDTH_SET_DIR_SWAPPED) < 0) {
goto error;
+ }
return 0;
@@ -2190,8 +2193,11 @@ networkStartNetworkBridge(virNetworkObj *obj)
* type BRIDGE, is started. On failure, undo anything you've done,
* and return -1. On success return 0.
*/
- if (virNetDevBandwidthSet(def->bridge, def->bandwidth, true, true) < 0)
+ if (virNetDevBandwidthSet(def->bridge, def->bandwidth,
+ VIR_NETDEV_BANDWIDTH_SET_HIERARCHICAL_CLASS
+ | VIR_NETDEV_BANDWIDTH_SET_DIR_SWAPPED) < 0) {
goto error;
+ }
if (networkStartHandleMACTableManagerMode(obj) < 0)
goto error;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index f4430275dc..3d0a5dd460 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -8694,9 +8694,14 @@ qemuBuildInterfaceCommandLine(virQEMUDriver *driver,
def->uuid,
!virDomainNetTypeSharesHostView(net)) < 0)
goto cleanup;
- } else if (virNetDevBandwidthSet(net->ifname, actualBandwidth, false,
- !virDomainNetTypeSharesHostView(net)) < 0) {
- goto cleanup;
+ } else {
+ unsigned int flags = 0;
+
+ if (!virDomainNetTypeSharesHostView(net))
+ flags |= VIR_NETDEV_BANDWIDTH_SET_DIR_SWAPPED;
+
+ if (virNetDevBandwidthSet(net->ifname, actualBandwidth, flags) < 0)
+ goto cleanup;
}
} else {
VIR_WARN("setting bandwidth on interfaces of "
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 5b9c55f704..425d583e99 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -9938,21 +9938,22 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom,
virErrorRestore(&orig_err);
goto endjob;
}
- } else if (virNetDevBandwidthSet(net->ifname, newBandwidth, false,
- !virDomainNetTypeSharesHostView(net)) < 0) {
- virErrorPtr orig_err;
-
- virErrorPreserveLast(&orig_err);
- ignore_value(virNetDevBandwidthSet(net->ifname,
- net->bandwidth,
- false,
- !virDomainNetTypeSharesHostView(net)));
- if (net->bandwidth) {
- ignore_value(virDomainNetBandwidthUpdate(net,
- net->bandwidth));
+ } else {
+ unsigned int bwflags = 0;
+
+ if (!virDomainNetTypeSharesHostView(net))
+ bwflags |= VIR_NETDEV_BANDWIDTH_SET_DIR_SWAPPED;
+
+ if (virNetDevBandwidthSet(net->ifname, newBandwidth, bwflags) < 0) {
+ virErrorPtr orig_err;
+
+ virErrorPreserveLast(&orig_err);
+ ignore_value(virNetDevBandwidthSet(net->ifname, net->bandwidth, bwflags));
+ if (net->bandwidth)
+ ignore_value(virDomainNetBandwidthUpdate(net, net->bandwidth));
+ virErrorRestore(&orig_err);
+ goto endjob;
}
- virErrorRestore(&orig_err);
- goto endjob;
}
/* If the old bandwidth was cleared out, restore qdisc. */
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 55512476e4..b06ae61a44 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1331,9 +1331,14 @@ qemuDomainAttachNetDevice(virQEMUDriver *driver,
vm->def->uuid,
!virDomainNetTypeSharesHostView(net)) < 0)
goto cleanup;
- } else if (virNetDevBandwidthSet(net->ifname, actualBandwidth, false,
- !virDomainNetTypeSharesHostView(net)) < 0) {
- goto cleanup;
+ } else {
+ int flags = 0;
+
+ if (!virDomainNetTypeSharesHostView(net))
+ flags |= VIR_NETDEV_BANDWIDTH_SET_DIR_SWAPPED;
+
+ if (virNetDevBandwidthSet(net->ifname, actualBandwidth, flags) < 0)
+ goto cleanup;
}
} else {
VIR_WARN("setting bandwidth on interfaces of "
@@ -4181,9 +4186,14 @@ qemuDomainChangeNet(virQEMUDriver *driver,
vm->def->uuid,
!virDomainNetTypeSharesHostView(newdev)) < 0)
goto cleanup;
- } else if (virNetDevBandwidthSet(newdev->ifname, newb, false,
- !virDomainNetTypeSharesHostView(newdev)) < 0) {
- goto cleanup;
+ } else {
+ int flags = 0;
+
+ if (!virDomainNetTypeSharesHostView(newdev))
+ flags |= VIR_NETDEV_BANDWIDTH_SET_DIR_SWAPPED;
+
+ if (virNetDevBandwidthSet(newdev->ifname, newb, flags) < 0)
+ goto cleanup;
}
} else {
if (virDomainInterfaceClearQoS(vm->def, olddev) < 0)
diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c
index 2b58c58d3e..1baad849c6 100644
--- a/src/util/virnetdevbandwidth.c
+++ b/src/util/virnetdevbandwidth.c
@@ -173,30 +173,35 @@ virNetDevBandwidthManipulateFilter(const char *ifname,
* virNetDevBandwidthSet:
* @ifname: on which interface
* @bandwidth: rates to set (may be NULL)
- * @hierarchical_class: whether to create hierarchical class
- * @swapped: true if IN/OUT should be set contrariwise
+ * @flags: bits indicating certain optional actions
*
+
* This function enables QoS on specified interface
* and set given traffic limits for both, incoming
- * and outgoing traffic. Any previous setting get
- * overwritten. If @hierarchical_class is TRUE, create
- * hierarchical class. It is used to guarantee minimal
- * throughput ('floor' attribute in NIC).
+ * and outgoing traffic.
+ *
+ * @flags bits and their meanings:
+ *
+ * VIR_NETDEV_BANDWIDTH_SET_HIERARCHICAL_CLASS
+ * whether to create a hierarchical class
+ * A hiearchical class structure is used to implement a minimal
+ * throughput guarantee ('floor' attribute in NIC).
*
- * If @swapped is set, the IN part of @bandwidth is set on
- * @ifname's TX, and vice versa. If it is not set, IN is set on
- * RX and OUT on TX. This is because for some types of interfaces
- * domain and the host live on the same side of the interface (so
- * domain's RX/TX is host's RX/TX), and for some it's swapped
- * (domain's RX/TX is hosts's TX/RX).
+ * VIR_NETDEV_BANDWIDTH_SET_DIR_SWAPPED
+ * set if IN/OUT should be set backwards from what's indicated in
+ * the bandwidth, i.e. the IN part of @bandwidth is set on
+ * @ifname's TX, and the OUT part of @bandwidth is set on
+ * @ifname's RX. This is needed because for some types of
+ * interfaces the domain and the host live on the same side of the
+ * interface (so domain's RX/TX is host's RX/TX), and for some
+ * it's swapped (domain's RX/TX is hosts's TX/RX).
*
* Return 0 on success, -1 otherwise.
*/
int
virNetDevBandwidthSet(const char *ifname,
const virNetDevBandwidth *bandwidth,
- bool hierarchical_class,
- bool swapped)
+ unsigned int flags)
{
int ret = -1;
virNetDevBandwidthRate *rx = NULL; /* From domain POV */
@@ -205,6 +210,7 @@ virNetDevBandwidthSet(const char *ifname,
char *average = NULL;
char *peak = NULL;
char *burst = NULL;
+ bool hierarchical_class = flags & VIR_NETDEV_BANDWIDTH_SET_HIERARCHICAL_CLASS;
if (!bandwidth) {
/* nothing to be enabled */
@@ -224,7 +230,7 @@ virNetDevBandwidthSet(const char *ifname,
return -1;
}
- if (swapped) {
+ if (flags & VIR_NETDEV_BANDWIDTH_SET_DIR_SWAPPED) {
rx = bandwidth->out;
tx = bandwidth->in;
} else {
diff --git a/src/util/virnetdevbandwidth.h b/src/util/virnetdevbandwidth.h
index 6d268fb119..414d6c97eb 100644
--- a/src/util/virnetdevbandwidth.h
+++ b/src/util/virnetdevbandwidth.h
@@ -35,15 +35,20 @@ struct _virNetDevBandwidth {
virNetDevBandwidthRate *out;
};
-void virNetDevBandwidthFree(virNetDevBandwidth *def);
+ void virNetDevBandwidthFree(virNetDevBandwidth *def);
G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNetDevBandwidth, virNetDevBandwidthFree);
+typedef enum {
+ VIR_NETDEV_BANDWIDTH_SET_HIERARCHICAL_CLASS = (1 << 0),
+ VIR_NETDEV_BANDWIDTH_SET_DIR_SWAPPED = (1 << 1),
+} virNetDevBandwidthSetFlags;
+
int virNetDevBandwidthSet(const char *ifname,
const virNetDevBandwidth *bandwidth,
- bool hierarchical_class,
- bool swapped)
+ unsigned int flags)
G_GNUC_WARN_UNUSED_RESULT;
+
int virNetDevBandwidthClear(const char *ifname);
int virNetDevBandwidthCopy(virNetDevBandwidth **dest,
const virNetDevBandwidth *src)
diff --git a/tests/virnetdevbandwidthtest.c b/tests/virnetdevbandwidthtest.c
index f7c38faa2e..3a87d876c8 100644
--- a/tests/virnetdevbandwidthtest.c
+++ b/tests/virnetdevbandwidthtest.c
@@ -82,8 +82,14 @@ testVirNetDevBandwidthSet(const void *data)
if (virNetDevOpenvswitchInterfaceSetQos(iface, band, info->uuid, true) < 0)
return -1;
} else {
+ int flags = (VIR_NETDEV_BANDWIDTH_SET_DIR_SWAPPED);
+
+ if (info->hierarchical_class)
+ flags |= VIR_NETDEV_BANDWIDTH_SET_HIERARCHICAL_CLASS;
+
exp_cmd = info->exp_cmd_tc;
- if (virNetDevBandwidthSet(iface, band, info->hierarchical_class, true) < 0)
+
+ if (virNetDevBandwidthSet(iface, band, flags) < 0)
return -1;
}
--
2.47.0On 11/26/24 04:24, Laine Stump wrote:
> Having two bools in the arg list is on the borderline of being
> confusing to anyone trying to read the code, but we're about to add a
> 3rd. This patch replaces the two bools with a single flags argument
> which will instead have one or more bits from virNetDevBandwidthFlags
> set.
>
> Signed-off-by: Laine Stump <laine@redhat.com>
> ---
> src/lxc/lxc_driver.c | 8 ++++++--
> src/lxc/lxc_process.c | 8 ++++++--
> src/network/bridge_driver.c | 10 ++++++++--
> src/qemu/qemu_command.c | 11 ++++++++---
> src/qemu/qemu_driver.c | 29 ++++++++++++++-------------
> src/qemu/qemu_hotplug.c | 22 +++++++++++++++------
> src/util/virnetdevbandwidth.c | 36 ++++++++++++++++++++--------------
> src/util/virnetdevbandwidth.h | 11 ++++++++---
> tests/virnetdevbandwidthtest.c | 8 +++++++-
> 9 files changed, 95 insertions(+), 48 deletions(-)
Squash this in:
diff --git i/src/util/virnetdevbandwidth.h w/src/util/virnetdevbandwidth.h
index 414d6c97eb..80dc654486 100644
--- i/src/util/virnetdevbandwidth.h
+++ w/src/util/virnetdevbandwidth.h
@@ -35,7 +35,7 @@ struct _virNetDevBandwidth {
virNetDevBandwidthRate *out;
};
- void virNetDevBandwidthFree(virNetDevBandwidth *def);
+void virNetDevBandwidthFree(virNetDevBandwidth *def);
G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNetDevBandwidth, virNetDevBandwidthFree);
diff --git i/tests/virnetdevbandwidthtest.c w/tests/virnetdevbandwidthtest.c
index 3a87d876c8..6529ff4026 100644
--- i/tests/virnetdevbandwidthtest.c
+++ w/tests/virnetdevbandwidthtest.c
@@ -82,7 +82,7 @@ testVirNetDevBandwidthSet(const void *data)
if (virNetDevOpenvswitchInterfaceSetQos(iface, band, info->uuid, true) < 0)
return -1;
} else {
- int flags = (VIR_NETDEV_BANDWIDTH_SET_DIR_SWAPPED);
+ unsigned int flags = VIR_NETDEV_BANDWIDTH_SET_DIR_SWAPPED;
if (info->hierarchical_class)
flags |= VIR_NETDEV_BANDWIDTH_SET_HIERARCHICAL_CLASS;
Michal
On 11/26/24 7:18 AM, Michal Prívozník wrote:
> On 11/26/24 04:24, Laine Stump wrote:
>> Having two bools in the arg list is on the borderline of being
>> confusing to anyone trying to read the code, but we're about to add a
>> 3rd. This patch replaces the two bools with a single flags argument
>> which will instead have one or more bits from virNetDevBandwidthFlags
>> set.
>>
>> Signed-off-by: Laine Stump <laine@redhat.com>
>> ---
>> src/lxc/lxc_driver.c | 8 ++++++--
>> src/lxc/lxc_process.c | 8 ++++++--
>> src/network/bridge_driver.c | 10 ++++++++--
>> src/qemu/qemu_command.c | 11 ++++++++---
>> src/qemu/qemu_driver.c | 29 ++++++++++++++-------------
>> src/qemu/qemu_hotplug.c | 22 +++++++++++++++------
>> src/util/virnetdevbandwidth.c | 36 ++++++++++++++++++++--------------
>> src/util/virnetdevbandwidth.h | 11 ++++++++---
>> tests/virnetdevbandwidthtest.c | 8 +++++++-
>> 9 files changed, 95 insertions(+), 48 deletions(-)
>
>
> Squash this in:
>
> diff --git i/src/util/virnetdevbandwidth.h w/src/util/virnetdevbandwidth.h
> index 414d6c97eb..80dc654486 100644
> --- i/src/util/virnetdevbandwidth.h
> +++ w/src/util/virnetdevbandwidth.h
> @@ -35,7 +35,7 @@ struct _virNetDevBandwidth {
> virNetDevBandwidthRate *out;
> };
>
> - void virNetDevBandwidthFree(virNetDevBandwidth *def);
> +void virNetDevBandwidthFree(virNetDevBandwidth *def);
>
> G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNetDevBandwidth, virNetDevBandwidthFree);
>
> diff --git i/tests/virnetdevbandwidthtest.c w/tests/virnetdevbandwidthtest.c
> index 3a87d876c8..6529ff4026 100644
> --- i/tests/virnetdevbandwidthtest.c
> +++ w/tests/virnetdevbandwidthtest.c
> @@ -82,7 +82,7 @@ testVirNetDevBandwidthSet(const void *data)
> if (virNetDevOpenvswitchInterfaceSetQos(iface, band, info->uuid, true) < 0)
> return -1;
> } else {
> - int flags = (VIR_NETDEV_BANDWIDTH_SET_DIR_SWAPPED);
> + unsigned int flags = VIR_NETDEV_BANDWIDTH_SET_DIR_SWAPPED;
>
Huh. I thought I had caught all of those, but I guess not. (I'd
originally made them all just "int", but the syntax-check stuff pointed
out that any arg to a function named "flags" should be unsigned)
© 2016 - 2024 Red Hat, Inc.