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

[Hailiang Zhang <zhang.zhanghailiang@huawei.com>]

On 2016/2/26 14:49, Jason Wang wrote:
>
>
> 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 <zhang.zhanghailiang@huawei.com>
>> Cc: Jason Wang <jasowang@redhat.com>
>> Cc: Yang Hongyang <hongyang.yang@easystack.cn>
>> ---
>> 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.
>

OK, and how to do the split ?
One patch to add the 'status' property and notifier for filter, and another to
realize the status changing callback for buffer filter ?

>> ---
>>   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.
>

OK.

>> +
>>   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.
>

What's the type of 'status' ? 'char *' ?

>>       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.
>

Yes, you are right, it is needed.

>>   }
>>
>>   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.
>

OK.

>> +    } 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.
>

I will fix it. Thanks.

>> +    } 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.
>>
>> -@item -object filter-buffer,id=@var{id},netdev=@var{netdevid},interval=@var{t}[,queue=@var{all|rx|tx}]
>> +@item -object filter-buffer,id=@var{id},netdev=@var{netdevid},interval=@var{t}[,queue=@var{all|rx|tx}][,status=@var{enable|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.
>> +@option{status} 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.
>>
>
>
> .
>