Re: [Qemu-devel] [PATCH v3 04/12] net: add/remove filters from network backend

[Yang Hongyang <yanghy@cn.fujitsu.com>]

On 08/04/2015 01:53 PM, Jason Wang wrote:
>
>
> On 08/04/2015 01:39 PM, Yang Hongyang wrote:
>> On 08/04/2015 12:56 PM, Jason Wang wrote:
>>>
>>>
>>> On 08/03/2015 04:30 PM, Yang Hongyang wrote:
>>>> add/remove filters from network backend
>>>>
>>>> Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
>>>> ---
>>>>    include/net/net.h |  8 ++++++++
>>>>    net/filter.c      |  4 ++--
>>>>    net/net.c         | 33 +++++++++++++++++++++++++++++++++
>>>>    3 files changed, 43 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/include/net/net.h b/include/net/net.h
>>>> index 6a6cbef..5c5c109 100644
>>>> --- a/include/net/net.h
>>>> +++ b/include/net/net.h
>>>> @@ -40,6 +40,11 @@ typedef struct NICConf {
>>>>
>>>>
>>>>    /* Net clients */
>>>> +typedef struct Filter Filter;
>>>> +struct Filter {
>>>> +    NetFilterState *nf;
>>>> +    QTAILQ_ENTRY(Filter) next;
>>>> +};
>>>
>>> Didn't understand why need another structure here. Could we just use
>>> NetFilterState?
>>
>> There's a QTAILQ_ENTRY next in NetFilterState, but used by filter
>> layer, so
>> that we can manage all filters together.
>>
>> This struct used by netdev. filter belongs to the specific netdev is
>> in this queue.
>>
>> Just use NetFilterState in this case need to introduce another
>> QTAILQ_ENTRY
>> in NetFilterState, maybe will make it confusing?
>
> Probably not.
>
>> and it seems that it's
>> not common to have 2 QTAILQ_ENTRYs in one struct?
>
> I think this is ok. You can use something like global_list.

Sounds good, thank you!

>
>>
>>>
>>>>
>>>>    typedef void (NetPoll)(NetClientState *, bool enable);
>>>>    typedef int (NetCanReceive)(NetClientState *);
>>>> @@ -92,6 +97,7 @@ struct NetClientState {
>>>>        NetClientDestructor *destructor;
>>>>        unsigned int queue_index;
>>>>        unsigned rxfilter_notify_enabled:1;
>>>> +    QTAILQ_HEAD(, Filter) filters;
>>>>    };
>>>>
>>>>    typedef struct NICState {
>>>> @@ -109,6 +115,8 @@ NetClientState
>>>> *qemu_new_net_client(NetClientInfo *info,
>>>>                                        NetClientState *peer,
>>>>                                        const char *model,
>>>>                                        const char *name);
>>>> +int qemu_netdev_add_filter(NetClientState *nc, NetFilterState *nf);
>>>> +void qemu_netdev_remove_filter(NetClientState *nc, NetFilterState
>>>> *nf);
>>>>    NICState *qemu_new_nic(NetClientInfo *info,
>>>>                           NICConf *conf,
>>>>                           const char *model,
>>>> diff --git a/net/filter.c b/net/filter.c
>>>> index 86eed8a..1ae9344 100644
>>>> --- a/net/filter.c
>>>> +++ b/net/filter.c
>>>> @@ -38,14 +38,14 @@ NetFilterState
>>>> *qemu_new_net_filter(NetFilterInfo *info,
>>>>        nf->netdev = netdev;
>>>>        nf->chain = chain;
>>>>        QTAILQ_INSERT_TAIL(&net_filters, nf, next);
>>>> -    /* TODO: attach netfilter to netdev */
>>>> +    qemu_netdev_add_filter(netdev, nf);
>>>>
>>>>        return nf;
>>>>    }
>>>>
>>>>    static void qemu_cleanup_net_filter(NetFilterState *nf)
>>>>    {
>>>> -    /* TODO: remove netfilter from netdev */
>>>> +    qemu_netdev_remove_filter(nf->netdev, nf);
>>>>
>>>>        QTAILQ_REMOVE(&net_filters, nf, next);
>>>>
>>>> diff --git a/net/net.c b/net/net.c
>>>> index 28a5597..00c5ca3 100644
>>>> --- a/net/net.c
>>>> +++ b/net/net.c
>>>> @@ -287,6 +287,7 @@ static void qemu_net_client_setup(NetClientState
>>>> *nc,
>>>>
>>>>        nc->incoming_queue = qemu_new_net_queue(nc);
>>>>        nc->destructor = destructor;
>>>> +    QTAILQ_INIT(&nc->filters);
>>>>    }
>>>>
>>>>    NetClientState *qemu_new_net_client(NetClientInfo *info,
>>>> @@ -305,6 +306,38 @@ NetClientState
>>>> *qemu_new_net_client(NetClientInfo *info,
>>>>        return nc;
>>>>    }
>>>>
>>>> +int qemu_netdev_add_filter(NetClientState *nc, NetFilterState *nf)
>>>> +{
>>>> +    Filter *filter = g_malloc0(sizeof(*filter));
>>>> +
>>>> +    filter->nf = nf;
>>>> +    QTAILQ_INSERT_TAIL(&nc->filters, filter, next);
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static void remove_filter(NetClientState *nc, Filter *filter)
>>>> +{
>>>> +    if (!filter) {
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    QTAILQ_REMOVE(&nc->filters, filter, next);
>>>> +    g_free(filter);
>>>> +}
>>>> +
>>>> +void qemu_netdev_remove_filter(NetClientState *nc, NetFilterState *nf)
>>>> +{
>>>> +    Filter *filter = NULL;
>>>> +
>>>> +    QTAILQ_FOREACH(filter, &nc->filters, next) {
>>>> +        if (filter->nf == nf) {
>>>> +            break;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    remove_filter(nc, filter);
>>>> +}
>>>> +
>>>>    NICState *qemu_new_nic(NetClientInfo *info,
>>>>                           NICConf *conf,
>>>>                           const char *model,
>>>
>>> Another thing may need consider is qemu_flush_queued_packets(). Look
>>> like we need also flush packets inside each filter in this case?
>>
>> When a filter is removed, the filter itself will flush the packets.
>> when a filter queued a packet, we assume the filter will take care
>> of it. the filter is not a netdev, so I think we do not need to flush
>> packets in qemu_flush_queued_packets(), otherwise, the buffer filter
>> will be useless, because when qemu_flush_queued_packets() is called,
>> it will flush the packets, it's not what we want.
>
> Right.
> .
>

-- 
Thanks,
Yang.