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

[Michal Privoznik]

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 at 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