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

[Yang Hongyang]

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.

> > 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?

> > diff --git a/vl.c b/vl.c
> > index 584ca88..aee931a 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -75,6 +75,7 @@ int main(int argc, char **argv)
> >   #include "monitor/qdev.h"
> >   #include "sysemu/bt.h"
> >   #include "net/net.h"
> > +#include "net/filter.h"
> >   #include "net/slirp.h"
> >   #include "monitor/monitor.h"
> >   #include "ui/console.h"
> > @@ -2998,6 +2999,7 @@ int main(int argc, char **argv, char **envp)
> >       qemu_add_opts(&qemu_device_opts);
> >       qemu_add_opts(&qemu_netdev_opts);
> >       qemu_add_opts(&qemu_net_opts);
> > +    qemu_add_opts(&qemu_netfilter_opts);
> >       qemu_add_opts(&qemu_rtc_opts);
> >       qemu_add_opts(&qemu_global_opts);
> >       qemu_add_opts(&qemu_mon_opts);
> > @@ -3284,6 +3286,13 @@ int main(int argc, char **argv, char **envp)
> >                       exit(1);
> >                   }
> >                   break;
> > +            case QEMU_OPTION_netfilter:
> > +                opts = qemu_opts_parse_noisily(qemu_find_opts("netfilter"),
> > +                                               optarg, true);
> > +                if (!opts) {
> > +                    exit(1);
> > +                }
> > +                break;
> >   #ifdef CONFIG_LIBISCSI
> >               case QEMU_OPTION_iscsi:
> >                   opts = qemu_opts_parse_noisily(qemu_find_opts("iscsi"),
> > @@ -4413,6 +4422,10 @@ int main(int argc, char **argv, char **envp)
> >           exit(1);
> >       }
> >
> > +    if (net_init_filters() < 0) {
> > +        exit(1);
> > +    }
> > +
> >   #ifdef CONFIG_TPM
> >       if (tpm_init() < 0) {
> >           exit(1);
>
> .

--
Thanks,
Yang.