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

[Markus Armbruster]

Yang Hongyang <address@hidden> 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 <address@hidden> 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 <address@hidden>
>>> CC: Paolo Bonzini <address@hidden>
>>> CC: Eric Blake <address@hidden>
>>> Reviewed-by: Thomas Huth <address@hidden>
>>> ---
>>>   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[,address@hidden,address@hidden,address@hidden 
>>> [,address@hidden,address@hidden,address@hidden
>>>   @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.

[...]