From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52473) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1acrGp-0002EW-Ab for qemu-devel@nongnu.org; Mon, 07 Mar 2016 04:16:09 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1acrGk-0007EO-Ka for qemu-devel@nongnu.org; Mon, 07 Mar 2016 04:16:07 -0500 Received: from [59.151.112.132] (port=50055 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1acrGi-0007Ds-KG for qemu-devel@nongnu.org; Mon, 07 Mar 2016 04:16:02 -0500 References: <1457092906-19828-1-git-send-email-zhangchen.fnst@cn.fujitsu.com> <1457092906-19828-3-git-send-email-zhangchen.fnst@cn.fujitsu.com> <56DD3414.2040301@redhat.com> From: Zhang Chen Message-ID: <56DD470F.2050702@cn.fujitsu.com> Date: Mon, 7 Mar 2016 17:17:03 +0800 MIME-Version: 1.0 In-Reply-To: <56DD3414.2040301@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V3 2/3] net/filter-mirror:Add filter-redirector func List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jason Wang , qemu devel Cc: Li Zhijian , Gui jianfeng , "eddie.dong" , zhanghailiang , "Dr. David Alan Gilbert" , Yang Hongyang On 03/07/2016 03:56 PM, Jason Wang wrote: > > On 03/04/2016 08:01 PM, Zhang Chen wrote: >> Filter-redirector is a netfilter plugin. >> It gives qemu the ability to redirect net packet. >> redirector can redirect filter's net packet to outdev. >> and redirect indev's packet to filter. >> >> filter >> + >> | >> | >> redirector | >> +--------------+ >> | | | >> | | | >> | | | >> indev +-----------+ +----------> outdev >> | | | >> | | | >> | | | >> +--------------+ >> | >> | >> v >> filter >> >> usage: >> >> -netdev user,id=hn0 >> -chardev socket,id=s0,host=ip_primary,port=X,server,nowait >> -chardev socket,id=s1,host=ip_primary,port=Y,server,nowait >> -filter-redirector,id=r0,netdev=hn0,queue=tx/rx/all,indev=s0,outdev=s1 >> >> Signed-off-by: Zhang Chen >> Signed-off-by: Wen Congyang >> --- >> net/filter-mirror.c | 211 ++++++++++++++++++++++++++++++++++++++++++++++++++++ >> qemu-options.hx | 8 ++ >> vl.c | 3 +- >> 3 files changed, 221 insertions(+), 1 deletion(-) >> >> diff --git a/net/filter-mirror.c b/net/filter-mirror.c >> index 4ff7619..d137168 100644 >> --- a/net/filter-mirror.c >> +++ b/net/filter-mirror.c >> @@ -25,11 +25,19 @@ >> #define FILTER_MIRROR(obj) \ >> OBJECT_CHECK(MirrorState, (obj), TYPE_FILTER_MIRROR) >> >> +#define FILTER_REDIRECTOR(obj) \ >> + OBJECT_CHECK(MirrorState, (obj), TYPE_FILTER_REDIRECTOR) >> + >> #define TYPE_FILTER_MIRROR "filter-mirror" >> +#define TYPE_FILTER_REDIRECTOR "filter-redirector" >> +#define REDIRECT_HEADER_LEN sizeof(uint32_t) >> >> typedef struct MirrorState { >> NetFilterState parent_obj; >> + NetQueue *incoming_queue; >> + char *indev; >> char *outdev; >> + CharDriverState *chr_in; >> CharDriverState *chr_out; >> } MirrorState; >> >> @@ -67,6 +75,68 @@ err: >> return ret < 0 ? ret : -EIO; >> } >> >> +static int redirector_chr_can_read(void *opaque) >> +{ >> + return REDIRECT_HEADER_LEN; >> +} >> + >> +static void redirector_chr_read(void *opaque, const uint8_t *buf, int size) >> +{ >> + NetFilterState *nf = opaque; >> + MirrorState *s = FILTER_REDIRECTOR(nf); >> + uint32_t len; >> + int ret = 0; >> + uint8_t *recv_buf; >> + >> + memcpy(&len, buf, size); > stack overflow if size > sizeof(len)? >> + if (size < REDIRECT_HEADER_LEN) { >> + ret = qemu_chr_fe_read_all(s->chr_in, ((uint8_t *)&len) + size, >> + REDIRECT_HEADER_LEN - size); > There maybe some misunderstanding for my previous reply. You can have a > look at net_socket_send() for reference. You could > > - use a buffer for storing len > - each time when you receive partial len, store them in the buffer and > advance the pointer until you receive at least sizeof(len) bytes. > > Then you can proceed and do something similar when you're receiving the > packet itself. >> + if (ret != (REDIRECT_HEADER_LEN - size)) { >> + error_report("filter-redirector recv size failed"); >> + return; >> + } >> + } >> + >> + len = ntohl(len); >> + >> + if (len > 0 && len < NET_BUFSIZE) { >> + recv_buf = g_malloc(len); >> + ret = qemu_chr_fe_read_all(s->chr_in, recv_buf, len); >> + if (ret != len) { >> + error_report("filter-redirector recv buf failed"); >> + g_free(recv_buf); >> + return; >> + } >> + >> + if (nf->direction == NET_FILTER_DIRECTION_ALL || >> + nf->direction == NET_FILTER_DIRECTION_TX) { >> + ret = qemu_net_queue_send(s->incoming_queue, nf->netdev, >> + 0, (const uint8_t *)recv_buf, len, NULL); > Rethink about this, we don't queue any packet in incoming_queue, so > probably there's no need to have a incoming_queue for redirector. We can > just use qemu_netfilter_pass_to_next() here. OK~~ I will fix it. > >> + >> + if (ret < 0) { >> + /* >> + *FIXME: just report an error, let NET_FILTER_DIRECTION_RX have chance to pass >> + * to next filter ? >> + */ >> + error_report("filter-redirector out to guest failed"); >> + } >> + } >> + >> + if (nf->direction == NET_FILTER_DIRECTION_ALL || >> + nf->direction == NET_FILTER_DIRECTION_RX) { >> + struct iovec iov = { >> + .iov_base = recv_buf, >> + .iov_len = sizeof(recv_buf) >> + }; >> + qemu_netfilter_pass_to_next(nf->netdev->peer, 0, &iov, 1, nf); > Another issue not relate to this patch. Looks like we can rename this to > qemu_netfilter_iterate(), which seems better. Care to send a patch of this? OK, I will send a patch for this~~ > >> + } >> + g_free(recv_buf); >> + } else { >> + error_report("filter-redirector recv len failed"); > What will happen if the socket was closed at the other end? Can we get > size == 0 here? Btw, the grammar looks not correct :) I will fix it in next version. >> + } >> +} >> + >> static ssize_t filter_mirror_receive_iov(NetFilterState *nf, >> NetClientState *sender, >> unsigned flags, >> @@ -89,6 +159,27 @@ static ssize_t filter_mirror_receive_iov(NetFilterState *nf, >> return 0; >> } >> >> +static ssize_t filter_redirector_receive_iov(NetFilterState *nf, >> + NetClientState *sender, >> + unsigned flags, >> + const struct iovec *iov, >> + int iovcnt, >> + NetPacketSent *sent_cb) >> +{ >> + MirrorState *s = FILTER_REDIRECTOR(nf); >> + int ret; >> + >> + if (s->chr_out) { >> + ret = filter_mirror_send(s->chr_out, iov, iovcnt); >> + if (ret) { >> + error_report("filter_mirror_send failed(%s)", strerror(-ret)); >> + } >> + return iov_size(iov, iovcnt); >> + } else { >> + return 0; >> + } >> +} >> + >> static void filter_mirror_cleanup(NetFilterState *nf) >> { >> MirrorState *s = FILTER_MIRROR(nf); >> @@ -98,6 +189,18 @@ static void filter_mirror_cleanup(NetFilterState *nf) >> } >> } >> >> +static void filter_redirector_cleanup(NetFilterState *nf) >> +{ >> + MirrorState *s = FILTER_REDIRECTOR(nf); >> + >> + if (s->chr_in) { >> + qemu_chr_fe_release(s->chr_in); >> + } >> + if (s->chr_out) { >> + qemu_chr_fe_release(s->chr_out); >> + } >> +} >> + >> static void filter_mirror_setup(NetFilterState *nf, Error **errp) >> { >> MirrorState *s = FILTER_MIRROR(nf); >> @@ -121,6 +224,47 @@ static void filter_mirror_setup(NetFilterState *nf, Error **errp) >> } >> } >> >> +static void filter_redirector_setup(NetFilterState *nf, Error **errp) >> +{ >> + MirrorState *s = FILTER_REDIRECTOR(nf); >> + >> + if (!s->indev && !s->outdev) { >> + error_setg(errp, "filter redirector needs 'indev' or " >> + "'outdev' at least one property set"); >> + return; >> + } else if (s->indev && s->outdev) { >> + if (!strcmp(s->indev, s->outdev)) { >> + error_setg(errp, "filter redirector needs 'indev' and " >> + "'outdev' are not same"); >> + return; >> + } >> + } >> + >> + if (s->indev) { >> + s->chr_in = qemu_chr_find(s->indev); >> + if (s->chr_in == NULL) { >> + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, >> + "IN Device '%s' not found", s->indev); >> + return; >> + } >> + >> + qemu_chr_fe_claim_no_fail(s->chr_in); >> + qemu_chr_add_handlers(s->chr_in, redirector_chr_can_read, >> + redirector_chr_read, NULL, nf); >> + s->incoming_queue = qemu_new_net_queue(qemu_netfilter_pass_to_next, nf); >> + } >> + >> + if (s->outdev) { >> + s->chr_out = qemu_chr_find(s->outdev); >> + if (s->chr_out == NULL) { >> + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, >> + "OUT Device '%s' not found", s->outdev); >> + return; >> + } >> + qemu_chr_fe_claim_no_fail(s->chr_out); >> + } >> +} >> + >> static void filter_mirror_class_init(ObjectClass *oc, void *data) >> { >> NetFilterClass *nfc = NETFILTER_CLASS(oc); >> @@ -130,6 +274,31 @@ static void filter_mirror_class_init(ObjectClass *oc, void *data) >> nfc->receive_iov = filter_mirror_receive_iov; >> } >> >> +static void filter_redirector_class_init(ObjectClass *oc, void *data) >> +{ >> + NetFilterClass *nfc = NETFILTER_CLASS(oc); >> + >> + nfc->setup = filter_redirector_setup; >> + nfc->cleanup = filter_redirector_cleanup; >> + nfc->receive_iov = filter_redirector_receive_iov; >> +} >> + >> +static char *filter_redirector_get_indev(Object *obj, Error **errp) >> +{ >> + MirrorState *s = FILTER_REDIRECTOR(obj); >> + >> + return g_strdup(s->indev); >> +} >> + >> +static void >> +filter_redirector_set_indev(Object *obj, const char *value, Error **errp) >> +{ >> + MirrorState *s = FILTER_REDIRECTOR(obj); >> + >> + g_free(s->indev); >> + s->indev = g_strdup(value); >> +} >> + >> static char *filter_mirror_get_outdev(Object *obj, Error **errp) >> { >> MirrorState *s = FILTER_MIRROR(obj); >> @@ -151,12 +320,36 @@ filter_mirror_set_outdev(Object *obj, const char *value, Error **errp) >> } >> } >> >> +static char *filter_redirector_get_outdev(Object *obj, Error **errp) >> +{ >> + MirrorState *s = FILTER_REDIRECTOR(obj); >> + >> + return g_strdup(s->outdev); >> +} >> + >> +static void >> +filter_redirector_set_outdev(Object *obj, const char *value, Error **errp) >> +{ >> + MirrorState *s = FILTER_REDIRECTOR(obj); >> + >> + g_free(s->outdev); >> + s->outdev = g_strdup(value); >> +} >> + >> static void filter_mirror_init(Object *obj) >> { >> object_property_add_str(obj, "outdev", filter_mirror_get_outdev, >> filter_mirror_set_outdev, NULL); >> } >> >> +static void filter_redirector_init(Object *obj) >> +{ >> + object_property_add_str(obj, "indev", filter_redirector_get_indev, >> + filter_redirector_set_indev, NULL); >> + object_property_add_str(obj, "outdev", filter_redirector_get_outdev, >> + filter_redirector_set_outdev, NULL); >> +} >> + >> static void filter_mirror_fini(Object *obj) >> { >> MirrorState *s = FILTER_MIRROR(obj); >> @@ -164,6 +357,23 @@ static void filter_mirror_fini(Object *obj) >> g_free(s->outdev); >> } >> >> +static void filter_redirector_fini(Object *obj) >> +{ >> + MirrorState *s = FILTER_REDIRECTOR(obj); >> + >> + g_free(s->indev); >> + g_free(s->outdev); >> +} >> + >> +static const TypeInfo filter_redirector_info = { >> + .name = TYPE_FILTER_REDIRECTOR, >> + .parent = TYPE_NETFILTER, >> + .class_init = filter_redirector_class_init, >> + .instance_init = filter_redirector_init, >> + .instance_finalize = filter_redirector_fini, >> + .instance_size = sizeof(MirrorState), >> +}; >> + >> static const TypeInfo filter_mirror_info = { >> .name = TYPE_FILTER_MIRROR, >> .parent = TYPE_NETFILTER, >> @@ -176,6 +386,7 @@ static const TypeInfo filter_mirror_info = { >> static void register_types(void) >> { >> type_register_static(&filter_mirror_info); >> + type_register_static(&filter_redirector_info); >> } >> >> type_init(register_types); >> diff --git a/qemu-options.hx b/qemu-options.hx >> index ca27863..84584e3 100644 >> --- a/qemu-options.hx >> +++ b/qemu-options.hx >> @@ -3764,6 +3764,14 @@ queue @var{all|rx|tx} is an option that can be applied to any netfilter. >> filter-mirror on netdev @var{netdevid},mirror net packet to chardev >> @var{chardevid} >> >> +@item -object filter-redirector,id=@var{id},netdev=@var{netdevid},indev=@var{chardevid}, >> +outdev=@var{chardevid}[,queue=@var{all|rx|tx}] >> + >> +filter-redirector on netdev @var{netdevid},redirect filter's net packet to chardev >> +@var{chardevid},and redirect indev's packet to filter. >> +Create a filter-redirector we need to differ outdev id from indev id, id can not >> +be the same. we can just use indev or outdev, but at least one to be set. > Not a native English speaker, but this looks better: > > At least one of indev or outdev need to be specified. I will fix it in next version. Thanks zhangchen > >> + >> @item -object filter-dump,id=@var{id},netdev=@var{dev},file=@var{filename}][,maxlen=@var{len}] >> >> Dump the network traffic on netdev @var{dev} to the file specified by >> diff --git a/vl.c b/vl.c >> index d68533a..c389a3f 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -2799,7 +2799,8 @@ static bool object_create_initial(const char *type) >> */ >> if (g_str_equal(type, "filter-buffer") || >> g_str_equal(type, "filter-dump") || >> - g_str_equal(type, "filter-mirror")) { >> + g_str_equal(type, "filter-mirror") || >> + g_str_equal(type, "filter-redirector")) { >> return false; >> } >> > > > . > -- Thanks zhangchen