net: walk through filters reversely if traffic is outgress

Previously, if the netdev has more than one filters, the ingress
or outgress traffic pass the filter in the same order. this patch
is to make the outgress pass the filter in a reverse order

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
---
 include/net/net.h |  4 +++-
 net/filter.c      | 21 +++++++++++++++++++--
 net/net.c         | 23 ++++++++++++++++++-----
 3 files changed, 40 insertions(+), 8 deletions(-)

On 01/22/2016 04:11 PM, Li Zhijian wrote:
> Previously, if the netdev has more than one filters, the ingress
> or outgress traffic pass the filter in the same order. this patch
> is to make the outgress pass the filter in a reverse order

Need a description why we need this.

>
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> ---
>  include/net/net.h |  4 +++-
>  net/filter.c      | 21 +++++++++++++++++++--
>  net/net.c         | 23 ++++++++++++++++++-----
>  3 files changed, 40 insertions(+), 8 deletions(-)
>
> diff --git a/include/net/net.h b/include/net/net.h
> index 7af3e15..1d807cc 100644
> --- a/include/net/net.h
> +++ b/include/net/net.h
> @@ -79,6 +79,8 @@ typedef struct NetClientInfo {
>      SetVnetBE *set_vnet_be;
>  } NetClientInfo;
>  
> +QTAILQ_HEAD(NetFilterHead, NetFilterState);
> +
>  struct NetClientState {
>      NetClientInfo *info;
>      int link_down;
> @@ -92,7 +94,7 @@ struct NetClientState {
>      NetClientDestructor *destructor;
>      unsigned int queue_index;
>      unsigned rxfilter_notify_enabled:1;
> -    QTAILQ_HEAD(, NetFilterState) filters;
> +    struct NetFilterHead filters;
>  };
>  
>  typedef struct NICState {
> diff --git a/net/filter.c b/net/filter.c
> index 5d90f83..17a8398 100644
> --- a/net/filter.c
> +++ b/net/filter.c
> @@ -34,6 +34,22 @@ ssize_t qemu_netfilter_receive(NetFilterState *nf,
>      return 0;
>  }
>  
> +static NetFilterState *netfilter_next(NetFilterState *nf,
> +                                      NetFilterDirection dir)
> +{
> +    NetFilterState *next;
> +
> +    if (dir == NET_FILTER_DIRECTION_TX) {
> +        /* forward walk through filters */
> +        next = QTAILQ_NEXT(nf, next);
> +    } else {
> +        /* reverse order */
> +        next = QTAILQ_PREV(nf, NetFilterHead, next);
> +    }
> +
> +    return next;
> +}
> +
>  ssize_t qemu_netfilter_pass_to_next(NetClientState *sender,
>                                      unsigned flags,
>                                      const struct iovec *iov,
> @@ -43,7 +59,7 @@ ssize_t qemu_netfilter_pass_to_next(NetClientState *sender,
>      int ret = 0;
>      int direction;
>      NetFilterState *nf = opaque;
> -    NetFilterState *next = QTAILQ_NEXT(nf, next);
> +    NetFilterState *next = NULL;
>  
>      if (!sender || !sender->peer) {
>          /* no receiver, or sender been deleted, no need to pass it further */
> @@ -61,6 +77,7 @@ ssize_t qemu_netfilter_pass_to_next(NetClientState *sender,
>          direction = nf->direction;
>      }
>  
> +    next = netfilter_next(nf, direction);
>      while (next) {
>          /*
>           * if qemu_netfilter_pass_to_next been called, means that
> @@ -73,7 +90,7 @@ ssize_t qemu_netfilter_pass_to_next(NetClientState *sender,
>          if (ret) {
>              return ret;
>          }
> -        next = QTAILQ_NEXT(next, next);
> +        next = netfilter_next(next, direction);
>      }
>  
>      /*
> diff --git a/net/net.c b/net/net.c
> index 87dd356..05ec996 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -580,11 +580,24 @@ static ssize_t filter_receive_iov(NetClientState *nc,
>      ssize_t ret = 0;
>      NetFilterState *nf = NULL;
>  
> -    QTAILQ_FOREACH(nf, &nc->filters, next) {
> -        ret = qemu_netfilter_receive(nf, direction, sender, flags, iov,
> -                                     iovcnt, sent_cb);
> -        if (ret) {
> -            return ret;
> +    assert(direction == NET_FILTER_DIRECTION_TX ||
> +           direction == NET_FILTER_DIRECTION_RX);
> +

Don't get why we need this assert.

Other looks good.

> +    if (direction == NET_FILTER_DIRECTION_TX) {
> +        QTAILQ_FOREACH(nf, &nc->filters, next) {
> +            ret = qemu_netfilter_receive(nf, direction, sender, flags, iov,
> +                                         iovcnt, sent_cb);
> +            if (ret) {
> +                return ret;
> +            }
> +        }
> +    } else {
> +        QTAILQ_FOREACH_REVERSE(nf, &nc->filters, NetFilterHead, next) {
> +            ret = qemu_netfilter_receive(nf, direction, sender, flags, iov,
> +                                         iovcnt, sent_cb);
> +            if (ret) {
> +                return ret;
> +            }
>          }
>      }
>

On 01/25/2016 12:58 PM, Jason Wang wrote:
>
>
> On 01/22/2016 04:11 PM, Li Zhijian wrote:
>> Previously, if the netdev has more than one filters, the ingress
>> or outgress traffic pass the filter in the same order. this patch
>> is to make the outgress pass the filter in a reverse order
>
> Need a description why we need this.
how about the following description:
Previously, if we attach more than filters for one netdev, IN/OUT traffic pass through
filters in the a same order.
ingress: netdev ->filter1 ->filter2->...filter[n] -> emulated device
outgress: emulated device ->filter1 ->filter2 ->...filter[n] ->netdev.

But in some scene, we hope filters handle the outgress traffic in a reverse order.
For example, in colo-proxy(will be implemented later), we have a redirector filter and
a colo-rewriter filter, we need the filter behavior like that:
ingress(->)/outgress(<-): <->redirector <-> colo-rewriter <->emulated device

So, the patch will make the outgress traffic pass the filter in a reverse order.


>
>>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>> ---
>>   include/net/net.h |  4 +++-
>>   net/filter.c      | 21 +++++++++++++++++++--
>>   net/net.c         | 23 ++++++++++++++++++-----
>>   3 files changed, 40 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/net/net.h b/include/net/net.h
>> index 7af3e15..1d807cc 100644
>> --- a/include/net/net.h
>> +++ b/include/net/net.h
>> @@ -79,6 +79,8 @@ typedef struct NetClientInfo {
>>       SetVnetBE *set_vnet_be;
>>   } NetClientInfo;
>>
>> +QTAILQ_HEAD(NetFilterHead, NetFilterState);
>> +
>>   struct NetClientState {
>>       NetClientInfo *info;
>>       int link_down;
>> @@ -92,7 +94,7 @@ struct NetClientState {
>>       NetClientDestructor *destructor;
>>       unsigned int queue_index;
>>       unsigned rxfilter_notify_enabled:1;
>> -    QTAILQ_HEAD(, NetFilterState) filters;
>> +    struct NetFilterHead filters;
>>   };
>>
>>   typedef struct NICState {
>> diff --git a/net/filter.c b/net/filter.c
>> index 5d90f83..17a8398 100644
>> --- a/net/filter.c
>> +++ b/net/filter.c
>> @@ -34,6 +34,22 @@ ssize_t qemu_netfilter_receive(NetFilterState *nf,
>>       return 0;
>>   }
>>
>> +static NetFilterState *netfilter_next(NetFilterState *nf,
>> +                                      NetFilterDirection dir)
>> +{
>> +    NetFilterState *next;
>> +
>> +    if (dir == NET_FILTER_DIRECTION_TX) {
>> +        /* forward walk through filters */
>> +        next = QTAILQ_NEXT(nf, next);
>> +    } else {
>> +        /* reverse order */
>> +        next = QTAILQ_PREV(nf, NetFilterHead, next);
>> +    }
>> +
>> +    return next;
>> +}
>> +
>>   ssize_t qemu_netfilter_pass_to_next(NetClientState *sender,
>>                                       unsigned flags,
>>                                       const struct iovec *iov,
>> @@ -43,7 +59,7 @@ ssize_t qemu_netfilter_pass_to_next(NetClientState *sender,
>>       int ret = 0;
>>       int direction;
>>       NetFilterState *nf = opaque;
>> -    NetFilterState *next = QTAILQ_NEXT(nf, next);
>> +    NetFilterState *next = NULL;
>>
>>       if (!sender || !sender->peer) {
>>           /* no receiver, or sender been deleted, no need to pass it further */
>> @@ -61,6 +77,7 @@ ssize_t qemu_netfilter_pass_to_next(NetClientState *sender,
>>           direction = nf->direction;
>>       }
>>
>> +    next = netfilter_next(nf, direction);
>>       while (next) {
>>           /*
>>            * if qemu_netfilter_pass_to_next been called, means that
>> @@ -73,7 +90,7 @@ ssize_t qemu_netfilter_pass_to_next(NetClientState *sender,
>>           if (ret) {
>>               return ret;
>>           }
>> -        next = QTAILQ_NEXT(next, next);
>> +        next = netfilter_next(next, direction);
>>       }
>>
>>       /*
>> diff --git a/net/net.c b/net/net.c
>> index 87dd356..05ec996 100644
>> --- a/net/net.c
>> +++ b/net/net.c
>> @@ -580,11 +580,24 @@ static ssize_t filter_receive_iov(NetClientState *nc,
>>       ssize_t ret = 0;
>>       NetFilterState *nf = NULL;
>>
>> -    QTAILQ_FOREACH(nf, &nc->filters, next) {
>> -        ret = qemu_netfilter_receive(nf, direction, sender, flags, iov,
>> -                                     iovcnt, sent_cb);
>> -        if (ret) {
>> -            return ret;
>> +    assert(direction == NET_FILTER_DIRECTION_TX ||
>> +           direction == NET_FILTER_DIRECTION_RX);
>> +
>
> Don't get why we need this assert.
>
i will remove this assertion.


Thanks
Li Zhijian

> Other looks good.
>
>> +    if (direction == NET_FILTER_DIRECTION_TX) {
>> +        QTAILQ_FOREACH(nf, &nc->filters, next) {
>> +            ret = qemu_netfilter_receive(nf, direction, sender, flags, iov,
>> +                                         iovcnt, sent_cb);
>> +            if (ret) {
>> +                return ret;
>> +            }
>> +        }
>> +    } else {
>> +        QTAILQ_FOREACH_REVERSE(nf, &nc->filters, NetFilterHead, next) {
>> +            ret = qemu_netfilter_receive(nf, direction, sender, flags, iov,
>> +                                         iovcnt, sent_cb);
>> +            if (ret) {
>> +                return ret;
>> +            }
>>           }
>>       }
>>
>
>
>
> .
>

On 01/25/2016 02:27 PM, Li Zhijian wrote:
>
>
> On 01/25/2016 12:58 PM, Jason Wang wrote:
>>
>>
>> On 01/22/2016 04:11 PM, Li Zhijian wrote:
>>> Previously, if the netdev has more than one filters, the ingress
>>> or outgress traffic pass the filter in the same order. this patch
>>> is to make the outgress pass the filter in a reverse order
>>
>> Need a description why we need this.
> how about the following description:
> Previously, if we attach more than filters for one netdev, IN/OUT
> traffic pass through
> filters in the a same order.
> ingress: netdev ->filter1 ->filter2->...filter[n] -> emulated device
> outgress: emulated device ->filter1 ->filter2 ->...filter[n] ->netdev.
>
> But in some scene, we hope filters handle the outgress traffic in a
> reverse order.

After the changes, it become 'always' not 'sometimes' :)

> For example, in colo-proxy(will be implemented later), we have a
> redirector filter and
> a colo-rewriter filter, we need the filter behavior like that:
> ingress(->)/outgress(<-): <->redirector <-> colo-rewriter <->emulated
> device
>
> So, the patch will make the outgress traffic pass the filter in a
> reverse order.
>

Other, looks ok tough I may tweak the commit more or less.

Thanks

>
>>
>>>
>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>>> ---
>>>   include/net/net.h |  4 +++-
>>>   net/filter.c      | 21 +++++++++++++++++++--
>>>   net/net.c         | 23 ++++++++++++++++++-----
>>>   3 files changed, 40 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/include/net/net.h b/include/net/net.h
>>> index 7af3e15..1d807cc 100644
>>> --- a/include/net/net.h
>>> +++ b/include/net/net.h
>>> @@ -79,6 +79,8 @@ typedef struct NetClientInfo {
>>>       SetVnetBE *set_vnet_be;
>>>   } NetClientInfo;
>>>
>>> +QTAILQ_HEAD(NetFilterHead, NetFilterState);
>>> +
>>>   struct NetClientState {
>>>       NetClientInfo *info;
>>>       int link_down;
>>> @@ -92,7 +94,7 @@ struct NetClientState {
>>>       NetClientDestructor *destructor;
>>>       unsigned int queue_index;
>>>       unsigned rxfilter_notify_enabled:1;
>>> -    QTAILQ_HEAD(, NetFilterState) filters;
>>> +    struct NetFilterHead filters;
>>>   };
>>>
>>>   typedef struct NICState {
>>> diff --git a/net/filter.c b/net/filter.c
>>> index 5d90f83..17a8398 100644
>>> --- a/net/filter.c
>>> +++ b/net/filter.c
>>> @@ -34,6 +34,22 @@ ssize_t qemu_netfilter_receive(NetFilterState *nf,
>>>       return 0;
>>>   }
>>>
>>> +static NetFilterState *netfilter_next(NetFilterState *nf,
>>> +                                      NetFilterDirection dir)
>>> +{
>>> +    NetFilterState *next;
>>> +
>>> +    if (dir == NET_FILTER_DIRECTION_TX) {
>>> +        /* forward walk through filters */
>>> +        next = QTAILQ_NEXT(nf, next);
>>> +    } else {
>>> +        /* reverse order */
>>> +        next = QTAILQ_PREV(nf, NetFilterHead, next);
>>> +    }
>>> +
>>> +    return next;
>>> +}
>>> +
>>>   ssize_t qemu_netfilter_pass_to_next(NetClientState *sender,
>>>                                       unsigned flags,
>>>                                       const struct iovec *iov,
>>> @@ -43,7 +59,7 @@ ssize_t qemu_netfilter_pass_to_next(NetClientState
>>> *sender,
>>>       int ret = 0;
>>>       int direction;
>>>       NetFilterState *nf = opaque;
>>> -    NetFilterState *next = QTAILQ_NEXT(nf, next);
>>> +    NetFilterState *next = NULL;
>>>
>>>       if (!sender || !sender->peer) {
>>>           /* no receiver, or sender been deleted, no need to pass it
>>> further */
>>> @@ -61,6 +77,7 @@ ssize_t qemu_netfilter_pass_to_next(NetClientState
>>> *sender,
>>>           direction = nf->direction;
>>>       }
>>>
>>> +    next = netfilter_next(nf, direction);
>>>       while (next) {
>>>           /*
>>>            * if qemu_netfilter_pass_to_next been called, means that
>>> @@ -73,7 +90,7 @@ ssize_t qemu_netfilter_pass_to_next(NetClientState
>>> *sender,
>>>           if (ret) {
>>>               return ret;
>>>           }
>>> -        next = QTAILQ_NEXT(next, next);
>>> +        next = netfilter_next(next, direction);
>>>       }
>>>
>>>       /*
>>> diff --git a/net/net.c b/net/net.c
>>> index 87dd356..05ec996 100644
>>> --- a/net/net.c
>>> +++ b/net/net.c
>>> @@ -580,11 +580,24 @@ static ssize_t
>>> filter_receive_iov(NetClientState *nc,
>>>       ssize_t ret = 0;
>>>       NetFilterState *nf = NULL;
>>>
>>> -    QTAILQ_FOREACH(nf, &nc->filters, next) {
>>> -        ret = qemu_netfilter_receive(nf, direction, sender, flags,
>>> iov,
>>> -                                     iovcnt, sent_cb);
>>> -        if (ret) {
>>> -            return ret;
>>> +    assert(direction == NET_FILTER_DIRECTION_TX ||
>>> +           direction == NET_FILTER_DIRECTION_RX);
>>> +
>>
>> Don't get why we need this assert.
>>
> i will remove this assertion.
>
>
> Thanks
> Li Zhijian
>
>> Other looks good.
>>
>>> +    if (direction == NET_FILTER_DIRECTION_TX) {
>>> +        QTAILQ_FOREACH(nf, &nc->filters, next) {
>>> +            ret = qemu_netfilter_receive(nf, direction, sender,
>>> flags, iov,
>>> +                                         iovcnt, sent_cb);
>>> +            if (ret) {
>>> +                return ret;
>>> +            }
>>> +        }
>>> +    } else {
>>> +        QTAILQ_FOREACH_REVERSE(nf, &nc->filters, NetFilterHead,
>>> next) {
>>> +            ret = qemu_netfilter_receive(nf, direction, sender,
>>> flags, iov,
>>> +                                         iovcnt, sent_cb);
>>> +            if (ret) {
>>> +                return ret;
>>> +            }
>>>           }
>>>       }
>>>
>>
>>
>>
>> .
>>
>
>
>