Re: [libvirt] [PATCH 5/7] virnetdevbandwidth.c: Separate tc filter creation to a function

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(a)redhat.com&gt; > --- > 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?