Re: [Qemu-devel] [PATCH RFC 6/7] net/filter: Add a default filter to each netdev

[Hailiang Zhang]

On 2016/1/26 11:18, Jason Wang wrote:
> On 01/25/2016 03:22 PM, Hailiang Zhang wrote:
> > On 2016/1/25 13:18, Jason Wang wrote:
> > > On 01/22/2016 04:36 PM, zhanghailiang wrote:
> > > > We add each netdev a default buffer filter, which the name is
> > > > 'nop', and the default buffer filter is disabled, so it has
> > > > no side effect for packets delivering in qemu net layer.
> > > >
> > > > The default buffer filter can be used by COLO or Micro-checkpoint,
> > > > The reason we add the default filter is we hope to support
> > > > hot add network during COLO state in future.
> > > >
> > > > Signed-off-by: zhanghailiang <address@hidden>
> > > > ---
> > > >    include/net/filter.h | 11 +++++++++++
> > > >    net/dump.c           |  2 --
> > > >    net/filter.c         | 15 ++++++++++++++-
> > > >    net/net.c            | 18 ++++++++++++++++++
> > > >    4 files changed, 43 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/include/net/filter.h b/include/net/filter.h
> > > > index c7bd8f9..2043609 100644
> > > > --- a/include/net/filter.h
> > > > +++ b/include/net/filter.h
> > > > @@ -22,6 +22,16 @@
> > > >    #define NETFILTER_CLASS(klass) \
> > > >        OBJECT_CLASS_CHECK(NetFilterClass, (klass), TYPE_NETFILTER)
>
> [...]
>
> > > >        nf->netdev = ncs[0];
> > > > +    nf->is_default = !strcmp(path, DEFAULT_FILTER_NAME);
> > > > +    /*
> > > > +    * For the default buffer filter, it will be disabled by default,
> > > > +    * So it will not buffer any packets.
> > > > +    */
> > > > +    if (nf->is_default) {
> > > > +        nf->enabled = false;
> > > > +    }
> > >
> > > This seems not very elegant. Besides DEFAULT_FILTER_NAME(TYPE), we may
> > > also want a DEFAULT_FILTER_PROPERTIES? Then you can store the "status"
> > > into properties.
> >
> > A little confused, do you mean add a 'default' property for filter ?
> > Just like the new 'status' property which is exported to users ?
> > Is the type of 'default' property string or bool ?
>
> For example, is it possible to store the default property into a string
> and just create the filter through qemu_opts_parse_noisily() by just
> pass a string as its parameter? (Of course, it needs some code to
> generate ids). With this, there's even no need for you to duplicate
> codes like what patch 4 does.

Er, got it, that seems to be a cleverer method, i will look at it,
thanks very much for your patient explanation.

> > > >        if (nfc->setup) {
> > > >            nfc->setup(nf, &local_err);
> > > > diff --git a/net/net.c b/net/net.c
> > > > index ec43105..9630234 100644
> > > > --- a/net/net.c
> > > > +++ b/net/net.c
> > > > @@ -76,6 +76,12 @@ const char *host_net_devices[] = {
> > > >
> > > >    int default_net = 1;
> > > >
> > > > +/*
> > > > + * FIXME: Export this with an option for users to control
> > > > + * this with comand line ?
> > >
> > > This could be done in the future.
> >
> > Should i change the tag to 'TODO' ?
>
> Ok.

> > > > + */
> > > > +int default_netfilter = NETFILTER_ID_BUFFER;
> > >
> > > Why not just use a string here?
> >
> > No special, i will convert it to use string here.
> >
> > > > +
> > > >    /***********************************************************/
> > > >    /* network device redirectors */
> > > >
> > > > @@ -1032,6 +1038,18 @@ static int net_client_init1(const void
> > > > *object, int is_netdev, Error **errp)
> > > >            }
> > > >            return -1;
> > > >        }
> > > > +
> > > > +    if (is_netdev) {
> > > > +        const Netdev *netdev = object;
> > > > +        /*
> > > > +        * Here we add each netdev a default filter whose name is
> > > > 'nop',
> > > > +        * it will disabled by default, Users can enable it when
> > > > necessary.
> > > > +        */
> > >
> > > If we support default properties, the above comment could be removed.
> > >
> > > > +        netdev_add_filter(netdev->id,
> > > > +                          netfilter_type_lookup[default_netfilter],
> > > > +                          DEFAULT_FILTER_NAME,
> > >
> > > I believe some logic to generate id automatically is needed here.
> >
> > Yes, you are right, here this patch will break QEMU with multi-NICs
> > configured,
> > the error report is " attempt to add duplicate property 'nop' to object".
> > I will fix it in next version.
> >
> > Thanks,
> > Hailiang
> >
> > > > +                          errp);
> > > > +    }
> > > >        return 0;
> > > >    }
> > >
> > >
> > > .
>
>
> .