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

[Lukas Straub <lukasstraub2@web.de>]

On Fri, 23 Aug 2019 03:24:02 +0000
"Zhang, Chen" <chen.zhang@intel.com> wrote:

> > -----Original Message-----
> > From: Lukas Straub [mailto:lukasstraub2@web.de]
> > Sent: Friday, August 16, 2019 2:49 AM
> > To: qemu-devel <qemu-devel@nongnu.org>
> > Cc: Zhang, Chen <chen.zhang@intel.com>; Jason Wang
> > <jasowang@redhat.com>; Wen Congyang <wencongyang2@huawei.com>; Xie
> > Changlong <xiechanglong.d@gmail.com>
> > 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 <lukasstraub2@web.de>
> > ---
> >  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
>