qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] filter/fiter-buffer: Add a 'status' property fo


From: Jason Wang
Subject: Re: [Qemu-devel] [PATCH] filter/fiter-buffer: Add a 'status' property for filter object
Date: Fri, 26 Feb 2016 14:49:47 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1


On 02/25/2016 12:05 PM, zhanghailiang wrote:
> With this property, users can control if this filter is 'enable'
> or 'disable'. The default behavior for filter is enabled.
>
> For buffer filter, we need to release all the buffered packets
> while disabled it. Here we use the 'disable' callback of NetFilterClass
> to realize this capability. We register it with filter_buffer_flush().
> The other types of filters can realize their own 'disable' callback.
>
> We will skip the disabled filter when delivering packets in net layer.
>
> Signed-off-by: zhanghailiang <address@hidden>
> Cc: Jason Wang <address@hidden>
> Cc: Yang Hongyang <address@hidden>
> ---
> This is picked from COLO series, which is to realize the new 'status'
> property for filter.

Looks good overall, just few nits. And let's split this into two patches.

> ---
>  include/net/filter.h |  4 ++++
>  net/filter-buffer.c  |  1 +
>  net/filter.c         | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  qemu-options.hx      |  4 +++-
>  4 files changed, 52 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/filter.h b/include/net/filter.h
> index 5639976..bd27074 100644
> --- a/include/net/filter.h
> +++ b/include/net/filter.h
> @@ -36,12 +36,15 @@ typedef ssize_t (FilterReceiveIOV)(NetFilterState *nc,
>                                     int iovcnt,
>                                     NetPacketSent *sent_cb);
>  
> +typedef void (FilterDisable) (NetFilterState *nf);

Let's rename this to 'FilterStatusChanged' to be aligned with link status.

> +
>  typedef struct NetFilterClass {
>      ObjectClass parent_class;
>  
>      /* optional */
>      FilterSetup *setup;
>      FilterCleanup *cleanup;
> +    FilterDisable *disable;

So we can use 'status_changed' instead of 'disable' here.

>      /* mandatory */
>      FilterReceiveIOV *receive_iov;
>  } NetFilterClass;
> @@ -55,6 +58,7 @@ struct NetFilterState {
>      char *netdev_id;
>      NetClientState *netdev;
>      NetFilterDirection direction;
> +    bool enabled;

'status' is much better.

>      QTAILQ_ENTRY(NetFilterState) next;
>  };
>  
> diff --git a/net/filter-buffer.c b/net/filter-buffer.c
> index 12ad2e3..b1464c9 100644
> --- a/net/filter-buffer.c
> +++ b/net/filter-buffer.c
> @@ -131,6 +131,7 @@ static void filter_buffer_class_init(ObjectClass *oc, 
> void *data)
>      nfc->setup = filter_buffer_setup;
>      nfc->cleanup = filter_buffer_cleanup;
>      nfc->receive_iov = filter_buffer_receive_iov;
> +    nfc->disable = filter_buffer_flush;

I believe we should start and stop the timer during status changed.

>  }
>  
>  static void filter_buffer_get_interval(Object *obj, Visitor *v,
> diff --git a/net/filter.c b/net/filter.c
> index d2a514e..edd18c4 100644
> --- a/net/filter.c
> +++ b/net/filter.c
> @@ -17,6 +17,11 @@
>  #include "qom/object_interfaces.h"
>  #include "qemu/iov.h"
>  
> +static inline bool qemu_can_skip_netfilter(NetFilterState *nf)
> +{
> +    return nf->enabled ? false : true;
> +}
> +
>  ssize_t qemu_netfilter_receive(NetFilterState *nf,
>                                 NetFilterDirection direction,
>                                 NetClientState *sender,
> @@ -25,6 +30,9 @@ ssize_t qemu_netfilter_receive(NetFilterState *nf,
>                                 int iovcnt,
>                                 NetPacketSent *sent_cb)
>  {
> +    if (qemu_can_skip_netfilter(nf)) {
> +        return 0;
> +    }
>      if (nf->direction == direction ||
>          nf->direction == NET_FILTER_DIRECTION_ALL) {
>          return NETFILTER_GET_CLASS(OBJECT(nf))->receive_iov(
> @@ -134,8 +142,41 @@ static void netfilter_set_direction(Object *obj, int 
> direction, Error **errp)
>      nf->direction = direction;
>  }
>  
> +static char *netfilter_get_status(Object *obj, Error **errp)
> +{
> +    NetFilterState *nf = NETFILTER(obj);
> +
> +    if (nf->enabled) {
> +        return g_strdup("enable");

Let's use "on" and "off" here.

> +    } else {
> +        return g_strdup("disable");
> +    }
> +}
> +
> +static void netfilter_set_status(Object *obj, const char *str, Error **errp)
> +{
> +    NetFilterState *nf = NETFILTER(obj);
> +    NetFilterClass *nfc = NETFILTER_GET_CLASS(obj);
> +
> +    if (!strcmp(str, "enable")) {
> +        nf->enabled = true;

We'd better also call status changed here, in case the filter (e.g
future implementation) need some setup.

> +    } else if (!strcmp(str, "disable")) {
> +        nf->enabled = false;
> +        if (nfc->disable) {
> +            nfc->disable(nf);
> +        }
> +    } else {
> +        error_setg(errp, "Invalid value for netfilter status, "
> +                         "should be 'enable' or 'disable'");
> +    }
> +}
> +
>  static void netfilter_init(Object *obj)
>  {
> +    NetFilterState *nf = NETFILTER(obj);
> +
> +    nf->enabled = true;
> +
>      object_property_add_str(obj, "netdev",
>                              netfilter_get_netdev_id, netfilter_set_netdev_id,
>                              NULL);
> @@ -143,6 +184,9 @@ static void netfilter_init(Object *obj)
>                               NetFilterDirection_lookup,
>                               netfilter_get_direction, 
> netfilter_set_direction,
>                               NULL);
> +    object_property_add_str(obj, "status",
> +                            netfilter_get_status, netfilter_set_status,
> +                            NULL);
>  }
>  
>  static void netfilter_complete(UserCreatable *uc, Error **errp)
> diff --git a/qemu-options.hx b/qemu-options.hx
> index f528405..15b8e48 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3746,11 +3746,13 @@ version by providing the @var{passwordid} parameter. 
> This provides
>  the ID of a previously created @code{secret} object containing the
>  password for decryption.
>  
> address@hidden -object 
> filter-buffer,address@hidden,address@hidden,address@hidden,address@hidden|rx|tx}]
> address@hidden -object 
> filter-buffer,address@hidden,address@hidden,address@hidden,address@hidden|rx|tx}][,address@hidden|disable}]
>  
>  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
>  until the end of the interval. Interval is in microseconds.
> address@hidden is optional that indicate whether the netfilter is enabled
> +or disabled, the default status for netfilter will be enabled.
>  
>  queue @var{all|rx|tx} is an option that can be applied to any netfilter.
>  




reply via email to

[Prev in Thread] Current Thread [Next in Thread]