From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46759) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aOhGU-0002Ms-F8 for qemu-devel@nongnu.org; Thu, 28 Jan 2016 02:45:18 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aOhGO-0001WU-TB for qemu-devel@nongnu.org; Thu, 28 Jan 2016 02:45:14 -0500 Received: from [59.151.112.132] (port=62841 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aOhGO-0001PA-0G for qemu-devel@nongnu.org; Thu, 28 Jan 2016 02:45:08 -0500 References: <1453862428-25570-1-git-send-email-zhangchen.fnst@cn.fujitsu.com> <56A9AAC8.4010803@redhat.com> From: Zhang Chen Message-ID: <56A9C6E4.2050309@cn.fujitsu.com> Date: Thu, 28 Jan 2016 15:44:36 +0800 MIME-Version: 1.0 In-Reply-To: <56A9AAC8.4010803@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V2] net/traffic-mirror:Add traffic-mirror List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jason Wang , qemu devel Cc: Li Zhijian , Gui jianfeng , "eddie.dong" , "Dr. David Alan Gilbert" , Yang Hongyang , zhanghailiang On 01/28/2016 01:44 PM, Jason Wang wrote: > > On 01/27/2016 10:40 AM, Zhang Chen wrote: >> From: ZhangChen >> >> Traffic-mirror is a netfilter plugin. >> It gives qemu the ability to copy and mirror guest's >> net packet. we output packet to chardev. >> >> usage: >> >> -netdev tap,id=hn0 >> -chardev socket,id=mirror0,host=ip_primary,port=X,server,nowait >> -traffic-mirror,id=m0,netdev=hn0,queue=tx/rx/all,outdev=mirror0 >> >> Signed-off-by: ZhangChen >> Signed-off-by: Wen Congyang >> Reviewed-by: Yang Hongyang > Thanks for the patch. Several questions: > > - I'm curious about how the patch was tested? Simple setup e.g: > > -netdev tap,id=hn0 -device virtio-net-pci,netdev=hn0 -chardev > socket,id=c0,host=localhost,port=4444,server,nowait -object > traffic-mirror,netdev=hn0,outdev=c0,id=f0 -netdev > socket,id=s0,connect=127.0.0.1:4444 -device e1000,netdev=s0 > > does not works for me. I test it in this way. primary: -netdev tap,id=hn0 -device e1000,netdev=hn0 -chardev socket,id=mirror0,host=3.3.3.3,port=9003,server,nowait -object traffic-mirror,id=f0,netdev=hn0,queue=tx,outdev=mirror0 secondary: -netdev tap,id=hn0 -device e1000,netdev=hn0 -chardev socket,id=mirror0,host=3.3.3.3,port=9003 -object traffic-reader,id=f1,netdev=hn0,queue=rx,indev=mirror0 I write a traffic-reader demo to read chardev socket and print it in monitor. > > - Is a reliable mirroring (e.g no packet drops during mirroring) is > needed for COLO? If yes, this patch seems could not guarantee this. I will fix it in V3 > - Please consider to write a unit test for this patch. write a unit test like tests/test-netfilter.c ? > And see comments below. > > Thanks > > >> --- >> net/Makefile.objs | 1 + >> net/traffic-mirror.c | 173 +++++++++++++++++++++++++++++++++++++++++++++++++++ >> qemu-options.hx | 5 ++ >> vl.c | 3 +- >> 4 files changed, 181 insertions(+), 1 deletion(-) >> create mode 100644 net/traffic-mirror.c >> >> diff --git a/net/Makefile.objs b/net/Makefile.objs >> index 5fa2f97..de06ebe 100644 >> --- a/net/Makefile.objs >> +++ b/net/Makefile.objs >> @@ -15,3 +15,4 @@ common-obj-$(CONFIG_VDE) += vde.o >> common-obj-$(CONFIG_NETMAP) += netmap.o >> common-obj-y += filter.o >> common-obj-y += filter-buffer.o >> +common-obj-y += traffic-mirror.o > Let's s/traffic-mirror/filter-mirror/g to be consistent with other filters. > OK~ I will fix it in V3 >> diff --git a/net/traffic-mirror.c b/net/traffic-mirror.c >> new file mode 100644 >> index 0000000..bed915c >> --- /dev/null >> +++ b/net/traffic-mirror.c >> @@ -0,0 +1,173 @@ >> +/* >> + * Copyright (c) 2016 HUAWEI TECHNOLOGIES CO., LTD. >> + * Copyright (c) 2016 FUJITSU LIMITED >> + * Copyright (c) 2016 Intel Corporation >> + * >> + * Author: Zhang Chen >> + * >> + * This work is licensed under the terms of the GNU GPL, version 2 or >> + * later. See the COPYING file in the top-level directory. >> + */ >> + >> +#include "net/filter.h" >> +#include "net/net.h" >> +#include "qemu-common.h" >> +#include "qapi/qmp/qerror.h" >> +#include "qapi-visit.h" >> +#include "qom/object.h" >> +#include "qemu/main-loop.h" >> +#include "qemu/error-report.h" >> +#include "trace.h" >> +#include "sysemu/char.h" >> +#include "qemu/iov.h" >> + >> +#define FILTER_TRAFFIC_MIRROR(obj) \ >> + OBJECT_CHECK(MirrorState, (obj), TYPE_FILTER_TRAFFIC_MIRROR) >> + >> +#define TYPE_FILTER_TRAFFIC_MIRROR "traffic-mirror" >> + >> +typedef struct MirrorState { >> + NetFilterState parent_obj; >> + char *outdev; >> + CharDriverState *chr_out; >> + >> +} MirrorState; >> + >> +static ssize_t traffic_mirror_send(NetFilterState *nf, >> + const struct iovec *iov, >> + int iovcnt) >> +{ >> + MirrorState *s = FILTER_TRAFFIC_MIRROR(nf); >> + ssize_t ret = 0; >> + ssize_t size = 0; >> + char *buf; >> + >> + size = iov_size(iov, iovcnt); >> + if (!size) { >> + return 0; >> + } >> + >> + buf = g_malloc0(size); >> + iov_to_buf(iov, iovcnt, 0, buf, size); >> + ret = qemu_chr_fe_write(s->chr_out, (uint8_t *)&size, sizeof(size)); > htonl(size)? We do not need this. >> + if (ret < 0) { > This check is not sufficient, for some reason, only part of the packets > maybe sent by the socket. Need to handle this properly, otherwise it may > confuse receiver. I will fix it in next version. >> + g_free(buf); >> + return ret; >> + } >> + >> + ret = qemu_chr_fe_write(s->chr_out, (uint8_t *)buf, size); >> + g_free(buf); >> + return ret; > Ditto. I will fix it in next version. >> +} >> + >> +static ssize_t traffic_mirror_receive_iov(NetFilterState *nf, >> + NetClientState *sender, >> + unsigned flags, >> + const struct iovec *iov, >> + int iovcnt, >> + NetPacketSent *sent_cb) >> +{ >> + /* >> + * We copy and mirror packet to outdev, >> + * then put back the packet. >> + */ > The code could explain itself, so the comment is unnecessary. OK,I will remove it. Thanks zhangchen >> + ssize_t ret = 0; >> + >> + ret = traffic_mirror_send(nf, iov, iovcnt); >> + if (ret < 0) { >> + error_report("traffic_mirror_send failed"); > Monitor could be flooded by this. > >> + } >> + >> + return 0; >> +} >> + > Other looks good. > > > > . > -- Thanks zhangchen