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

[Jason Wang]

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.

Btw, I remember we allow disabling filter through qmp, if it's true, we 
probably need some notification to management, but this can be done in 
the future.

> > > +        break;
> > > +    default:
> > > +        break;
> > > +    }
> > > +}
> > > +
> > >    static void netfilter_class_init(ObjectClass *oc, void *data)
> > >    {
> > >        UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
> > > +    NetFilterClass *nfc = NETFILTER_CLASS(oc);
> > >
> > >        ucc->complete = netfilter_complete;
> > > +    nfc->handle_event = dummy_handle_event;
> > >    }
> > >
> > >    static const TypeInfo netfilter_info = {
> > > diff --git a/net/net.c b/net/net.c
> > > index a77ea88fff..b4f6a2efb2 100644
> > > --- a/net/net.c
> > > +++ b/net/net.c
> > > @@ -1331,6 +1331,34 @@ void hmp_info_network(Monitor *mon, const QDict
> > *qdict)
> > >        }
> > >    }
> > >
> > > +void colo_notify_filters_event(int event, Error **errp)
> > > +{
> > > +    NetClientState *nc, *peer;
> > > +    NetClientDriver type;
> > > +    NetFilterState *nf;
> > > +    NetFilterClass *nfc = NULL;
> > > +    Error *local_err = NULL;
> > > +
> > > +    QTAILQ_FOREACH(nc, &net_clients, next) {
> > > +        peer = nc->peer;
> > > +        type = nc->info->type;
> > > +        if (!peer || type != NET_CLIENT_DRIVER_TAP) {
> > > +            continue;
> > > +        }
> > The check the TAP is redundant with previous patch.
> OK, I will remove it.
>
>
> > > +        QTAILQ_FOREACH(nf, &nc->filters, next) {
> > > +            nfc =  NETFILTER_GET_CLASS(OBJECT(nf));
> > > +            if (!nfc->handle_event) {
> > Looks like this won't happen.
> It will happen. like filter-mirror and filter-redirector haven't this event.

But you do:

  static void netfilter_class_init(ObjectClass *oc, void *data)
  {
      UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
+    NetFilterClass *nfc = NETFILTER_CLASS(oc);

      ucc->complete = netfilter_complete;
+    nfc->handle_event = dummy_handle_event;
  }

?

Thanks

> Thanks
> Zhang Chen
>
>
> > Thanks
> >
> > > +                continue;
> > > +            }
> > > +            nfc->handle_event(nf, event, &local_err);
> > > +            if (local_err) {
> > > +                error_propagate(errp, local_err);
> > > +                return;
> > > +            }
> > > +        }
> > > +    }
> > > +}
> > > +
> > >    void qmp_set_link(const char *name, bool up, Error **errp)
> > >    {
> > >        NetClientState *ncs[MAX_QUEUE_NUM];