From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60824) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aZCjV-0008Ka-V6 for qemu-devel@nongnu.org; Fri, 26 Feb 2016 02:22:41 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aZCjS-0004Rq-6l for qemu-devel@nongnu.org; Fri, 26 Feb 2016 02:22:37 -0500 Received: from szxga01-in.huawei.com ([58.251.152.64]:40547) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aZCjR-0004Ou-2k for qemu-devel@nongnu.org; Fri, 26 Feb 2016 02:22:34 -0500 References: <1456373149-26256-1-git-send-email-zhang.zhanghailiang@huawei.com> <56CFF58B.2050702@redhat.com> From: Hailiang Zhang Message-ID: <56CFFD02.3080604@huawei.com> Date: Fri, 26 Feb 2016 15:21:38 +0800 MIME-Version: 1.0 In-Reply-To: <56CFF58B.2050702@redhat.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] filter/fiter-buffer: Add a 'status' property for filter object List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jason Wang , qemu-devel@nongnu.org Cc: peter.huangpeng@huawei.com, Yang Hongyang 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 >> Cc: Jason Wang >> Cc: Yang Hongyang >> --- >> 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. >> > > > . >