On Wed, Aug 22, 2018 at 4:22 PM Jason Wang <jasow...@redhat.com> wrote:
>
>
> On 2018年08月21日 17:25, Zhang Chen wrote:
> > On Tue, Aug 21, 2018 at 11:30 AM Jason Wang <jasow...@redhat.com> 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 <zhang.zhanghaili...@huawei.com>
> >>> ---
> >>> 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];
> >>
>
>
