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

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

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? and it seems that it's
not common to have 2 QTAILQ_ENTRYs in one struct?

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

> .
>

-- 
Thanks,
Yang.