Re: [Qemu-devel] [RFC PATCH V3 1/4] colo-compare: introduce colo compare initlization

[Jason Wang <jasowang@redhat.com>]

On 04/28/2016 03:53 PM, Zhang Chen wrote:
>
>
> On 04/28/2016 02:53 PM, Jason Wang wrote:
>>
>> On 04/18/2016 07:11 PM, Zhang Chen wrote:
>>> packet come from primary char indev will be send to
>>> outdev - packet come from secondary char dev will be drop
>>>
>>> usage:
>>>
>>> primary:
>>> -netdev
>>> tap,id=hn0,vhost=off,script=/etc/qemu-ifup,downscript=/etc/qemu-ifdown
>>> -device e1000,id=e0,netdev=hn0,mac=52:a4:00:12:78:66
>>> -chardev socket,id=mirror0,host=3.3.3.3,port=9003,server,nowait
>>> -chardev socket,id=compare1,host=3.3.3.3,port=9004,server,nowait
>>> -chardev socket,id=compare0,host=3.3.3.3,port=9001,server,nowait
>>> -chardev socket,id=compare0-0,host=3.3.3.3,port=9001
>>> -chardev socket,id=compare_out,host=3.3.3.3,port=9005,server,nowait
>>> -chardev socket,id=compare_out0,host=3.3.3.3,port=9005
>>> -object filter-mirror,id=m0,netdev=hn0,queue=tx,outdev=mirror0
>>> -object
>>> filter-redirector,netdev=hn0,id=redire0,queue=rx,indev=compare_out
>>> -object
>>> filter-redirector,netdev=hn0,id=redire1,queue=rx,outdev=compare0
>>> -object
>>> colo-compare,id=comp0,primary_in=compare0-0,secondary_in=compare1,outdev=compare_out0
>>>
>>> secondary:
>>> -netdev tap,id=hn0,vhost=off,script=/etc/qemu-ifup,down
>>> script=/etc/qemu-ifdown
>>> -device e1000,netdev=hn0,mac=52:a4:00:12:78:66
>>> -chardev socket,id=red0,host=3.3.3.3,port=9003
>>> -chardev socket,id=red1,host=3.3.3.3,port=9004
>>> -object filter-redirector,id=f1,netdev=hn0,queue=tx,indev=red0
>>> -object filter-redirector,id=f2,netdev=hn0,queue=rx,outdev=red1
>>>
>>> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>>> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>>> ---
>>>   net/Makefile.objs  |   1 +
>>>   net/colo-compare.c | 361
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   qemu-options.hx    |   6 +
>>>   vl.c               |   3 +-
>>>   4 files changed, 370 insertions(+), 1 deletion(-)
>>>   create mode 100644 net/colo-compare.c
>>>
>>> diff --git a/net/Makefile.objs b/net/Makefile.objs
>>> index b7c22fd..ba92f73 100644
>>> --- a/net/Makefile.objs
>>> +++ b/net/Makefile.objs
>>> @@ -16,3 +16,4 @@ common-obj-$(CONFIG_NETMAP) += netmap.o
>>>   common-obj-y += filter.o
>>>   common-obj-y += filter-buffer.o
>>>   common-obj-y += filter-mirror.o
>>> +common-obj-y += colo-compare.o
>>> diff --git a/net/colo-compare.c b/net/colo-compare.c
>>> new file mode 100644
>>> index 0000000..c45b132
>>> --- /dev/null
>>> +++ b/net/colo-compare.c
>>> @@ -0,0 +1,361 @@
>>> +/*
>>> + * Copyright (c) 2016 FUJITSU LIMITED
>>> + * Author: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
>>> + *
>>> + * 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 "qemu/osdep.h"
>>> +#include "qemu-common.h"
>>> +#include "qapi/qmp/qerror.h"
>>> +#include "qemu/error-report.h"
>>> +#include "qapi/error.h"
>>> +#include "net/net.h"
>>> +#include "net/vhost_net.h"
>>> +#include "qom/object_interfaces.h"
>>> +#include "qemu/iov.h"
>>> +#include "qom/object.h"
>>> +#include "qemu/typedefs.h"
>>> +#include "net/queue.h"
>>> +#include "sysemu/char.h"
>>> +#include "qemu/sockets.h"
>>> +#include "qapi-visit.h"
>>> +#include "trace.h"
>>> +
>>> +#define TYPE_COLO_COMPARE "colo-compare"
>>> +#define COLO_COMPARE(obj) \
>>> +    OBJECT_CHECK(CompareState, (obj), TYPE_COLO_COMPARE)
>>> +
>>> +#define COMPARE_READ_LEN_MAX NET_BUFSIZE
>>> +
>>> +static QTAILQ_HEAD(, CompareState) net_compares =
>>> +       QTAILQ_HEAD_INITIALIZER(net_compares);
>> Don't see any usage of this, if it was used in the following patches,
>> better move this to that patch.
>
> Hi~ jason~
>
> Thanks for review~~
>
> will fix it in next version.
>
>>
>>> +
>>> +typedef struct ReadState {
>>> +    int state; /* 0 = getting length, 1 = getting data */
>>> +    uint32_t index;
>>> +    uint32_t packet_len;
>>> +    uint8_t buf[COMPARE_READ_LEN_MAX];
>>> +} ReadState;
>>> +
>>> +typedef struct CompareState {
>>> +    Object parent;
>>> +
>>> +    char *pri_indev;
>>> +    char *sec_indev;
>>> +    char *outdev;
>>> +    CharDriverState *chr_pri_in;
>>> +    CharDriverState *chr_sec_in;
>>> +    CharDriverState *chr_out;
>>> +    QTAILQ_ENTRY(CompareState) next;
>>> +    ReadState pri_rs;
>>> +    ReadState sec_rs;
>> What did 'rs' short for, ReadState? :)
>
> Yes,should I change the 'rs' to another name?

Probably not.

>
>>
>>> +} CompareState;
>>> +
>>> +typedef struct CompareClass {
>>> +    ObjectClass parent_class;
>>> +} CompareClass;
>>> +
>>> +static int compare_chr_send(CharDriverState *out,
>>> +                            const uint8_t *buf,
>>> +                            uint32_t size)
>>> +{
>>> +    int ret = 0;
>>> +    uint32_t len = htonl(size);
>>> +
>>> +    if (!size) {
>>> +        return 0;
>>> +    }
>>> +
>>> +    ret = qemu_chr_fe_write_all(out, (uint8_t *)&len, sizeof(len));
>>> +    if (ret != sizeof(len)) {
>>> +        goto err;
>>> +    }
>>> +
>>> +    ret = qemu_chr_fe_write_all(out, (uint8_t *)buf, size);
>>> +    if (ret != size) {
>>> +        goto err;
>>> +    }
>>> +
>>> +    return 0;
>>> +
>>> +err:
>>> +    return ret < 0 ? ret : -EIO;
>>> +}
>>> +
>>> +static int compare_chr_can_read(void *opaque)
>>> +{
>>> +    return COMPARE_READ_LEN_MAX;
>>> +}
>>> +
>>> +/*
>>> + * called from the main thread on the primary
>>> + * for get packets
>>> + * Returns
>>> + * 0: readstate is not ready
>>> + * 1: readstate is ready
>>> + * otherwise error occurs
>>> + */
>>> +static int compare_chr_fill_rstate(ReadState *rs, const uint8_t
>>> *buf, int size)
>>> +{
>>> +    unsigned int l;
>>> +    while (size > 0) {
>>> +        /* reassemble a packet from the network */
>>> +        switch (rs->state) { /* 0 = getting length, 1 = getting
>>> data */
>>> +        case 0:
>>> +            l = 4 - rs->index;
>>> +            if (l > size) {
>>> +                l = size;
>>> +            }
>>> +            memcpy(rs->buf + rs->index, buf, l);
>>> +            buf += l;
>>> +            size -= l;
>>> +            rs->index += l;
>>> +            if (rs->index == 4) {
>>> +                /* got length */
>>> +                rs->packet_len = ntohl(*(uint32_t *)rs->buf);
>>> +                rs->index = 0;
>>> +                rs->state = 1;
>>> +            }
>>> +            break;
>>> +        case 1:
>>> +            l = rs->packet_len - rs->index;
>>> +            if (l > size) {
>>> +                l = size;
>>> +            }
>>> +            if (rs->index + l <= sizeof(rs->buf)) {
>>> +                memcpy(rs->buf + rs->index, buf, l);
>>> +            } else {
>>> +                error_report("colo-compare: "
>>> +                             "Received oversized packet over socket");
>>> +                rs->index = rs->state = 0;
>>> +                return -1;
>>> +            }
>>> +
>>> +            rs->index += l;
>>> +            buf += l;
>>> +            size -= l;
>>> +            if (rs->index >= rs->packet_len) {
>>> +                rs->index = 0;
>>> +                rs->state = 0;
>>> +                return 1;
>>> +            }
>>> +            break;
>>> +        }
>>> +    }
>>> +    return 0;
>>> +}
>> This looks rather similar to redirector_chr_read(), any chance to unify
>> the code?
>
> Yes,I think we can create 'colo-base.c' and 'colo-base.h'
> in net/ to share codes for colo-compare,filter-redirector
> and filter-rewriter. how about it?

You can, but need a generic name since it would be used by redirector
too. Looking at net/socket.c, I wonder whether or not we could reuse
NetSocketState directly.

>
>
>>
>>> +
>>> +/*
>>> + * called from the main thread on the primary for packets
>>> + * arriving over the socket from the primary.
>>> + */
>>> +static void compare_pri_chr_in(void *opaque, const uint8_t *buf,
>>> int size)
>>> +{
>>> +    CompareState *s = COLO_COMPARE(opaque);
>>> +    int ret;
>>> +
>>> +    ret = compare_chr_fill_rstate(&s->pri_rs, buf, size);
>>> +    if (ret == 1) {
>>> +        /* FIXME: enqueue to primary packet list */
>> Do you mean to use some internal data structure instead of chardev? If
>> yes, probably a "TODO", not "FIXME".
>
> Yes,will change to "TODO"
>
>>> +        compare_chr_send(s->chr_out, s->pri_rs.buf,
>>> s->pri_rs.packet_len);
>>> +    } else if (ret == -1) {
>>> +        qemu_chr_add_handlers(s->chr_pri_in, NULL, NULL, NULL, NULL);
>> Should we add a warning here?
>
> OK~ I will add error_report()
>
>>
>>> +    }
>>> +}
>>> +
>>> +/*
>>> + * called from the main thread on the primary for packets
>>> + * arriving over the socket from the secondary.
>>> + */
>>> +static void compare_sec_chr_in(void *opaque, const uint8_t *buf,
>>> int size)
>>> +{
>>> +    CompareState *s = COLO_COMPARE(opaque);
>>> +    int ret;
>>> +
>>> +    ret = compare_chr_fill_rstate(&s->sec_rs, buf, size);
>>> +    if (ret == 1) {
>>> +        /* TODO: enqueue to secondary packet list*/
>>> +        /* should we send sec arp pkt? */
>> What's the problem here?
>
> This comments will be move to patch 2/4
>
>
> The reason is I send secondary guest's arp and ipv6
> packet to ensure it can get mac addr for send other IP
> packet for compare.but currently I don't know this job
> may be affected to something?

Still not clear, I thought we don't need to do any trick for arp to work?

>
>>
>>> +        compare_chr_send(s->chr_out, s->sec_rs.buf,
>>> s->sec_rs.packet_len);
>>> +    } else if (ret == -1) {
>>> +        qemu_chr_add_handlers(s->chr_sec_in, NULL, NULL, NULL, NULL);
>>> +    }
>> This function is similar to primary version, may need to unify the
>> codes.
>
> Thanks
> zhangchen

[...]