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

[Zhang Chen]

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, and I think loop to
check the COLO event in filter-rewriter is a bad choice.
Any thing I misunderstand?



>
> 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;
>    }
>
> ?
>

Oh, I forgot thiat. you are right, I will remove this in next version.

Thanks
Zhang Chen


>
> 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];
> >>
>
>