From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54533) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZMVA7-0002q5-Ns for qemu-devel@nongnu.org; Tue, 04 Aug 2015 01:53:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZMVA4-00064m-GX for qemu-devel@nongnu.org; Tue, 04 Aug 2015 01:53:19 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50239) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZMVA4-00064S-8T for qemu-devel@nongnu.org; Tue, 04 Aug 2015 01:53:16 -0400 Message-ID: <55C05343.5020101@redhat.com> Date: Tue, 04 Aug 2015 13:53:07 +0800 From: Jason Wang MIME-Version: 1.0 References: <1438590616-21142-1-git-send-email-yanghy@cn.fujitsu.com> <1438590616-21142-5-git-send-email-yanghy@cn.fujitsu.com> <55C045FB.0@redhat.com> <55C05019.7000808@cn.fujitsu.com> In-Reply-To: <55C05019.7000808@cn.fujitsu.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 04/12] net: add/remove filters from network backend List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Yang Hongyang , qemu-devel@nongnu.org Cc: thuth@redhat.com, zhang.zhanghailiang@huawei.com, lizhijian@cn.fujitsu.com, dgilbert@redhat.com, mrhines@linux.vnet.ibm.com, stefanha@redhat.com On 08/04/2015 01:39 PM, Yang Hongyang wrote: > On 08/04/2015 12:56 PM, Jason Wang wrote: >> >> >> On 08/03/2015 04:30 PM, Yang Hongyang wrote: >>> add/remove filters from network backend >>> >>> Signed-off-by: Yang Hongyang >>> --- >>> include/net/net.h | 8 ++++++++ >>> net/filter.c | 4 ++-- >>> net/net.c | 33 +++++++++++++++++++++++++++++++++ >>> 3 files changed, 43 insertions(+), 2 deletions(-) >>> >>> diff --git a/include/net/net.h b/include/net/net.h >>> index 6a6cbef..5c5c109 100644 >>> --- a/include/net/net.h >>> +++ b/include/net/net.h >>> @@ -40,6 +40,11 @@ typedef struct NICConf { >>> >>> >>> /* Net clients */ >>> +typedef struct Filter Filter; >>> +struct Filter { >>> + NetFilterState *nf; >>> + QTAILQ_ENTRY(Filter) next; >>> +}; >> >> Didn't understand why need another structure here. Could we just use >> NetFilterState? > > There's a QTAILQ_ENTRY next in NetFilterState, but used by filter > layer, so > that we can manage all filters together. > > This struct used by netdev. filter belongs to the specific netdev is > in this queue. > > Just use NetFilterState in this case need to introduce another > QTAILQ_ENTRY > in NetFilterState, maybe will make it confusing? Probably not. > and it seems that it's > not common to have 2 QTAILQ_ENTRYs in one struct? I think this is ok. You can use something like global_list. > >> >>> >>> typedef void (NetPoll)(NetClientState *, bool enable); >>> typedef int (NetCanReceive)(NetClientState *); >>> @@ -92,6 +97,7 @@ struct NetClientState { >>> NetClientDestructor *destructor; >>> unsigned int queue_index; >>> unsigned rxfilter_notify_enabled:1; >>> + QTAILQ_HEAD(, Filter) filters; >>> }; >>> >>> typedef struct NICState { >>> @@ -109,6 +115,8 @@ NetClientState >>> *qemu_new_net_client(NetClientInfo *info, >>> NetClientState *peer, >>> const char *model, >>> const char *name); >>> +int qemu_netdev_add_filter(NetClientState *nc, NetFilterState *nf); >>> +void qemu_netdev_remove_filter(NetClientState *nc, NetFilterState >>> *nf); >>> NICState *qemu_new_nic(NetClientInfo *info, >>> NICConf *conf, >>> const char *model, >>> diff --git a/net/filter.c b/net/filter.c >>> index 86eed8a..1ae9344 100644 >>> --- a/net/filter.c >>> +++ b/net/filter.c >>> @@ -38,14 +38,14 @@ NetFilterState >>> *qemu_new_net_filter(NetFilterInfo *info, >>> nf->netdev = netdev; >>> nf->chain = chain; >>> QTAILQ_INSERT_TAIL(&net_filters, nf, next); >>> - /* TODO: attach netfilter to netdev */ >>> + qemu_netdev_add_filter(netdev, nf); >>> >>> return nf; >>> } >>> >>> static void qemu_cleanup_net_filter(NetFilterState *nf) >>> { >>> - /* TODO: remove netfilter from netdev */ >>> + qemu_netdev_remove_filter(nf->netdev, nf); >>> >>> QTAILQ_REMOVE(&net_filters, nf, next); >>> >>> diff --git a/net/net.c b/net/net.c >>> index 28a5597..00c5ca3 100644 >>> --- a/net/net.c >>> +++ b/net/net.c >>> @@ -287,6 +287,7 @@ static void qemu_net_client_setup(NetClientState >>> *nc, >>> >>> nc->incoming_queue = qemu_new_net_queue(nc); >>> nc->destructor = destructor; >>> + QTAILQ_INIT(&nc->filters); >>> } >>> >>> NetClientState *qemu_new_net_client(NetClientInfo *info, >>> @@ -305,6 +306,38 @@ NetClientState >>> *qemu_new_net_client(NetClientInfo *info, >>> return nc; >>> } >>> >>> +int qemu_netdev_add_filter(NetClientState *nc, NetFilterState *nf) >>> +{ >>> + Filter *filter = g_malloc0(sizeof(*filter)); >>> + >>> + filter->nf = nf; >>> + QTAILQ_INSERT_TAIL(&nc->filters, filter, next); >>> + return 0; >>> +} >>> + >>> +static void remove_filter(NetClientState *nc, Filter *filter) >>> +{ >>> + if (!filter) { >>> + return; >>> + } >>> + >>> + QTAILQ_REMOVE(&nc->filters, filter, next); >>> + g_free(filter); >>> +} >>> + >>> +void qemu_netdev_remove_filter(NetClientState *nc, NetFilterState *nf) >>> +{ >>> + Filter *filter = NULL; >>> + >>> + QTAILQ_FOREACH(filter, &nc->filters, next) { >>> + if (filter->nf == nf) { >>> + break; >>> + } >>> + } >>> + >>> + remove_filter(nc, filter); >>> +} >>> + >>> NICState *qemu_new_nic(NetClientInfo *info, >>> NICConf *conf, >>> const char *model, >> >> Another thing may need consider is qemu_flush_queued_packets(). Look >> like we need also flush packets inside each filter in this case? > > When a filter is removed, the filter itself will flush the packets. > when a filter queued a packet, we assume the filter will take care > of it. the filter is not a netdev, so I think we do not need to flush > packets in qemu_flush_queued_packets(), otherwise, the buffer filter > will be useless, because when qemu_flush_queued_packets() is called, > it will flush the packets, it's not what we want. Right.