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

[Hailiang Zhang]

On 2016/1/25 15:22, 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)
> > >
> > > +#define DEFAULT_FILTER_NAME "nop"
> >
> > Maybe DEFAULT_FILTER_TYPE?
>
> > > +
> > > +#define TYPE_FILTER_BUFFER "filter-buffer"
> > > +#define TYPE_FILTER_DUMP "filter-dump"
> > > +
> > > +#define NETFILTER_ID_BUFFER 1
> > > +#define NETFILTER_ID_DUMP 2
> > > +
> > > +extern const char *const netfilter_type_lookup[];
> > > +
> > >   typedef void (FilterSetup) (NetFilterState *nf, Error **errp);
> > >   typedef void (FilterCleanup) (NetFilterState *nf);
> > >   /*
> > > @@ -55,6 +65,7 @@ struct NetFilterState {
> > >       char *netdev_id;
> > >       NetClientState *netdev;
> > >       NetFilterDirection direction;
> > > +    bool is_default;
> > >       bool enabled;
> > >       QTAILQ_ENTRY(NetFilterState) next;
> > >   };
> > > diff --git a/net/dump.c b/net/dump.c
> > > index 88d9582..82727a6 100644
> > > --- a/net/dump.c
> > > +++ b/net/dump.c
> > > @@ -229,8 +229,6 @@ int net_init_dump(const NetClientOptions *opts, const char 
> > > *name,
> > >
> > >   /* Dumping via filter */
> > >
> > > -#define TYPE_FILTER_DUMP "filter-dump"
> > > -
> > >   #define FILTER_DUMP(obj) \
> > >       OBJECT_CHECK(NetFilterDumpState, (obj), TYPE_FILTER_DUMP)
> > >
> > > diff --git a/net/filter.c b/net/filter.c
> > > index 4d96301..a126a3b 100644
> > > --- a/net/filter.c
> > > +++ b/net/filter.c
> > > @@ -21,6 +21,11 @@
> > >   #include "qapi/qmp-input-visitor.h"
> > >   #include "monitor/monitor.h"
> > >
> > > +const char *const netfilter_type_lookup[] = {
> > > +    [NETFILTER_ID_BUFFER] = TYPE_FILTER_BUFFER,
> > > +    [NETFILTER_ID_DUMP] = TYPE_FILTER_DUMP,
> > > +};
> > > +
> > >   ssize_t qemu_netfilter_receive(NetFilterState *nf,
> > >                                  NetFilterDirection direction,
> > >                                  NetClientState *sender,
> > > @@ -200,7 +205,7 @@ static void netfilter_complete(UserCreatable *uc, Error 
> > > **errp)
> > >       NetFilterClass *nfc = NETFILTER_GET_CLASS(uc);
> > >       int queues;
> > >       Error *local_err = NULL;
> > > -
> > > +    char *path = object_get_canonical_path_component(OBJECT(nf));
> > >
> > >       if (!nf->netdev_id) {
> > >           error_setg(errp, "Parameter 'netdev' is required");
> > > @@ -225,6 +230,14 @@ static void netfilter_complete(UserCreatable *uc, Error 
> > > **errp)
> > >       }
> > >
> > >       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 ?

I don't think it is necessary to add a 'default' property, because
we don't want such a property to be controlled by user. It is
only used internally.
We have another choice to deal with this,
Add a 'bool is_default' parameter for netdev_add_filter(),
and handle the default filter in it, just like:

void netdev_add_filter(const char *netdev_id,
                       const char *filter_type,
                       const char *id,
                       bool is_default,
                       Error **errp)
{
    ... ...
    object_add(filter_type, id, qdict, iv, &err);
    .... ...
    if (is_default) {
        Object *obj, *container;
        NetFilterState *nf;

        container = object_get_objects_root();
        obj = object_resolve_path_component(container, id);
        if (!obj) {
            error_setg(errp, "object id not found");
            return;
        }
        nf = NETFILTER(obj);
        nf->is_default = true;
        nf->enabled = false;
    }

}

Is this acceptable ?

Thanks,
Hailiang

> > >       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' ?
>
> > > + */
> > > +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;
> > >   }
> >
> >
> > .