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@xxxxxxxxxx> >> --- >> 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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list