Re: [Qemu-devel] [PATCH V4 1/2] net/filter-mirror: implement filter-redirector

[Jason Wang <jasowang@redhat.com>]

On 03/16/2016 05:34 PM, Wen Congyang wrote:
> On 03/16/2016 04:18 PM, Jason Wang wrote:
>> > 
>> > 
>> > On 03/15/2016 06:03 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 <zhangchen.fnst@cn.fujitsu.com>
>>> >> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>> >> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>>> >> ---
>>> >>  net/filter-mirror.c | 236 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>> >>  qemu-options.hx     |   9 ++
>>> >>  vl.c                |   3 +-
>>> >>  3 files changed, 247 insertions(+), 1 deletion(-)
>>> >>
>>> >> diff --git a/net/filter-mirror.c b/net/filter-mirror.c
>>> >> index 1b1ec16..77ece41 100644
>>> >> --- a/net/filter-mirror.c
>>> >> +++ b/net/filter-mirror.c
>>> >> @@ -26,12 +26,23 @@
>>> >>  #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 REDIRECTOR_MAX_LEN NET_BUFSIZE
>>> >>  
>>> >>  typedef struct MirrorState {
>>> >>      NetFilterState parent_obj;
>>> >> +    char *indev;
>>> >>      char *outdev;
>>> >> +    CharDriverState *chr_in;
>>> >>      CharDriverState *chr_out;
>>> >> +    int state; /* 0 = getting length, 1 = getting data */
>>> >> +    unsigned int index;
>>> >> +    unsigned int packet_len;
>>> >> +    uint8_t buf[REDIRECTOR_MAX_LEN];
>>> >>  } MirrorState;
>>> >>  
>>> >>  static int filter_mirror_send(CharDriverState *chr_out,
>>> >> @@ -68,6 +79,89 @@ err:
>>> >>      return ret < 0 ? ret : -EIO;
>>> >>  }
>>> >>  
>>> >> +static void
>>> >> +redirector_to_filter(NetFilterState *nf, const uint8_t *buf, int len)
>>> >> +{
>>> >> +    struct iovec iov = {
>>> >> +        .iov_base = (void *)buf,
>>> >> +        .iov_len = len,
>>> >> +    };
>>> >> +
>>> >> +    if (nf->direction == NET_FILTER_DIRECTION_ALL ||
>>> >> +        nf->direction == NET_FILTER_DIRECTION_TX) {
>>> >> +        qemu_netfilter_pass_to_next(nf->netdev, 0, &iov, 1, nf);
>>> >> +    }
>>> >> +
>>> >> +    if (nf->direction == NET_FILTER_DIRECTION_ALL ||
>>> >> +        nf->direction == NET_FILTER_DIRECTION_RX) {
>>> >> +        qemu_netfilter_pass_to_next(nf->netdev->peer, 0, &iov, 1, nf);
>>> >> +     }
>>> >> +}
>>> >> +
>>> >> +static int redirector_chr_can_read(void *opaque)
>>> >> +{
>>> >> +    return REDIRECTOR_MAX_LEN;
>>> >> +}
>>> >> +
>>> >> +static void redirector_chr_read(void *opaque, const uint8_t *buf, int size)
>>> >> +{
>>> >> +    NetFilterState *nf = opaque;
>>> >> +    MirrorState *s = FILTER_REDIRECTOR(nf);
>>> >> +    unsigned int l;
>>> >> +
>>> >> +    if (size == 0) {
>>> >> +        /* the peer is closed ? */
>>> >> +        return ;
>>> >> +    }
>> > 
>> > Looks like if you want to handle connection close, you need use event
>> > handler when calling qemu_chr_add_handlers().
> In which case, we will see size is 0 if we don't have a event handler?

Looking at tcp_chr_read(), it seems that we won't see zero size.

>
> For redirector filter, I think we don't care about if the char device
> is disconnected. If the char device is ready again, we will continue
> to read from the char device.
>
> So I think we just add more comments here.
>
>> > 
>>> >> +
>>> >> +    /* most of code is stolen from net_socket_send */
>> > 
>> > This comment seems redundant.
>> > 
>>> >> +    while (size > 0) {
>>> >> +        /* reassemble a packet from the network */
>>> >> +        switch (s->state) {
>>> >> +        case 0:
>>> >> +            l = 4 - s->index;
>>> >> +            if (l > size) {
>>> >> +                l = size;
>>> >> +            }
>>> >> +            memcpy(s->buf + s->index, buf, l);
>>> >> +            buf += l;
>>> >> +            size -= l;
>>> >> +            s->index += l;
>>> >> +            if (s->index == 4) {
>>> >> +                /* got length */
>>> >> +                s->packet_len = ntohl(*(uint32_t *)s->buf);
>>> >> +                s->index = 0;
>>> >> +                s->state = 1;
>>> >> +            }
>>> >> +            break;
>>> >> +        case 1:
>>> >> +            l = s->packet_len - s->index;
>>> >> +            if (l > size) {
>>> >> +                l = size;
>>> >> +            }
>>> >> +            if (s->index + l <= sizeof(s->buf)) {
>>> >> +                memcpy(s->buf + s->index, buf, l);
>>> >> +            } else {
>>> >> +                fprintf(stderr, "serious error: oversized packet received,"
>>> >> +                    "connection terminated.\n");
>> > 
>> > error_report() looks better.
>> > 
>>> >> +                s->state = 0;
>>> >> +                /* FIXME: do something ? */
>> > 
>> > This needs some thought, but at least reset the fd handler and state is
>> > needed.
> Maybe we can read the data from the chardev, and drop it?
>
> Thanks
> Wen Congyang
>

No function difference, but reading and dropping will cost extra cpu
which seems not good.