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

[Jason Wang <jasowang@redhat.com>]

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.

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