Re: [Qemu-devel] [PATCH] net: walk through filters reversely if traffic is outgress

[Jason Wang]

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 <address@hidden>
>>> Signed-off-by: Li Zhijian <address@hidden>
>>> ---
>>>   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;
>>> +            }
>>>           }
>>>       }
>>>
>>
>>
>>
>> .
>>
>
>
>