Re: [Qemu-devel] [PATCH v4 02/11] init/cleanup of netfilter object

[Jason Wang <jasowang@redhat.com>]

On 08/06/2015 03:50 PM, Yang Hongyang wrote:
> On 08/06/2015 03:43 PM, Jason Wang wrote:
>> On 08/06/2015 03:35 PM, Yang Hongyang wrote:
>>> On 08/06/2015 03:29 PM, Jason Wang wrote:
>>>>
>>>>
>>>> On 08/06/2015 03:22 PM, Yang Hongyang wrote:
>>>>> On 08/06/2015 03:07 PM, Jason Wang wrote:
>>>>>>
>>>>>>
>>>>>> On 08/04/2015 04:30 PM, Yang Hongyang wrote:
>>>>>>> QTAILQ_ENTRY global_list but used by filter layer, so that we can
>>>>>>> manage all filters together.
>>>>>>> QTAILQ_ENTRY next used by netdev, filter belongs to the specific
>>>>>>> netdev is
>>>>>>> in this queue.
>>>>>>> This is mostly the same with init/cleanup of netdev object.
>>>>>>>
>>>>>>> Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
>>>>>>> ---
>>>>>>>     include/net/filter.h    |  39 +++++++++++++
>>>>>>>     include/net/net.h       |   1 +
>>>>>>>     include/qemu/typedefs.h |   1 +
>>>>>>>     net/filter.c            | 147
>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>     net/net.c               |   1 +
>>>>>>>     qapi-schema.json        |  37 ++++++++++++
>>>>>>>     6 files changed, 226 insertions(+)
>>>>>>>
>>>>>>> diff --git a/include/net/filter.h b/include/net/filter.h
>>>>>>> index 4242ded..9aafe08 100644
>>>>>>> --- a/include/net/filter.h
>>>>>>> +++ b/include/net/filter.h
>>>>>>> @@ -9,7 +9,46 @@
>>>>>>>     #define QEMU_NET_FILTER_H
>>>>>>>
>>>>>>>     #include "qemu-common.h"
>>>>>>> +#include "qemu/typedefs.h"
>>>>>>> +
>>>>>>> +/* the netfilter chain */
>>>>>>> +enum {
>>>>>>> +    NET_FILTER_IN,
>>>>>>> +    NET_FILTER_OUT,
>>>>>>> +    NET_FILTER_ALL,
>>>>>>> +};
>>>>>>> +
>>>>>>> +typedef void (FilterCleanup) (NetFilterState *);
>>>>>>> +/*
>>>>>>> + * Return:
>>>>>>> + *   0: finished handling the packet, we should continue
>>>>>>> + *   size: filter stolen this packet, we stop pass this packet
>>>>>>> further
>>>>>>> + */
>>>>>>> +typedef ssize_t (FilterReceiveIOV)(NetFilterState *,
>>>>>>> NetClientState
>>>>>>> *sender,
>>>>>>> +                                   unsigned flags, const struct
>>>>>>> iovec *, int);
>>>>>>
>>>>>> Please name all parameters.
>>>>>
>>>>> Ok, thanks.
>>>>>
>>>>>>
>>>>>>> +
>>>>>>> +typedef struct NetFilterInfo {
>>>>>>> +    NetFilterOptionsKind type;
>>>>>>> +    size_t size;
>>>>>>> +    FilterCleanup *cleanup;
>>>>>>> +    FilterReceiveIOV *receive_iov;
>>>>>>> +} NetFilterInfo;
>>>>>>> +
>>>>>>> +struct NetFilterState {
>>>>>>> +    NetFilterInfo *info;
>>>>>>> +    char *model;
>>>>>>
>>>>>> Looks like model is never used?
>>>>>
>>>>> It can be used when we want to find filters by model. For example,
>>>>> when we need to find all "buffer" filter, and release all buffered
>>>>> packets.
>>>>
>>>> But this is not implemented in this series.
>>>
>>> true.
>>>
>>>> And I don't get why you need
>>>> a such command. Management should keep track of this.
>>>
>>> There's a patch in v1 which introduce an API use this, and I dropped
>>> that
>>> because there's no user currently, but that API I suppose to be used
>>> by MC or COLO which can do periodic checkpoint, to release packets.
>>> the API is filter_buffer_release_all() IIRC...
>>
>> I see, let's start with simple case to make review and merging easier.
>> You can re-introduce them when it has real users.
>
> So do I need to remove model also?

Yes, please.