RE: [PATCH v5 3/4] net/filter.c: Add Options to insert filters anywhere in the filter list

[Zhang, Chen]

> -----Original Message-----
> From: Lukas Straub <address@hidden>
> Sent: Monday, September 16, 2019 3:20 AM
> To: qemu-devel <address@hidden>
> Cc: Zhang, Chen <address@hidden>; Jason Wang
> <address@hidden>; Wen Congyang <address@hidden>;
> Xie Changlong <address@hidden>; address@hidden;
> address@hidden
> Subject: [PATCH v5 3/4] 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 "head" or "tail" to insert at the head or tail of the 
> filter list
> or it should be "id=<id>" to specify the id of another filter.
> insert should be either "before" or "behind" to specify where to insert the
> new filter relative to the one specified with position.
> 
> Signed-off-by: Lukas Straub <address@hidden>
> ---
>  include/net/filter.h |  2 +
>  net/filter.c         | 92
> +++++++++++++++++++++++++++++++++++++++++++-
>  qemu-options.hx      | 10 ++---
>  3 files changed, 98 insertions(+), 6 deletions(-)
> 
> diff --git a/include/net/filter.h b/include/net/filter.h index
> 49da666ac0..22a723305b 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_flag;
>      QTAILQ_ENTRY(NetFilterState) next;
>  };
> 
> diff --git a/net/filter.c b/net/filter.c index 28d1930db7..cd2ef9e979 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_flag ? g_strdup("before") :
> +g_strdup("behind"); }
> +
> +static void netfilter_set_insert(Object *obj, const char *str, Error
> +**errp) {
> +    NetFilterState *nf = NETFILTER(obj);
> +
> +    if (strcmp(str, "before") && strcmp(str, "behind")) {
> +        error_setg(errp, "Invalid value for netfilter insert, "
> +                         "should be 'before' or 'behind'");
> +        return;
> +    }
> +
> +    nf->insert_before_flag = !strcmp(str, "before"); }
> +
>  static void netfilter_init(Object *obj)  {
>      NetFilterState *nf = NETFILTER(obj);
> 
>      nf->on = true;
> +    nf->insert_before_flag = 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,41 @@ static void netfilter_complete(UserCreatable *uc,
> Error **errp)
>          return;
>      }
> 
> +    if (strcmp(nf->position, "head") && strcmp(nf->position, "tail")) {
> +        Object *container;
> +        Object *obj;
> +        char *position_id;
> +
> +        if (!g_str_has_prefix(nf->position, "id=")) {
> +            error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "position",
> +                       "'head', 'tail' or 'id=<id>'");
> +            return;
> +        }
> +
> +        /* get the id from the string */
> +        position_id = g_strndup(nf->position + 3, strlen(nf->position)
> + - 3);
> +
> +        /* Search for the position to insert before/behind */
> +        container = object_get_objects_root();
> +        obj = object_resolve_path_component(container, position_id);
> +        if (!obj) {
> +            error_setg(errp, "filter '%s' not found", position_id);
> +            g_free(position_id);
> +            return;
> +        }
> +
> +        position = NETFILTER(obj);
> +
> +        if (position->netdev != ncs[0]) {
> +            error_setg(errp, "filter '%s' belongs to a different netdev",
> +                        position_id);
> +            g_free(position_id);
> +            return;
> +        }
> +
> +        g_free(position_id);
> +    }
> +
>      nf->netdev = ncs[0];
> 
>      if (nfc->setup) {
> @@ -228,7 +306,18 @@ static void netfilter_complete(UserCreatable *uc,
> Error **errp)
>              return;
>          }
>      }
> -    QTAILQ_INSERT_TAIL(&nf->netdev->filters, nf, next);
> +
> +    if (position) {
> +        if (nf->insert_before_flag) {
> +            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 +334,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..23fa5a344e 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|rx|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=<id
> +>}][,insert=@var{behind|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
> =@var{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=<
> i
> +d>}][,insert=@var{behind|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.
> 

Please add description for the newly added parameter in each filter.
After that:
Reviewed-by: Zhang Chen <address@hidden>

Thanks
Zhang Chen


> -@item -object filter-
> redirector,id=@var{id},netdev=@var{netdevid},indev=@var{chardevid},out
> dev=@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][,p
> os
> +ition=@var{head|tail|id=<id>}][,insert=@var{behind|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=<id>}][,insert=@var{beh
> +ind|before}]
> 
>  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=<id>}][,insert=@var{behind|befor
> +e}]
> 
>  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