Re: [Qemu-devel] [PATCH COLO-Frame v14 31/40] net/filter: Add a 'status' property for filter object

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

Hi Jason,

Happy spring festival! ;)

On 2016/2/18 11:00, Jason Wang wrote:
>
>
> On 02/06/2016 05:28 PM, zhanghailiang wrote:
>> With this property, users can control if this filter is 'enable'
>> or 'disable'. The default behavior for filter is enabled.
>>
>> 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>
>> ---
>>   include/net/filter.h |  1 +
>>   net/filter.c         | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 46 insertions(+)
>>
>> diff --git a/include/net/filter.h b/include/net/filter.h
>> index 5639976..af3c53c 100644
>> --- a/include/net/filter.h
>> +++ b/include/net/filter.h
>> @@ -55,6 +55,7 @@ struct NetFilterState {
>>       char *netdev_id;
>>       NetClientState *netdev;
>>       NetFilterDirection direction;
>> +    bool enabled;
>>       QTAILQ_ENTRY(NetFilterState) next;
>>   };
>>
>> diff --git a/net/filter.c b/net/filter.c
>> index d2a514e..5551cf1 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_need_skip_netfilter(NetFilterState *nf)
>> +{
>> +    return nf->enabled ? false : true;
>> +}
>
> Suggest to rename this to qemu_netfilter_can_skip.
>

OK, i will fix it.

>> +
>>   ssize_t qemu_netfilter_receive(NetFilterState *nf,
>>                                  NetFilterDirection direction,
>>                                  NetClientState *sender,
>> @@ -25,6 +30,10 @@ ssize_t qemu_netfilter_receive(NetFilterState *nf,
>>                                  int iovcnt,
>>                                  NetPacketSent *sent_cb)
>>   {
>> +    /* Don't go through the filter if it is disabled */
>> +    if (qemu_need_skip_netfilter(nf)) {
>> +        return 0;
>> +    }
>
> The code is self explained, so the commnet is useless.
>

OK, i will remove it.

>>       if (nf->direction == direction ||
>>           nf->direction == NET_FILTER_DIRECTION_ALL) {
>>           return NETFILTER_GET_CLASS(OBJECT(nf))->receive_iov(
>> @@ -134,8 +143,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");
>> +    } else {
>> +        return g_strdup("disable");
>> +    }
>> +}
>> +
>> +static void netfilter_set_status(Object *obj, const char *str, Error **errp)
>> +{
>> +    NetFilterState *nf = NETFILTER(obj);
>> +
>> +    if (!strcmp(str, "enable")) {
>> +        nf->enabled = true;
>> +    } else if (!strcmp(str, "disable")) {
>> +        nf->enabled = false;
>
> Do we need a filter specific callback here to drain filter's queue? E.g
> for filter-buffer ,need to release all the packets that has been buffered.
>

I don't think we need to do that here, we drain the filter's queue explicitly
when we try to change the filter's status .
Besides, it seems that, we didn't have a callback in filter layer to process
the queued packets for different types of filters.

>> +    } else {
>> +        error_setg(errp, "Invalid value for netfilter status, "
>> +                         "should be 'enable' or 'disable'");
>> +    }
>> +}
>> +
>>   static void netfilter_init(Object *obj)
>>   {
>> +    NetFilterState *nf = NETFILTER(obj);
>> +
>> +    /*
>> +    * If not configured with 'status' property, the default status
>> +    * for netfilter will be enabled.
>> +    */
>> +    nf->enabled = true;
>
> The code is clear too, so the comment need to be moved to qemu-options.hx.
>

OK, i will fix it in next version.

Thanks,
Hailiang

>> +
>>       object_property_add_str(obj, "netdev",
>>                               netfilter_get_netdev_id, netfilter_set_netdev_id,
>>                               NULL);
>> @@ -143,6 +185,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)
>
>
> .
>