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

[Jason Wang <jasowang@redhat.com>]

On 02/29/2016 03:34 PM, Hailiang Zhang wrote:
> 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' ?

Or just "bool on" :)

>
>>
>> .
>>
>
>