Re: [Qemu-devel] [PATCH RFC 6/7] net/filter: Add a default filter to each netdev

[Jason Wang]

On 01/22/2016 04:36 PM, zhanghailiang wrote:
> We add each netdev a default buffer filter, which the name is
> 'nop', and the default buffer filter is disabled, so it has
> no side effect for packets delivering in qemu net layer.
>
> The default buffer filter can be used by COLO or Micro-checkpoint,
> The reason we add the default filter is we hope to support
> hot add network during COLO state in future.
>
> Signed-off-by: zhanghailiang <address@hidden>
> ---
>  include/net/filter.h | 11 +++++++++++
>  net/dump.c           |  2 --
>  net/filter.c         | 15 ++++++++++++++-
>  net/net.c            | 18 ++++++++++++++++++
>  4 files changed, 43 insertions(+), 3 deletions(-)
>
> diff --git a/include/net/filter.h b/include/net/filter.h
> index c7bd8f9..2043609 100644
> --- a/include/net/filter.h
> +++ b/include/net/filter.h
> @@ -22,6 +22,16 @@
>  #define NETFILTER_CLASS(klass) \
>      OBJECT_CLASS_CHECK(NetFilterClass, (klass), TYPE_NETFILTER)
>  
> +#define DEFAULT_FILTER_NAME "nop"

Maybe DEFAULT_FILTER_TYPE?

> +
> +#define TYPE_FILTER_BUFFER "filter-buffer"
> +#define TYPE_FILTER_DUMP "filter-dump"
> +
> +#define NETFILTER_ID_BUFFER 1
> +#define NETFILTER_ID_DUMP 2
> +
> +extern const char *const netfilter_type_lookup[];
> +
>  typedef void (FilterSetup) (NetFilterState *nf, Error **errp);
>  typedef void (FilterCleanup) (NetFilterState *nf);
>  /*
> @@ -55,6 +65,7 @@ struct NetFilterState {
>      char *netdev_id;
>      NetClientState *netdev;
>      NetFilterDirection direction;
> +    bool is_default;
>      bool enabled;
>      QTAILQ_ENTRY(NetFilterState) next;
>  };
> diff --git a/net/dump.c b/net/dump.c
> index 88d9582..82727a6 100644
> --- a/net/dump.c
> +++ b/net/dump.c
> @@ -229,8 +229,6 @@ int net_init_dump(const NetClientOptions *opts, const 
> char *name,
>  
>  /* Dumping via filter */
>  
> -#define TYPE_FILTER_DUMP "filter-dump"
> -
>  #define FILTER_DUMP(obj) \
>      OBJECT_CHECK(NetFilterDumpState, (obj), TYPE_FILTER_DUMP)
>  
> diff --git a/net/filter.c b/net/filter.c
> index 4d96301..a126a3b 100644
> --- a/net/filter.c
> +++ b/net/filter.c
> @@ -21,6 +21,11 @@
>  #include "qapi/qmp-input-visitor.h"
>  #include "monitor/monitor.h"
>  
> +const char *const netfilter_type_lookup[] = {
> +    [NETFILTER_ID_BUFFER] = TYPE_FILTER_BUFFER,
> +    [NETFILTER_ID_DUMP] = TYPE_FILTER_DUMP,
> +};
> +
>  ssize_t qemu_netfilter_receive(NetFilterState *nf,
>                                 NetFilterDirection direction,
>                                 NetClientState *sender,
> @@ -200,7 +205,7 @@ static void netfilter_complete(UserCreatable *uc, Error 
> **errp)
>      NetFilterClass *nfc = NETFILTER_GET_CLASS(uc);
>      int queues;
>      Error *local_err = NULL;
> -
> +    char *path = object_get_canonical_path_component(OBJECT(nf));
>  
>      if (!nf->netdev_id) {
>          error_setg(errp, "Parameter 'netdev' is required");
> @@ -225,6 +230,14 @@ static void netfilter_complete(UserCreatable *uc, Error 
> **errp)
>      }
>  
>      nf->netdev = ncs[0];
> +    nf->is_default = !strcmp(path, DEFAULT_FILTER_NAME);
> +    /*
> +    * 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;
> +    }

This seems not very elegant. Besides DEFAULT_FILTER_NAME(TYPE), we may
also want a DEFAULT_FILTER_PROPERTIES? Then you can store the "status"
into properties.

>  
>      if (nfc->setup) {
>          nfc->setup(nf, &local_err);
> diff --git a/net/net.c b/net/net.c
> index ec43105..9630234 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -76,6 +76,12 @@ const char *host_net_devices[] = {
>  
>  int default_net = 1;
>  
> +/*
> + * FIXME: Export this with an option for users to control
> + * this with comand line ?

This could be done in the future.

> + */
> +int default_netfilter = NETFILTER_ID_BUFFER;

Why not just use a string here?

> +
>  /***********************************************************/
>  /* network device redirectors */
>  
> @@ -1032,6 +1038,18 @@ static int net_client_init1(const void *object, int 
> is_netdev, Error **errp)
>          }
>          return -1;
>      }
> +
> +    if (is_netdev) {
> +        const Netdev *netdev = object;
> +        /*
> +        * Here we add each netdev a default filter whose name is 'nop',
> +        * it will disabled by default, Users can enable it when necessary.
> +        */

If we support default properties, the above comment could be removed.

> +        netdev_add_filter(netdev->id,
> +                          netfilter_type_lookup[default_netfilter],
> +                          DEFAULT_FILTER_NAME,

I believe some logic to generate id automatically is needed here.

> +                          errp);
> +    }
>      return 0;
>  }
>