Re: [Qemu-devel] [PATCH RFC v2 3/5] net/filter: Introduce a helper to add a filter to the netdev

[Hailiang Zhang]

On 2016/2/1 17:04, Jason Wang wrote:
> On 02/01/2016 03:56 PM, Hailiang Zhang wrote:
> > On 2016/2/1 15:46, Jason Wang wrote:
> > > On 02/01/2016 02:13 PM, Hailiang Zhang wrote:
> > > > On 2016/2/1 11:14, Jason Wang wrote:
> > > > > On 01/27/2016 04:29 PM, zhanghailiang wrote:
> > > > > > We add a new helper function netdev_add_filter(), this function
> > > > > > can help adding a filter object to a netdev.
> > > > > > Besides, we add a is_default member for struct NetFilterState
> > > > > > to indicate whether the filter is default or not.
> > > > > >
> > > > > > Signed-off-by: zhanghailiang <address@hidden>
> > > > > > ---
> > > > > > v2:
> > > > > >     -Re-implement netdev_add_filter() by re-using object_create()
> > > > > >      (Jason's suggestion)
> > > > > > ---
> > > > > >     include/net/filter.h |  7 +++++
> > > > > >     net/filter.c         | 80
> > > > > > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > >     2 files changed, 87 insertions(+)
>
> [...]
>
> > > > > > +
> > > > > > +    optarg =
> > > > > > g_strdup_printf("qom-type=%s,id=%s,netdev=%s,status=%s",
> > > > > > +            filter_type, id, netdev_id, is_default ? "disable" :
> > > > > > "enable"
> > > > >
> > > > > Instead of this, I wonder maybe it's better to:
> > > > >
> > > > > - store the default filter property into a pointer to string
> > > >
> > > > Do you mean, pass a string parameter which stores the filter property
> > > > instead of
> > > > assemble it in this helper ?
> > >
> > > Yes. E.g just a global string which could be changed by any subsystem.
> > > E.g colo may change it to "filter-buffer,interval=0,status=disable". But
> > > filter ids need to be generated automatically.
> >
> > Got it. Then we don't need the global default_netfilter_type[] in
> > patch 5,
>
> Yes.
>
> > Just use this global string instead ?
>
> Right.
>
> > > > > - colo code may change the pointer to "filter-buffer,status=disable"
> > > >
> > > > > Then, there's no need for lots of codes above:
> > > > > - no need a "is_default" parameter in netdev_add_filter which does not
> > > > > scale consider we may want to have more property in the future
> > > > > - no need to hacking like "qemu_filter_opts"
> > > >
> > > > Yes, we can use qemu_find_opts("object") instead of it.
> > > >
> > > > > - no need to have a special flag like "is_default"
> > > >
> > > > But we have to distinguish the default filter from the common
> > > > filter, use the name (id) to distinguish it ?
> > >
> > > What's the reason that you want to distinguish default filters from
> > > others?
> >
> > The default filters will be used by COLO or MC, (In COLO, we will use it
> > to control packets buffering/releasing).
>
> A question is how will you do this?

Er, for COLO, we will enable all the default filter in the initialization stage,
then the buffer-filter will buffer all netdev's packets,
after doing a checkpoint, we will release all the buffered packets (Flush all 
default
filters' packets). If VM is failover, we will set all default filters back to 
disabled
status.
(This is a periodic mode for COLO, different from another mode, which we will 
call it
hybrid mode, that is based on colo-proxy, which is in developing by zhangchen)

Thanks,
Hailiang

> > For COLO, we don't want to control (use) other filters that added by
> > users.
> >
> > Thanks,
> > Hailiang
> >
> > > Thanks
> > >
> > > > Thanks,
> > > > Hailiang
> > > >
> > > > > Thoughts?
> > > > >
> > > > > > +    opts = qemu_opts_parse_noisily(&qemu_filter_opts,
> > > > > > +                                   optarg, false);
> > > > > > +    if (!opts) {
> > > > > > +        error_report("Failed to parse param '%s'", optarg);
> > > > > > +        exit(1);
> > > > > > +    }
> > > > > > +    g_free(optarg);
> > > > > > +    if (object_create(NULL, opts, &err) < 0) {
> > > > > > +        error_report("Failed to create object");
> > > > > > +        goto out_clean;
> > > > > > +    }
> > > > > > +    filter_set_default_flag(id, is_default, &err);
> > > > > > +
> > > > > > +out_clean:
> > > > > > +    qemu_opts_del(opts);
> > > > > > +    if (err) {
> > > > > > +        error_propagate(errp, err);
> > > > > > +    }
> > > > > > +}
> > > > > > +
> > > > > >     static void netfilter_finalize(Object *obj)
> > > > > >     {
> > > > > >         NetFilterState *nf = NETFILTER(obj);
> > > > >
> > > > >
> > > > > .
> > >
> > >
> > > .
>
>
> .