From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57143) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aOi5M-0000dU-LL for qemu-devel@nongnu.org; Thu, 28 Jan 2016 03:37:49 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aOi5J-0006iN-Es for qemu-devel@nongnu.org; Thu, 28 Jan 2016 03:37:48 -0500 Received: from mx1.redhat.com ([209.132.183.28]:37552) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aOi5J-0006iB-74 for qemu-devel@nongnu.org; Thu, 28 Jan 2016 03:37:45 -0500 References: <1453862428-25570-1-git-send-email-zhangchen.fnst@cn.fujitsu.com> <56A9AAC8.4010803@redhat.com> <56A9C6E4.2050309@cn.fujitsu.com> From: Jason Wang Message-ID: <56A9D349.6090809@redhat.com> Date: Thu, 28 Jan 2016 16:37:29 +0800 MIME-Version: 1.0 In-Reply-To: <56A9C6E4.2050309@cn.fujitsu.com> Content-Type: text/plain; charset=utf-8 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: Zhang Chen , qemu devel Cc: zhanghailiang , Li Zhijian , Gui jianfeng , "eddie.dong" , "Dr. David Alan Gilbert" , Yang Hongyang On 01/28/2016 03:44 PM, Zhang Chen wrote: > > > 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. Ok, but maybe you can try socket backend. I think the protocol should be at least compatible with it. > > >> >> - 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 ? Even more for its basic function to work. E.g, start qemu with: -netdev socket,id=s0,listen=localhost:X -chardev socket,id=c0,host=localhost,pory=Y,server,nowait -object filter-mirror,netdev=hn0,outdev=c0 Then you can inject packet from the socket connected to s0 and see if you can read it from socket that connected from c0 (or your traffic reader). > >> 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. > Why? Did you test your mirroring on the wire? Thanks