Re: [Qemu-devel] [PATCH v8 01/11] net: add a new object netfilter

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

On 08/28/2015 07:29 PM, Markus Armbruster wrote:
> Yang Hongyang <yanghy@cn.fujitsu.com> writes:
>
>> On 08/26/2015 10:04 PM, Markus Armbruster wrote:
>>> Missed a bunch of revisions of this series, please excuse gaps in my
>>> understanding.
>>
>> Thank you for the review.
>>
>>>
>>> Yang Hongyang <yanghy@cn.fujitsu.com> writes:
>>>
>>>> Add the framework for a new netfilter object and a new
>>>> -netfilter CLI option as a basis for the following patches.
>>>>
>>>> Signed-off-by: Yang Hongyang <yanghy@cn.fujitsu.com>
>>>> CC: Paolo Bonzini <pbonzini@redhat.com>
>>>> CC: Eric Blake <eblake@redhat.com>
>>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>    include/net/filter.h    | 15 +++++++++++++++
>>>>    include/sysemu/sysemu.h |  1 +
>>>>    net/Makefile.objs       |  1 +
>>>>    net/filter.c            | 27 +++++++++++++++++++++++++++
>>>>    qemu-options.hx         |  1 +
>>>>    vl.c                    | 13 +++++++++++++
>>>>    6 files changed, 58 insertions(+)
>>>>    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..4242ded
>>>> --- /dev/null
>>>> +++ b/include/net/filter.h
>>>> @@ -0,0 +1,15 @@
>>>> +/*
>>>> + * Copyright (c) 2015 FUJITSU LIMITED
>>>> + *
>>>> + * 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 "qemu-common.h"
>>>> +
>>>> +int net_init_filters(void);
>>>> +
>>>> +#endif /* QEMU_NET_FILTER_H */
>>>> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
>>>> index 44570d1..15d6d00 100644
>>>> --- a/include/sysemu/sysemu.h
>>>> +++ b/include/sysemu/sysemu.h
>>>> @@ -212,6 +212,7 @@ extern QemuOptsList qemu_chardev_opts;
>>>>    extern QemuOptsList qemu_device_opts;
>>>>    extern QemuOptsList qemu_netdev_opts;
>>>>    extern QemuOptsList qemu_net_opts;
>>>> +extern QemuOptsList qemu_netfilter_opts;
>>>>    extern QemuOptsList qemu_global_opts;
>>>>    extern QemuOptsList qemu_mon_opts;
>>>>
>>>> diff --git a/net/Makefile.objs b/net/Makefile.objs
>>>> index ec19cb3..914aec0 100644
>>>> --- a/net/Makefile.objs
>>>> +++ b/net/Makefile.objs
>>>> @@ -13,3 +13,4 @@ common-obj-$(CONFIG_HAIKU) += tap-haiku.o
>>>>    common-obj-$(CONFIG_SLIRP) += slirp.o
>>>>    common-obj-$(CONFIG_VDE) += vde.o
>>>>    common-obj-$(CONFIG_NETMAP) += netmap.o
>>>> +common-obj-y += filter.o
>>>> diff --git a/net/filter.c b/net/filter.c
>>>> new file mode 100644
>>>> index 0000000..4e40f08
>>>> --- /dev/null
>>>> +++ b/net/filter.c
>>>> @@ -0,0 +1,27 @@
>>>> +/*
>>>> + * Copyright (c) 2015 FUJITSU LIMITED
>>>> + *
>>>> + * This work is licensed under the terms of the GNU GPL, version 2 or
>>>> + * later.  See the COPYING file in the top-level directory.
>>>> + */
>>>> +
>>>> +#include "qemu-common.h"
>>>> +#include "net/filter.h"
>>>> +
>>>> +int net_init_filters(void)
>>>> +{
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +QemuOptsList qemu_netfilter_opts = {
>>>> +    .name = "netfilter",
>>>> +    .implied_opt_name = "type",
>>>> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_netfilter_opts.head),
>>>> +    .desc = {
>>>> +        /*
>>>> +         * no elements => accept any params
>>>> +         * validation will happen later
>>>> +         */
>>>> +        { /* end of list */ }
>>>> +    },
>>>> +};
>>>
>>> Ignorant question: why dynamic validation (empty .desc) instead of
>>> statically defined parameters?  The documentation you add in PATCH 10
>>> suggests they're not actually dynamic.
>>
>> Actually I was following the netdev stuff. There might be bunch of filters,
>> and I don't know the params of each filter until it is realized.
>
> I realized that later on in your series.
>
>>>> diff --git a/qemu-options.hx b/qemu-options.hx
>>>> index 77f5853..0d52d02 100644
>>>> --- a/qemu-options.hx
>>>> +++ b/qemu-options.hx
>>>> @@ -1575,6 +1575,7 @@ DEF("net", HAS_ARG, QEMU_OPTION_net,
>>>>        "socket][,vlan=n][,option][,option][,...]\n"
>>>>        "                old way to initialize a host network interface\n"
>>>>        "                (use the -netdev option if possible instead)\n", QEMU_ARCH_ALL)
>>>> +DEF("netfilter", HAS_ARG, QEMU_OPTION_netfilter, "", QEMU_ARCH_ALL)
>>>>    STEXI
>>>>    @item -net nic[,vlan=@var{n}][,macaddr=@var{mac}][,model=@var{type}] [,name=@var{name}][,addr=@var{addr}][,vectors=@var{v}]
>>>>    @findex -net
>>>
>>> Wrong spot: this is between DEF(net, ...) and its STEXI..ETEXI stanza.
>>> Suggest to move it behind the ETEXI.
>>>
>>> Missing help string.  You add the help in PATCH 10.  What about adding
>>> it here already?  Would serve as a hint of the things to come later in
>>> your series.
>>>
>>> Missing STEXI..ETEXI stanza for the user manual.
>>
>> If I understand correctly, you are suggesting separate the netfilter from
>> net, seems more reasonable, so I should add something like:
>> DEF("netfilter", HAS_ARG, QEMU_OPTION_netfilter, "", QEMU_ARCH_ALL)
>> STEXI..ETEXI
>>
>> after the DEF(net, ...) and its STEXI..ETEXI stanza, am I right?
>
> Yes.  Always keep DEF(whatever, ...) and its STEXI..ETEXI together.
>
> Inserting netfilter after net seems the most logical place.

Yes, thank you.

>
> [...]
> .
>

-- 
Thanks,
Yang.