Yang Hongyang <yan...@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 <yan...@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 <yan...@cn.fujitsu.com>
>>> CC: Paolo Bonzini <pbonz...@redhat.com>
>>> CC: Eric Blake <ebl...@redhat.com>
>>> Reviewed-by: Thomas Huth <th...@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.
[...]
