Re: [Qemu-devel] [PATCH v11 02/12] init/cleanup of netfilter object

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

On 09/24/2015 07:40 PM, Markus Armbruster wrote:
> Yang Hongyang <yanghy@cn.fujitsu.com> writes:
>
>> On 09/24/2015 04:41 PM, Markus Armbruster wrote:
>>> Yang Hongyang <yanghy@cn.fujitsu.com> writes:
>>>
>>>> Add a netfilter object based on QOM.
>>>>
>>>> A netfilter is attached to a netdev, captures all network packets
>>>> that pass through the netdev. When we delete the netdev, we also
>>>> delete the netfilter object attached to it, because if the netdev is
>>>> removed, the filter which attached to it is useless.
>>>>
>>>> QTAILQ_ENTRY next used by netdev, filter belongs to the specific netdev is
>>>> in this queue.
>>>
>>> I don't get this paragraph.  Not sure it's needed.
>>>
>>>> Also init delayed object after net_init_clients, because netfilters need
>>>> to be initialized after net clients initialized.
>>>
>>> A paragraph starting with "Also" in a commit message is a pretty good
>>> sign the patch should be split :)
>>>
>>>>
>>>> Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
>>>> ---
>>>> v11: no need to free nf->netdev_id, it will be auto freeed while object deleted
>>>>        remove global_list net_filters, will add back when needed
>>>> v10: use QOM for netfilter
>>>> v9: use flat union instead of simple union in QAPI schema
>>>> v8: include vhost_net header
>>>> v7: add check for vhost
>>>>       fix error propagate bug
>>>> v6: add multiqueue support (net_filter_init1)
>>>> v5: remove model from NetFilterState
>>>>       add a sent_cb param to receive_iov API
>>>> ---
>>>>    include/net/filter.h    |  60 +++++++++++++++++++++
>>>>    include/net/net.h       |   1 +
>>>>    include/qemu/typedefs.h |   1 +
>>>>    net/Makefile.objs       |   1 +
>>>>    net/filter.c            | 138 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    net/net.c               |   7 +++
>>>>    qapi-schema.json        |  18 +++++++
>>>>    vl.c                    |  13 ++---
>>>>    8 files changed, 233 insertions(+), 6 deletions(-)
>>>>    create mode 100644 include/net/filter.h
>>>>    create mode 100644 net/filter.c
>>>>
>>>> diff --git a/include/net/filter.h b/include/net/filter.h
>>>> new file mode 100644
>>>> index 0000000..226f2f7
>>>> --- /dev/null
>>>> +++ b/include/net/filter.h
>>>> @@ -0,0 +1,60 @@
>>>> +/*
>>>> + * Copyright (c) 2015 FUJITSU LIMITED
>>>> + * Author: Yang Hongyang <yanghy@cn.fujitsu.com>
>>>> + *
>>>> + * This work is licensed under the terms of the GNU GPL, version 2 or
>>>> + * later.  See the COPYING file in the top-level directory.
>>>> + */
>>>> +
>>>> +#ifndef QEMU_NET_FILTER_H
>>>> +#define QEMU_NET_FILTER_H
>>>> +
>>>> +#include "qom/object.h"
>>>> +#include "qemu-common.h"
>>>> +#include "qemu/typedefs.h"
>>>> +#include "net/queue.h"
>>>> +
>>>> +#define TYPE_NETFILTER "netfilter"
>>>> +#define NETFILTER(obj) \
>>>> +    OBJECT_CHECK(NetFilterState, (obj), TYPE_NETFILTER)
>>>> +#define NETFILTER_GET_CLASS(obj) \
>>>> +    OBJECT_GET_CLASS(NetFilterClass, (obj), TYPE_NETFILTER)
>>>> +#define NETFILTER_CLASS(klass) \
>>>> +    OBJECT_CLASS_CHECK(NetFilterClass, (klass), TYPE_NETFILTER)
>>>> +
>>>> +typedef void (FilterSetup) (NetFilterState *nf, Error **errp);
>>>> +typedef void (FilterCleanup) (NetFilterState *nf);
>>>> +/*
>>>> + * 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 *nc,
>>>> +                                   NetClientState *sender,
>>>> +                                   unsigned flags,
>>>> +                                   const struct iovec *iov,
>>>> +                                   int iovcnt,
>>>> +                                   NetPacketSent *sent_cb);
>>>> +
>>>> +struct NetFilterClass {
>>>> +    ObjectClass parent_class;
>>>> +
>>>> +    FilterSetup *setup;
>>>> +    FilterCleanup *cleanup;
>>>> +    FilterReceiveIOV *receive_iov;
>>>> +};
>>>> +typedef struct NetFilterClass NetFilterClass;
>>>
>>> Not splitting the declaration is more concise:
>>>
>>>       typedef struct {
>>>           ObjectClass parent_class;
>>>           FilterSetup *setup;
>>>           FilterCleanup *cleanup;
>>>           FilterReceiveIOV *receive_iov;
>>>       } NetFilterClass;
>>>
>>> Are any of the methods optional?  If yes, please add suitable comments.
>>
>> Hi Markus, I split it because the checkpatch.pl told me to do so...
>
> Understand.  However, it's a recent change to checkpatch.pl that's going
> to be reverted:
> Message-ID: <55FAEB33.50809@redhat.com>
> http://lists.gnu.org/archive/html/qemu-devel/2015-09/msg04644.html

Thanks for the information.

>
> [...]
> .
>

-- 
Thanks,
Yang.