On 16.04.2015 19:25, Laine Stump wrote:
On 04/14/2015 12:59 PM, Michal Privoznik wrote:
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 | 142 +++++++++++++++++++++++++++++++----------- 1 file changed, 106 insertions(+), 36 deletions(-)
diff --git a/src/util/virnetdevbandwidth.c b/src/util/virnetdevbandwidth.c index 943178b..c57c8c0 100644 --- a/src/util/virnetdevbandwidth.c +++ b/src/util/virnetdevbandwidth.c @@ -43,6 +43,107 @@ 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 + * QDisc acts like black boxes deciding which packets should be
s/QDisc acts/QDiscs act/
+ * held up and which should be sent immediately, it's filter who
s/filter who/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.
I think this would be better as a bullet list of parameters with their purposes, rather than a chummy paragraph of text :-)
+ * + * 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!
Same here. Not that I don't appreciate the nice tone of the conversation, it's just easier to pick out what you need to know from a list than from a paragraph.
+ * + * 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)
How about making these two flags so that it will be easier to tell what's intended when looking at a call to the function?
I was thinking about this too, but then I went with booleans. My idea was that it's shorter this way than inventing new enum items like VIR_NET_DEV_BANDWIDTH_FILTER_CREATE or VIR_NET_DEV_BANDWIDTH_FILTER_REMOVE. But if somebody prefers the other way, I can switch to that. Michal