Re: [Qemu-devel] [PATCH V11 16/20] filter: Add handle_event method for NetFilterClass

[Jason Wang]

On 2018年08月24日 13:57, Zhang Chen wrote:
> On Wed, Aug 22, 2018 at 4:22 PM Jason Wang<address@hidden>  wrote:
>
> > On 2018年08月21日 17:25, Zhang Chen wrote:
> > > On Tue, Aug 21, 2018 at 11:30 AM Jason Wang<address@hidden>  wrote:
> > >
> > > > On 2018年08月12日 04:59, Zhang Chen wrote:
> > > > > Filter needs to process the event of checkpoint/failover or
> > > > > other event passed by COLO frame.
> > > > >
> > > > > Signed-off-by: zhanghailiang<address@hidden>
> > > > > ---
> > > > >     include/net/filter.h |  5 +++++
> > > > >     net/filter.c         | 17 +++++++++++++++++
> > > > >     net/net.c            | 28 ++++++++++++++++++++++++++++
> > > > >     3 files changed, 50 insertions(+)
> > > > >
> > > > > diff --git a/include/net/filter.h b/include/net/filter.h
> > > > > index 435acd6f82..49da666ac0 100644
> > > > > --- a/include/net/filter.h
> > > > > +++ b/include/net/filter.h
> > > > > @@ -38,6 +38,8 @@ typedef ssize_t (FilterReceiveIOV)(NetFilterState
> > *nc,
> > > > >     typedef void (FilterStatusChanged) (NetFilterState *nf, Error
> > **errp);
> > > > > +typedef void (FilterHandleEvent) (NetFilterState *nf, int event, Error
> > > > **errp);
> > > > > +
> > > > >     typedef struct NetFilterClass {
> > > > >         ObjectClass parent_class;
> > > > >
> > > > > @@ -45,6 +47,7 @@ typedef struct NetFilterClass {
> > > > >         FilterSetup *setup;
> > > > >         FilterCleanup *cleanup;
> > > > >         FilterStatusChanged *status_changed;
> > > > > +    FilterHandleEvent *handle_event;
> > > > >         /* mandatory */
> > > > >         FilterReceiveIOV *receive_iov;
> > > > >     } NetFilterClass;
> > > > > @@ -77,4 +80,6 @@ ssize_t qemu_netfilter_pass_to_next(NetClientState
> > > > *sender,
> > > > >                                         int iovcnt,
> > > > >                                         void *opaque);
> > > > >
> > > > > +void colo_notify_filters_event(int event, Error **errp);
> > > > > +
> > > > >     #endif /* QEMU_NET_FILTER_H */
> > > > > diff --git a/net/filter.c b/net/filter.c
> > > > > index 2fd7d7d663..0f17eba143 100644
> > > > > --- a/net/filter.c
> > > > > +++ b/net/filter.c
> > > > > @@ -17,6 +17,8 @@
> > > > >     #include "net/vhost_net.h"
> > > > >     #include "qom/object_interfaces.h"
> > > > >     #include "qemu/iov.h"
> > > > > +#include "net/colo.h"
> > > > > +#include "migration/colo.h"
> > > > >
> > > > >     static inline bool qemu_can_skip_netfilter(NetFilterState *nf)
> > > > >     {
> > > > > @@ -245,11 +247,26 @@ static void netfilter_finalize(Object *obj)
> > > > >         g_free(nf->netdev_id);
> > > > >     }
> > > > >
> > > > > +static void dummy_handle_event(NetFilterState *nf, int event, Error
> > > > **errp)
> > > > > +{
> > > > It's in fact not a "dummy" handler, Maybe it's better to rename it as
> > > > "default".
> > > OK, I will rename it in next version.
> > >
> > >
> > > > > +    switch (event) {
> > > > > +    case COLO_EVENT_CHECKPOINT:
> > > > > +        break;
> > > > > +    case COLO_EVENT_FAILOVER:
> > > > > +        object_property_set_str(OBJECT(nf), "off", "status", errp);
> > > > I think filter is a generic infrastructure, so it's better not have COLO
> > > > specific things like this. You can either add a generic name or have a
> > > > dedicated helper to just disable all net filters.
> > > Maybe we can rename it to "EVENT_CHECKPOINT" and "EVENT_FAILOVER" looks
> > > better?
> > I think registering notifier to COLO is better. For disabling filter, we
> > can have a generic dedicated helpers to do this.
> If we registering notifier to COLO (in migration/colo.c), filter-rewriter
> can not get the notify immediately when checkout occur.
> filter-reweiter didn't know when checkpoint happened,

I may miss something, isn't COLO which triggers the checkpoint even?

>   and I think loop to
> check the COLO event in filter-rewriter is a bad choice.

Well, anyway you need a loop somewhere to iterate all notifiers?

Thanks

> Any thing I misunderstand?