Re: [Qemu-devel] [PATCH COLO-Frame v13 34/39] net/filter-buffer: Add default filter-buffer for each netdev

[Jason Wang <jasowang@redhat.com>]

On 12/29/2015 03:09 PM, zhanghailiang wrote:
> We add each netdev (except vhost-net) a default filter-buffer,
> which will be used for COLO or Micro-checkpoint to buffer VM's packets.
> The name of default filter-buffer is 'nop'.
> For the default filter-buffer, it will not buffer any packets in default.
> So it has no side effect for the netdev.
>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: Yang Hongyang <hongyang.yang@easystack.cn>

This patch did three things:

1) the ability to enable or disable a netfilter
2) the ability to add a default filter
3) default filter attaching for filter-buffer

Better to split them into separate small patches.

And several questions:

For 1), I'm not sure this is real needed, we can in fact disable a
filter by removing it.
For 2), Instead of a specific code just for filter buffer, I think we
need a generic method for an arbitrary filter to be used as default.
And if we can achieve 2), 3) is not needed any more.

> ---
> v12:
> - Skip vhost-net when add default filter
> - Don't go through filter layer if the filter is disabled.
> v11:
> - New patch
> ---
>  include/net/filter.h | 10 +++++++
>  net/filter-buffer.c  | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  net/filter.c         |  6 +++-
>  net/net.c            | 12 ++++++++
>  4 files changed, 109 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/filter.h b/include/net/filter.h
> index 2deda36..40aa38c 100644
> --- a/include/net/filter.h
> +++ b/include/net/filter.h
> @@ -56,6 +56,8 @@ struct NetFilterState {
>      NetClientState *netdev;
>      NetFilterDirection direction;
>      char info_str[256];
> +    bool is_default;
> +    bool enabled;
>      QTAILQ_ENTRY(NetFilterState) next;
>  };
>  
> @@ -74,4 +76,12 @@ ssize_t qemu_netfilter_pass_to_next(NetClientState *sender,
>                                      int iovcnt,
>                                      void *opaque);
>  
> +static inline bool qemu_need_skip_netfilter(NetFilterState *nf)
> +{
> +    return nf->enabled ? false : true;
> +}
> +
> +void netdev_add_default_filter_buffer(const char *netdev_id,
> +                                      NetFilterDirection direction,
> +                                      Error **errp);
>  #endif /* QEMU_NET_FILTER_H */
> diff --git a/net/filter-buffer.c b/net/filter-buffer.c
> index 57be149..9cf3544 100644
> --- a/net/filter-buffer.c
> +++ b/net/filter-buffer.c
> @@ -14,6 +14,13 @@
>  #include "qapi/qmp/qerror.h"
>  #include "qapi-visit.h"
>  #include "qom/object.h"
> +#include "net/net.h"
> +#include "qapi/qmp/qdict.h"
> +#include "qapi/qmp-output-visitor.h"
> +#include "qapi/qmp-input-visitor.h"
> +#include "monitor/monitor.h"
> +#include "qmp-commands.h"
> +#include "net/vhost_net.h"
>  
>  #define TYPE_FILTER_BUFFER "filter-buffer"
>  
> @@ -102,6 +109,7 @@ static void filter_buffer_cleanup(NetFilterState *nf)
>  static void filter_buffer_setup(NetFilterState *nf, Error **errp)
>  {
>      FilterBufferState *s = FILTER_BUFFER(nf);
> +    char *path = object_get_canonical_path_component(OBJECT(nf));
>  
>      /*
>       * We may want to accept zero interval when VM FT solutions like MC
> @@ -114,6 +122,14 @@ static void filter_buffer_setup(NetFilterState *nf, Error **errp)
>      }
>  
>      s->incoming_queue = qemu_new_net_queue(qemu_netfilter_pass_to_next, nf);
> +    nf->is_default = !strcmp(path, "nop");
> +    /*
> +    * For the default buffer filter, it will be disabled by default,
> +    * So it will not buffer any packets.
> +    */
> +    if (nf->is_default) {
> +        nf->enabled = false;
> +    }
>      if (s->interval) {
>          timer_init_us(&s->release_timer, QEMU_CLOCK_VIRTUAL,
>                        filter_buffer_release_timer, nf);
> @@ -163,6 +179,72 @@ out:
>      error_propagate(errp, local_err);
>  }
>  
> +/*
> +* This will be used by COLO or MC FT, for which they will need
> +* to buffer the packets of VM's net devices, Here we add a default
> +* buffer filter for each netdev. The name of default buffer filter is
> +* 'nop'
> +*/
> +void netdev_add_default_filter_buffer(const char *netdev_id,
> +                                      NetFilterDirection direction,
> +                                      Error **errp)
> +{

Need a more generic way to add an arbitrary filter as default. E.g
during netdev init, query if there's a default and do the initialization
there.

> +    QmpOutputVisitor *qov;
> +    QmpInputVisitor *qiv;
> +    Visitor *ov, *iv;
> +    QObject *obj = NULL;
> +    QDict *qdict;
> +    void *dummy = NULL;
> +    const char *id = "nop";
> +    char *queue = g_strdup(NetFilterDirection_lookup[direction]);
> +    NetClientState *nc = qemu_find_netdev(netdev_id);
> +    Error *err = NULL;
> +
> +    /* FIXME: Not support multiple queues */
> +    if (!nc || nc->queue_index > 1) {
> +        g_free(queue);
> +        return;
> +    }
> +    /* Not support vhost-net */
> +    if (get_vhost_net(nc)) {
> +        g_free(queue);
> +        return;
> +    }
> +    qov = qmp_output_visitor_new();
> +    ov = qmp_output_get_visitor(qov);
> +    visit_start_struct(ov,  &dummy, NULL, NULL, 0, &err);
> +    if (err) {
> +        goto out;
> +    }
> +    visit_type_str(ov, &nc->name, "netdev", &err);
> +    if (err) {
> +        goto out;
> +    }
> +    visit_type_str(ov, &queue, "queue", &err);
> +    if (err) {
> +        goto out;
> +    }
> +    visit_end_struct(ov, &err);
> +    if (err) {
> +        goto out;
> +    }
> +    obj = qmp_output_get_qobject(qov);
> +    g_assert(obj != NULL);
> +    qdict = qobject_to_qdict(obj);
> +    qmp_output_visitor_cleanup(qov);
> +
> +    qiv = qmp_input_visitor_new(obj);
> +    iv = qmp_input_get_visitor(qiv);
> +    object_add(TYPE_FILTER_BUFFER, id, qdict, iv, &err);
> +    qmp_input_visitor_cleanup(qiv);
> +    qobject_decref(obj);
> +out:
> +    g_free(queue);
> +    if (err) {
> +        error_propagate(errp, err);
> +    }
> +}
> +
>  static void filter_buffer_init(Object *obj)
>  {
>      object_property_add(obj, "interval", "int",
> diff --git a/net/filter.c b/net/filter.c
> index 1365bad..0b1e408 100644
> --- a/net/filter.c
> +++ b/net/filter.c
> @@ -163,7 +163,8 @@ static void netfilter_complete(UserCreatable *uc, Error **errp)
>      }
>  
>      nf->netdev = ncs[0];
> -
> +    nf->is_default = false;
> +    nf->enabled = true;

If we really need something like enabled, need more code for user to
control this bit.

>      if (nfc->setup) {
>          nfc->setup(nf, &local_err);
>          if (local_err) {
> @@ -190,6 +191,9 @@ static void netfilter_complete(UserCreatable *uc, Error **errp)
>          g_free(info);
>      }
>      object_property_iter_free(iter);
> +    info = g_strdup_printf(",status=%s", nf->enabled ? "on" : "off");
> +    g_strlcat(nf->info_str, info, sizeof(nf->info_str));
> +    g_free(info);
>  }
>  
>  static void netfilter_finalize(Object *obj)
> diff --git a/net/net.c b/net/net.c
> index 87dd356..fd53cfc 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -581,6 +581,10 @@ static ssize_t filter_receive_iov(NetClientState *nc,
>      NetFilterState *nf = NULL;
>  
>      QTAILQ_FOREACH(nf, &nc->filters, next) {
> +        /* Don't go through filter if it is off */
> +        if (qemu_need_skip_netfilter(nf)) {
> +            continue;
> +        }

Better move the above to qemu_netfilter_receive()

>          ret = qemu_netfilter_receive(nf, direction, sender, flags, iov,
>                                       iovcnt, sent_cb);
>          if (ret) {
> @@ -1028,6 +1032,14 @@ static int net_client_init1(const void *object, int is_netdev, Error **errp)
>          }
>          return -1;
>      }
> +
> +    if (is_netdev) {
> +        const Netdev *netdev = object;
> +
> +        netdev_add_default_filter_buffer(netdev->id,
> +                                         NET_FILTER_DIRECTION_RX,
> +                                         errp);

Several issues here:

- Generic code should know nothing about a specific kind of netfilter.
So the default filter should be pointed out by user instead of
hard-coding it like this.
- Need to deal with the netdev hot add. It should still has the default
filter.

> +    }
>      return 0;
>  }
>