Re: [Qemu-devel] [PATCH v2 2/3] net/filter.c: Add Options to insert filters anywhere in the filter list

[Lukas Straub]

On Fri, 23 Aug 2019 03:24:02 +0000
"Zhang, Chen" <address@hidden> wrote:

> > -----Original Message-----
> > From: Lukas Straub [mailto:address@hidden]
> > Sent: Friday, August 16, 2019 2:49 AM
> > To: qemu-devel <address@hidden>
> > Cc: Zhang, Chen <address@hidden>; Jason Wang
> > <address@hidden>; Wen Congyang <address@hidden>; Xie
> > Changlong <address@hidden>
> > Subject: [PATCH v2 2/3] net/filter.c: Add Options to insert filters 
> > anywhere in
> > the filter list
> >
> > To switch the Secondary to Primary, we need to insert new filters before the
> > filter-rewriter.
> >
> > Add the options insert= and position= to be able to insert filters anywhere 
> > in the
> > filter list.
> >
> > position should be either "head", "tail" or the id of another filter.
> > insert should be either "before" or "after" to specify where to insert the 
> > new
> > filter relative to the one specified with position.
> >
>
> Hi Lukas,
>
> It looks no need to add the "insert = xxx" for this operation.
> For example:
>
> We have 3 net-filters, the running order like that:
>
> Fiter1   ---------->   Filter2 ------------> Filter3
>
> If we want to add another filter between filter1 and filter2.
> The "Position = head, insert = after" always seam with "position = filter2 
> id, insert = before".

Hi Zhang,
The insert= parameter is ignored if position=head or tail. It always Inserts at 
the head (before Filter1) or the tail (after Filter3) of the List in these 
cases.

> It seems the "insert" is a redundant args.
> So I think it is enough with the "position", we can make the "insert" always 
> equal "after" except the "head".

Yes, we still could do it without it, but its more convenient with the insert= 
parameter. For example our Case with inserting before the rewriter:

'filter-mirror', 'id': 'm0', 'props': { 'insert': 'before', 'position': 'rew0', 
'netdev': 'hn0', 'queue': 'tx', 'outdev': 'mirror0' }
'filter-redirector', 'id': 'redire0', 'props': { 'insert': 'before', 
'position': 'rew0', 'netdev': 'hn0', 'queue': 'rx', 'indev': 'compare_out' }
'filter-redirector', 'id': 'redire1', 'props': { 'insert': 'before', 
'position': 'rew0', 'netdev': 'hn0', 'queue': 'rx', 'outdev': 'compare0' }

You see directly that here 3 Filters are inserted before the rewriter.

would have to become:

'filter-mirror', 'id': 'm0', 'props': { 'position': 'head', 'netdev': 'hn0', 
'queue': 'tx', 'outdev': 'mirror0' }
'filter-redirector', 'id': 'redire0', 'props': { 'position': 'm0', 'netdev': 
'hn0', 'queue': 'rx', 'indev': 'compare_out' }
'filter-redirector', 'id': 'redire1', 'props': { 'position': 'redire0', 
'netdev': 'hn0', 'queue': 'rx', 'outdev': 'compare0' }

Which is less obvious.

Regards,
Lukas Straub

>
> Thanks
> Zhang Chen
>
> > Signed-off-by: Lukas Straub <address@hidden>
> > ---
> >  include/net/filter.h |  2 ++
> >  net/filter.c         | 71 +++++++++++++++++++++++++++++++++++++++++++-
> >  qemu-options.hx      | 10 +++----
> >  3 files changed, 77 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/net/filter.h b/include/net/filter.h index
> > 49da666ac0..355c178f75 100644
> > --- a/include/net/filter.h
> > +++ b/include/net/filter.h
> > @@ -62,6 +62,8 @@ struct NetFilterState {
> >      NetClientState *netdev;
> >      NetFilterDirection direction;
> >      bool on;
> > +    char *position;
> > +    bool insert_before;
> >      QTAILQ_ENTRY(NetFilterState) next;
> >  };
> >
> > diff --git a/net/filter.c b/net/filter.c index 28d1930db7..309fd778df 100644
> > --- a/net/filter.c
> > +++ b/net/filter.c
> > @@ -171,11 +171,47 @@ static void netfilter_set_status(Object *obj, const
> > char *str, Error **errp)
> >      }
> >  }
> >
> > +static char *netfilter_get_position(Object *obj, Error **errp) {
> > +    NetFilterState *nf = NETFILTER(obj);
> > +
> > +    return g_strdup(nf->position);
> > +}
> > +
> > +static void netfilter_set_position(Object *obj, const char *str, Error
> > +**errp) {
> > +    NetFilterState *nf = NETFILTER(obj);
> > +
> > +    nf->position = g_strdup(str);
> > +}
> > +
> > +static char *netfilter_get_insert(Object *obj, Error **errp) {
> > +    NetFilterState *nf = NETFILTER(obj);
> > +
> > +    return nf->insert_before ? g_strdup("before") : g_strdup("after");
> > +}
> > +
> > +static void netfilter_set_insert(Object *obj, const char *str, Error
> > +**errp) {
> > +    NetFilterState *nf = NETFILTER(obj);
> > +
> > +    if (strcmp(str, "before") && strcmp(str, "after")) {
> > +        error_setg(errp, "Invalid value for netfilter insert, "
> > +                         "should be 'head' or 'tail'");
> > +        return;
> > +    }
> > +
> > +    nf->insert_before = !strcmp(str, "before"); }
> > +
> >  static void netfilter_init(Object *obj)  {
> >      NetFilterState *nf = NETFILTER(obj);
> >
> >      nf->on = true;
> > +    nf->insert_before = false;
> > +    nf->position = g_strdup("tail");
> >
> >      object_property_add_str(obj, "netdev",
> >                              netfilter_get_netdev_id, 
> > netfilter_set_netdev_id, @@ -187,11
> > +223,18 @@ static void netfilter_init(Object *obj)
> >      object_property_add_str(obj, "status",
> >                              netfilter_get_status, netfilter_set_status,
> >                              NULL);
> > +    object_property_add_str(obj, "position",
> > +                            netfilter_get_position, netfilter_set_position,
> > +                            NULL);
> > +    object_property_add_str(obj, "insert",
> > +                            netfilter_get_insert, netfilter_set_insert,
> > +                            NULL);
> >  }
> >
> >  static void netfilter_complete(UserCreatable *uc, Error **errp)  {
> >      NetFilterState *nf = NETFILTER(uc);
> > +    NetFilterState *position = NULL;
> >      NetClientState *ncs[MAX_QUEUE_NUM];
> >      NetFilterClass *nfc = NETFILTER_GET_CLASS(uc);
> >      int queues;
> > @@ -219,6 +262,20 @@ static void netfilter_complete(UserCreatable *uc,
> > Error **errp)
> >          return;
> >      }
> >
> > +    if (strcmp(nf->position, "head") && strcmp(nf->position, "tail")) {
> > +        /* Search for the position to insert before/after */
> > +        Object *container;
> > +        Object *obj;
> > +
> > +        container = object_get_objects_root();
> > +        obj = object_resolve_path_component(container, nf->position);
> > +        if (!obj) {
> > +            error_setg(errp, "filter '%s' not found", nf->position);
> > +            return;
> > +        }
> > +        position = NETFILTER(obj);
> > +    }
> > +
> >      nf->netdev = ncs[0];
> >
> >      if (nfc->setup) {
> > @@ -228,7 +285,18 @@ static void netfilter_complete(UserCreatable *uc,
> > Error **errp)
> >              return;
> >          }
> >      }
> > -    QTAILQ_INSERT_TAIL(&nf->netdev->filters, nf, next);
> > +
> > +    if (position) {
> > +        if (nf->insert_before) {
> > +            QTAILQ_INSERT_BEFORE(position, nf, next);
> > +        } else {
> > +            QTAILQ_INSERT_AFTER(&nf->netdev->filters, position, nf, next);
> > +        }
> > +    } else if (!strcmp(nf->position, "head")) {
> > +        QTAILQ_INSERT_HEAD(&nf->netdev->filters, nf, next);
> > +    } else if (!strcmp(nf->position, "tail")) {
> > +        QTAILQ_INSERT_TAIL(&nf->netdev->filters, nf, next);
> > +    }
> >  }
> >
> >  static void netfilter_finalize(Object *obj) @@ -245,6 +313,7 @@ static void
> > netfilter_finalize(Object *obj)
> >          QTAILQ_REMOVE(&nf->netdev->filters, nf, next);
> >      }
> >      g_free(nf->netdev_id);
> > +    g_free(nf->position);
> >  }
> >
> >  static void default_handle_event(NetFilterState *nf, int event, Error 
> > **errp)
> > diff --git a/qemu-options.hx b/qemu-options.hx index 08749a3391..f0a47a0746
> > 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -4368,7 +4368,7 @@ applications, they can do this through this parameter.
> > Its format is  a gnutls priority string as described at
> > @url{https://gnutls.org/manual/html_node/Priority-Strings.html}.
> >
> > -@item -object filter-
> > buffer,id=@var{id},netdev=@var{netdevid},interval=@var{t}[,queue=@var{all|r
> > x|tx}][,status=@var{on|off}]
> > +@item -object
> > +filter-buffer,id=@var{id},netdev=@var{netdevid},interval=@var{t}[,queue
> > +=@var{all|rx|tx}][,status=@var{on|off}][,position=@var{head|tail|id}][,
> > +insert=@var{after|before}]
> >
> >  Interval @var{t} can't be 0, this filter batches the packet delivery: all  
> > packets
> > arriving in a given interval on netdev @var{netdevid} are delayed @@ 
> > -4387,11
> > +4387,11 @@ queue @var{all|rx|tx} is an option that can be applied to any
> > netfilter.
> >  @option{tx}: the filter is attached to the transmit queue of the netdev,
> >               where it will receive packets sent by the netdev.
> >
> > -@item -object filter-
> > mirror,id=@var{id},netdev=@var{netdevid},outdev=@var{chardevid},queue=@v
> > ar{all|rx|tx}[,vnet_hdr_support]
> > +@item -object
> > +filter-mirror,id=@var{id},netdev=@var{netdevid},outdev=@var{chardevid},
> > +queue=@var{all|rx|tx}[,vnet_hdr_support][,position=@var{head|tail|id}][
> > +,insert=@var{after|before}]
> >
> >  filter-mirror on netdev @var{netdevid},mirror net packet to
> > chardev@var{chardevid}, if it has the vnet_hdr_support flag, filter-mirror 
> > will
> > mirror packet with vnet_hdr_len.
> >
> > -@item -object filter-
> > redirector,id=@var{id},netdev=@var{netdevid},indev=@var{chardevid},outdev=
> > @var{chardevid},queue=@var{all|rx|tx}[,vnet_hdr_support]
> > +@item -object
> > +filter-redirector,id=@var{id},netdev=@var{netdevid},indev=@var{chardevi
> > +d},outdev=@var{chardevid},queue=@var{all|rx|tx}[,vnet_hdr_support][,pos
> > +ition=@var{head|tail|id}][,insert=@var{after|before}]
> >
> >  filter-redirector on netdev @var{netdevid},redirect filter's net packet to
> > chardev  @var{chardevid},and redirect indev's packet to filter.if it has the
> > vnet_hdr_support flag, @@ -4400,7 +4400,7 @@ Create a filter-redirector we
> > need to differ outdev id from indev id, id can not  be the same. we can 
> > just use
> > indev or outdev, but at least one of indev or outdev  need to be specified.
> >
> > -@item -object filter-
> > rewriter,id=@var{id},netdev=@var{netdevid},queue=@var{all|rx|tx},[vnet_hdr_
> > support]
> > +@item -object
> > +filter-rewriter,id=@var{id},netdev=@var{netdevid},queue=@var{all|rx|tx}
> > +,[vnet_hdr_support][,position=@var{head|tail|id}][,insert=@var{after|be
> > +fore}]
> >
> >  Filter-rewriter is a part of COLO project.It will rewrite tcp packet to  
> > secondary
> > from primary to keep secondary tcp connection,and rewrite @@ -4413,7
> > +4413,7 @@ colo secondary:
> >  -object filter-redirector,id=f2,netdev=hn0,queue=rx,outdev=red1
> >  -object filter-rewriter,id=rew0,netdev=hn0,queue=all
> >
> > -@item -object filter-
> > dump,id=@var{id},netdev=@var{dev}[,file=@var{filename}][,maxlen=@var{len}]
> > +@item -object
> > +filter-dump,id=@var{id},netdev=@var{dev}[,file=@var{filename}][,maxlen=
> > +@var{len}][,position=@var{head|tail|id}][,insert=@var{after|before}]
> >
> >  Dump the network traffic on netdev @var{dev} to the file specified by
> > @var{filename}. At most @var{len} bytes (64k by default) per packet are 
> > stored.
> > --
> > 2.20.1
>