Re: [Qemu-devel] [PATCH v2 1/2] filter: Add 'status' property for filter object

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

On 2016/2/29 15:26, Jason Wang wrote:
>
>
> On 02/29/2016 09:46 AM, zhanghailiang wrote:
>> With this property, users can control if this filter is 'on'
>> or 'off'. The default behavior for filter is 'on'.
>>
>> For some types of filters, they may need to react to status changing,
>> So here, we introduced status changing callback/notifier for filter class.
>>
>> We will skip the disabled ('off') 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>
>> ---
>> v2:
>> - Split the processing of buffer-filter into a new patch (Jason)
>> - Use 'status' instead of 'enabled' to store the filter state (Jason)
>> - Rename FilterDisable() callback to FilterStatusChanged(Jason)
>> ---
>
> Thanks, looks good, just few nits.
>
>>   include/net/filter.h |  4 ++++
>>   net/filter.c         | 42 ++++++++++++++++++++++++++++++++++++++++++
>>   qemu-options.hx      |  4 +++-
>>   3 files changed, 49 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/net/filter.h b/include/net/filter.h
>> index 5639976..ebef0dc 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 (FilterStatusChanged) (NetFilterState *nf, Error **errp);
>> +
>>   typedef struct NetFilterClass {
>>       ObjectClass parent_class;
>>
>>       /* optional */
>>       FilterSetup *setup;
>>       FilterCleanup *cleanup;
>> +    FilterStatusChanged *status_changed;
>>       /* mandatory */
>>       FilterReceiveIOV *receive_iov;
>>   } NetFilterClass;
>> @@ -55,6 +58,7 @@ struct NetFilterState {
>>       char *netdev_id;
>>       NetClientState *netdev;
>>       NetFilterDirection direction;
>> +    char *status;
>
> Let's use bool instead.
>

Er, then status=true means 'on' ? false means 'off' ?
That looks odd.  What about using 'bool status_on' ?

>
> .
>