[RFC,V3,1/4] colo-compare: introduce colo compare initlization

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

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.

> +
> +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? :)

> +} 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?

> +
> +/*
> + * 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".

> +        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?

> +    }
> +}
> +
> +/*
> + * 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?

> +        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.

> +}
> +
> +static char *compare_get_pri_indev(Object *obj, Error **errp)
> +{
> +    CompareState *s = COLO_COMPARE(obj);
> +
> +    return g_strdup(s->pri_indev);
> +}
> +
> +static void compare_set_pri_indev(Object *obj, const char *value, Error **errp)
> +{
> +    CompareState *s = COLO_COMPARE(obj);
> +
> +    g_free(s->pri_indev);
> +    s->pri_indev = g_strdup(value);
> +}
> +
> +static char *compare_get_sec_indev(Object *obj, Error **errp)
> +{
> +    CompareState *s = COLO_COMPARE(obj);
> +
> +    return g_strdup(s->sec_indev);
> +}
> +
> +static void compare_set_sec_indev(Object *obj, const char *value, Error **errp)
> +{
> +    CompareState *s = COLO_COMPARE(obj);
> +
> +    g_free(s->sec_indev);
> +    s->sec_indev = g_strdup(value);
> +}
> +
> +static char *compare_get_outdev(Object *obj, Error **errp)
> +{
> +    CompareState *s = COLO_COMPARE(obj);
> +
> +    return g_strdup(s->outdev);
> +}
> +
> +static void compare_set_outdev(Object *obj, const char *value, Error **errp)
> +{
> +    CompareState *s = COLO_COMPARE(obj);
> +
> +    g_free(s->outdev);
> +    s->outdev = g_strdup(value);
> +}
> +
> +/*
> + * called from the main thread on the primary
> + * to setup colo-compare.
> + */
> +static void colo_compare_complete(UserCreatable *uc, Error **errp)
> +{
> +    CompareState *s = COLO_COMPARE(uc);
> +
> +    if (!s->pri_indev || !s->sec_indev || !s->outdev) {
> +        error_setg(errp, "colo compare needs 'primary_in' ,"
> +                   "'secondary_in','outdev' property set");
> +        return;
> +    } else if (!strcmp(s->pri_indev, s->outdev) ||
> +               !strcmp(s->sec_indev, s->outdev) ||
> +               !strcmp(s->pri_indev, s->sec_indev)) {
> +        error_setg(errp, "'indev' and 'outdev' could not be same "
> +                   "for compare module");
> +        return;
> +    }
> +
> +    s->chr_pri_in = qemu_chr_find(s->pri_indev);
> +    if (s->chr_pri_in == NULL) {
> +        error_setg(errp, "Primary IN Device '%s' not found",
> +                   s->pri_indev);
> +        return;
> +    }
> +
> +    s->chr_sec_in = qemu_chr_find(s->sec_indev);
> +    if (s->chr_sec_in == NULL) {
> +        error_setg(errp, "Secondary IN Device '%s' not found",
> +                   s->sec_indev);
> +        return;
> +    }
> +
> +    s->chr_out = qemu_chr_find(s->outdev);
> +    if (s->chr_out == NULL) {
> +        error_setg(errp, "OUT Device '%s' not found", s->outdev);
> +        return;
> +    }
> +
> +    qemu_chr_fe_claim_no_fail(s->chr_pri_in);
> +    qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read,
> +                          compare_pri_chr_in, NULL, s);
> +
> +    qemu_chr_fe_claim_no_fail(s->chr_sec_in);
> +    qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read,
> +                          compare_sec_chr_in, NULL, s);
> +
> +    qemu_chr_fe_claim_no_fail(s->chr_out);
> +    QTAILQ_INSERT_TAIL(&net_compares, s, next);
> +
> +    return;
> +}
> +
> +static void colo_compare_class_init(ObjectClass *oc, void *data)
> +{
> +    UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
> +
> +    ucc->complete = colo_compare_complete;
> +}
> +
> +/*
> + * called from the main thread on the primary
> + * to cleanup colo-compare.
> + */
> +static void colo_compare_class_finalize(ObjectClass *oc, void *data)
> +{
> +    UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
> +    CompareState *s = COLO_COMPARE(ucc);
> +
> +    if (s->chr_pri_in) {
> +        qemu_chr_add_handlers(s->chr_pri_in, NULL, NULL, NULL, NULL);
> +        qemu_chr_fe_release(s->chr_pri_in);
> +    }
> +    if (s->chr_sec_in) {
> +        qemu_chr_add_handlers(s->chr_sec_in, NULL, NULL, NULL, NULL);
> +        qemu_chr_fe_release(s->chr_sec_in);
> +    }
> +    if (s->chr_out) {
> +        qemu_chr_fe_release(s->chr_out);
> +    }
> +
> +    if (!QTAILQ_EMPTY(&net_compares)) {
> +        QTAILQ_REMOVE(&net_compares, s, next);
> +    }
> +}
> +
> +static void colo_compare_init(Object *obj)
> +{
> +    object_property_add_str(obj, "primary_in",
> +                            compare_get_pri_indev, compare_set_pri_indev,
> +                            NULL);
> +    object_property_add_str(obj, "secondary_in",
> +                            compare_get_sec_indev, compare_set_sec_indev,
> +                            NULL);
> +    object_property_add_str(obj, "outdev",
> +                            compare_get_outdev, compare_set_outdev,
> +                            NULL);
> +}
> +
> +static void colo_compare_finalize(Object *obj)
> +{
> +    CompareState *s = COLO_COMPARE(obj);
> +
> +    g_free(s->pri_indev);
> +    g_free(s->sec_indev);
> +    g_free(s->outdev);
> +}
> +
> +static const TypeInfo colo_compare_info = {
> +    .name = TYPE_COLO_COMPARE,
> +    .parent = TYPE_OBJECT,
> +    .instance_size = sizeof(CompareState),
> +    .instance_init = colo_compare_init,
> +    .instance_finalize = colo_compare_finalize,
> +    .class_size = sizeof(CompareClass),
> +    .class_init = colo_compare_class_init,
> +    .class_finalize = colo_compare_class_finalize,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_USER_CREATABLE },
> +        { }
> +    }
> +};
> +
> +static void register_types(void)
> +{
> +    type_register_static(&colo_compare_info);
> +}
> +
> +type_init(register_types);
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 587de8f..38f58f7 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3866,6 +3866,12 @@ Dump the network traffic on netdev @var{dev} to the file specified by
>  The file format is libpcap, so it can be analyzed with tools such as tcpdump
>  or Wireshark.
>  
> +@item -object colo-compare,id=@var{id},primary_in=@var{chardevid},secondary_in=@var{chardevid},
> +outdev=@var{chardevid}
> +
> +Colo-compare get packet from primary_in@var{chardevid} and secondary_in@var{chardevid},
> +and output to outdev@var{chardevid}, we can use it with the help of filter-mirror and filter-redirector.
> +
>  @item -object secret,id=@var{id},data=@var{string},format=@var{raw|base64}[,keyid=@var{secretid},iv=@var{string}]
>  @item -object secret,id=@var{id},file=@var{filename},format=@var{raw|base64}[,keyid=@var{secretid},iv=@var{string}]
>  
> diff --git a/vl.c b/vl.c
> index cbe51ac..c6b9a6f 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2865,7 +2865,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-redirector")) {
> +        g_str_equal(type, "filter-redirector") ||
> +        g_str_equal(type, "colo-compare")) {
>          return false;
>      }
>

On 04/28/2016 02:53 PM, Jason Wang wrote:
> +static void compare_set_outdev(Object *obj, const char *value, Error **errp)
> > +{
> > +    CompareState *s = COLO_COMPARE(obj);
> > +
> > +    g_free(s->outdev);
> > +    s->outdev = g_strdup(value);
> > +}
> > +
> > +/*
> > + * called from the main thread on the primary
> > + * to setup colo-compare.
> > + */
> > +static void colo_compare_complete(UserCreatable *uc, Error **errp)
> > +{
> > +    CompareState *s = COLO_COMPARE(uc);
> > +
> > +    if (!s->pri_indev || !s->sec_indev || !s->outdev) {
> > +        error_setg(errp, "colo compare needs 'primary_in' ,"
> > +                   "'secondary_in','outdev' property set");
> > +        return;
> > +    } else if (!strcmp(s->pri_indev, s->outdev) ||
> > +               !strcmp(s->sec_indev, s->outdev) ||
> > +               !strcmp(s->pri_indev, s->sec_indev)) {
> > +        error_setg(errp, "'indev' and 'outdev' could not be same "
> > +                   "for compare module");
> > +        return;
> > +    }
> > +
> > +    s->chr_pri_in = qemu_chr_find(s->pri_indev);
> > +    if (s->chr_pri_in == NULL) {
> > +        error_setg(errp, "Primary IN Device '%s' not found",
> > +                   s->pri_indev);
> > +        return;
> > +    }
> > +
> > +    s->chr_sec_in = qemu_chr_find(s->sec_indev);
> > +    if (s->chr_sec_in == NULL) {
> > +        error_setg(errp, "Secondary IN Device '%s' not found",
> > +                   s->sec_indev);
> > +        return;
> > +    }
> > +
> > +    s->chr_out = qemu_chr_find(s->outdev);
> > +    if (s->chr_out == NULL) {
> > +        error_setg(errp, "OUT Device '%s' not found", s->outdev);
> > +        return;
> > +    }
> > +
> > +    qemu_chr_fe_claim_no_fail(s->chr_pri_in);
> > +    qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read,
> > +                          compare_pri_chr_in, NULL, s);
> > +
> > +    qemu_chr_fe_claim_no_fail(s->chr_sec_in);
> > +    qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read,
> > +                          compare_sec_chr_in, NULL, s);
> > +

Btw, what's the reason of handling this in main loop? I thought it would
be better to do this in colo thread? Otherwise, you need lots of extra
synchronizations?

> > +    qemu_chr_fe_claim_no_fail(s->chr_out);
> > +    QTAILQ_INSERT_TAIL(&net_compares, s, next);
> > +
> > +    return;
> > +}

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?

>
>> +} 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?

>
>> +
>> +/*
>> + * 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?

>
>> +        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

>> +}
>> +
>> +static char *compare_get_pri_indev(Object *obj, Error **errp)
>> +{
>> +    CompareState *s = COLO_COMPARE(obj);
>> +
>> +    return g_strdup(s->pri_indev);
>> +}
>> +
>> +static void compare_set_pri_indev(Object *obj, const char *value, Error **errp)
>> +{
>> +    CompareState *s = COLO_COMPARE(obj);
>> +
>> +    g_free(s->pri_indev);
>> +    s->pri_indev = g_strdup(value);
>> +}
>> +
>> +static char *compare_get_sec_indev(Object *obj, Error **errp)
>> +{
>> +    CompareState *s = COLO_COMPARE(obj);
>> +
>> +    return g_strdup(s->sec_indev);
>> +}
>> +
>> +static void compare_set_sec_indev(Object *obj, const char *value, Error **errp)
>> +{
>> +    CompareState *s = COLO_COMPARE(obj);
>> +
>> +    g_free(s->sec_indev);
>> +    s->sec_indev = g_strdup(value);
>> +}
>> +
>> +static char *compare_get_outdev(Object *obj, Error **errp)
>> +{
>> +    CompareState *s = COLO_COMPARE(obj);
>> +
>> +    return g_strdup(s->outdev);
>> +}
>> +
>> +static void compare_set_outdev(Object *obj, const char *value, Error **errp)
>> +{
>> +    CompareState *s = COLO_COMPARE(obj);
>> +
>> +    g_free(s->outdev);
>> +    s->outdev = g_strdup(value);
>> +}
>> +
>> +/*
>> + * called from the main thread on the primary
>> + * to setup colo-compare.
>> + */
>> +static void colo_compare_complete(UserCreatable *uc, Error **errp)
>> +{
>> +    CompareState *s = COLO_COMPARE(uc);
>> +
>> +    if (!s->pri_indev || !s->sec_indev || !s->outdev) {
>> +        error_setg(errp, "colo compare needs 'primary_in' ,"
>> +                   "'secondary_in','outdev' property set");
>> +        return;
>> +    } else if (!strcmp(s->pri_indev, s->outdev) ||
>> +               !strcmp(s->sec_indev, s->outdev) ||
>> +               !strcmp(s->pri_indev, s->sec_indev)) {
>> +        error_setg(errp, "'indev' and 'outdev' could not be same "
>> +                   "for compare module");
>> +        return;
>> +    }
>> +
>> +    s->chr_pri_in = qemu_chr_find(s->pri_indev);
>> +    if (s->chr_pri_in == NULL) {
>> +        error_setg(errp, "Primary IN Device '%s' not found",
>> +                   s->pri_indev);
>> +        return;
>> +    }
>> +
>> +    s->chr_sec_in = qemu_chr_find(s->sec_indev);
>> +    if (s->chr_sec_in == NULL) {
>> +        error_setg(errp, "Secondary IN Device '%s' not found",
>> +                   s->sec_indev);
>> +        return;
>> +    }
>> +
>> +    s->chr_out = qemu_chr_find(s->outdev);
>> +    if (s->chr_out == NULL) {
>> +        error_setg(errp, "OUT Device '%s' not found", s->outdev);
>> +        return;
>> +    }
>> +
>> +    qemu_chr_fe_claim_no_fail(s->chr_pri_in);
>> +    qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read,
>> +                          compare_pri_chr_in, NULL, s);
>> +
>> +    qemu_chr_fe_claim_no_fail(s->chr_sec_in);
>> +    qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read,
>> +                          compare_sec_chr_in, NULL, s);
>> +
>> +    qemu_chr_fe_claim_no_fail(s->chr_out);
>> +    QTAILQ_INSERT_TAIL(&net_compares, s, next);
>> +
>> +    return;
>> +}
>> +
>> +static void colo_compare_class_init(ObjectClass *oc, void *data)
>> +{
>> +    UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
>> +
>> +    ucc->complete = colo_compare_complete;
>> +}
>> +
>> +/*
>> + * called from the main thread on the primary
>> + * to cleanup colo-compare.
>> + */
>> +static void colo_compare_class_finalize(ObjectClass *oc, void *data)
>> +{
>> +    UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
>> +    CompareState *s = COLO_COMPARE(ucc);
>> +
>> +    if (s->chr_pri_in) {
>> +        qemu_chr_add_handlers(s->chr_pri_in, NULL, NULL, NULL, NULL);
>> +        qemu_chr_fe_release(s->chr_pri_in);
>> +    }
>> +    if (s->chr_sec_in) {
>> +        qemu_chr_add_handlers(s->chr_sec_in, NULL, NULL, NULL, NULL);
>> +        qemu_chr_fe_release(s->chr_sec_in);
>> +    }
>> +    if (s->chr_out) {
>> +        qemu_chr_fe_release(s->chr_out);
>> +    }
>> +
>> +    if (!QTAILQ_EMPTY(&net_compares)) {
>> +        QTAILQ_REMOVE(&net_compares, s, next);
>> +    }
>> +}
>> +
>> +static void colo_compare_init(Object *obj)
>> +{
>> +    object_property_add_str(obj, "primary_in",
>> +                            compare_get_pri_indev, compare_set_pri_indev,
>> +                            NULL);
>> +    object_property_add_str(obj, "secondary_in",
>> +                            compare_get_sec_indev, compare_set_sec_indev,
>> +                            NULL);
>> +    object_property_add_str(obj, "outdev",
>> +                            compare_get_outdev, compare_set_outdev,
>> +                            NULL);
>> +}
>> +
>> +static void colo_compare_finalize(Object *obj)
>> +{
>> +    CompareState *s = COLO_COMPARE(obj);
>> +
>> +    g_free(s->pri_indev);
>> +    g_free(s->sec_indev);
>> +    g_free(s->outdev);
>> +}
>> +
>> +static const TypeInfo colo_compare_info = {
>> +    .name = TYPE_COLO_COMPARE,
>> +    .parent = TYPE_OBJECT,
>> +    .instance_size = sizeof(CompareState),
>> +    .instance_init = colo_compare_init,
>> +    .instance_finalize = colo_compare_finalize,
>> +    .class_size = sizeof(CompareClass),
>> +    .class_init = colo_compare_class_init,
>> +    .class_finalize = colo_compare_class_finalize,
>> +    .interfaces = (InterfaceInfo[]) {
>> +        { TYPE_USER_CREATABLE },
>> +        { }
>> +    }
>> +};
>> +
>> +static void register_types(void)
>> +{
>> +    type_register_static(&colo_compare_info);
>> +}
>> +
>> +type_init(register_types);
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 587de8f..38f58f7 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -3866,6 +3866,12 @@ Dump the network traffic on netdev @var{dev} to the file specified by
>>   The file format is libpcap, so it can be analyzed with tools such as tcpdump
>>   or Wireshark.
>>   
>> +@item -object colo-compare,id=@var{id},primary_in=@var{chardevid},secondary_in=@var{chardevid},
>> +outdev=@var{chardevid}
>> +
>> +Colo-compare get packet from primary_in@var{chardevid} and secondary_in@var{chardevid},
>> +and output to outdev@var{chardevid}, we can use it with the help of filter-mirror and filter-redirector.
>> +
>>   @item -object secret,id=@var{id},data=@var{string},format=@var{raw|base64}[,keyid=@var{secretid},iv=@var{string}]
>>   @item -object secret,id=@var{id},file=@var{filename},format=@var{raw|base64}[,keyid=@var{secretid},iv=@var{string}]
>>   
>> diff --git a/vl.c b/vl.c
>> index cbe51ac..c6b9a6f 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -2865,7 +2865,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-redirector")) {
>> +        g_str_equal(type, "filter-redirector") ||
>> +        g_str_equal(type, "colo-compare")) {
>>           return false;
>>       }
>>   
>
>
> .
>

On 04/28/2016 03:16 PM, Jason Wang wrote:
>
> On 04/28/2016 02:53 PM, Jason Wang wrote:
>> +static void compare_set_outdev(Object *obj, const char *value, Error **errp)
>>> +{
>>> +    CompareState *s = COLO_COMPARE(obj);
>>> +
>>> +    g_free(s->outdev);
>>> +    s->outdev = g_strdup(value);
>>> +}
>>> +
>>> +/*
>>> + * called from the main thread on the primary
>>> + * to setup colo-compare.
>>> + */
>>> +static void colo_compare_complete(UserCreatable *uc, Error **errp)
>>> +{
>>> +    CompareState *s = COLO_COMPARE(uc);
>>> +
>>> +    if (!s->pri_indev || !s->sec_indev || !s->outdev) {
>>> +        error_setg(errp, "colo compare needs 'primary_in' ,"
>>> +                   "'secondary_in','outdev' property set");
>>> +        return;
>>> +    } else if (!strcmp(s->pri_indev, s->outdev) ||
>>> +               !strcmp(s->sec_indev, s->outdev) ||
>>> +               !strcmp(s->pri_indev, s->sec_indev)) {
>>> +        error_setg(errp, "'indev' and 'outdev' could not be same "
>>> +                   "for compare module");
>>> +        return;
>>> +    }
>>> +
>>> +    s->chr_pri_in = qemu_chr_find(s->pri_indev);
>>> +    if (s->chr_pri_in == NULL) {
>>> +        error_setg(errp, "Primary IN Device '%s' not found",
>>> +                   s->pri_indev);
>>> +        return;
>>> +    }
>>> +
>>> +    s->chr_sec_in = qemu_chr_find(s->sec_indev);
>>> +    if (s->chr_sec_in == NULL) {
>>> +        error_setg(errp, "Secondary IN Device '%s' not found",
>>> +                   s->sec_indev);
>>> +        return;
>>> +    }
>>> +
>>> +    s->chr_out = qemu_chr_find(s->outdev);
>>> +    if (s->chr_out == NULL) {
>>> +        error_setg(errp, "OUT Device '%s' not found", s->outdev);
>>> +        return;
>>> +    }
>>> +
>>> +    qemu_chr_fe_claim_no_fail(s->chr_pri_in);
>>> +    qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read,
>>> +                          compare_pri_chr_in, NULL, s);
>>> +
>>> +    qemu_chr_fe_claim_no_fail(s->chr_sec_in);
>>> +    qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read,
>>> +                          compare_sec_chr_in, NULL, s);
>>> +
> Btw, what's the reason of handling this in main loop? I thought it would
> be better to do this in colo thread? Otherwise, you need lots of extra
> synchronizations?

Do you mean we should start/stop/do checkpoint it by colo-frame?

>
>>> +    qemu_chr_fe_claim_no_fail(s->chr_out);
>>> +    QTAILQ_INSERT_TAIL(&net_compares, s, next);
>>> +
>>> +    return;
>>> +}
>
>
> .
>

On 04/28/2016 03:55 PM, Zhang Chen wrote:
>
>
> On 04/28/2016 03:16 PM, Jason Wang wrote:
>>
>> On 04/28/2016 02:53 PM, Jason Wang wrote:
>>> +static void compare_set_outdev(Object *obj, const char *value,
>>> Error **errp)
>>>> +{
>>>> +    CompareState *s = COLO_COMPARE(obj);
>>>> +
>>>> +    g_free(s->outdev);
>>>> +    s->outdev = g_strdup(value);
>>>> +}
>>>> +
>>>> +/*
>>>> + * called from the main thread on the primary
>>>> + * to setup colo-compare.
>>>> + */
>>>> +static void colo_compare_complete(UserCreatable *uc, Error **errp)
>>>> +{
>>>> +    CompareState *s = COLO_COMPARE(uc);
>>>> +
>>>> +    if (!s->pri_indev || !s->sec_indev || !s->outdev) {
>>>> +        error_setg(errp, "colo compare needs 'primary_in' ,"
>>>> +                   "'secondary_in','outdev' property set");
>>>> +        return;
>>>> +    } else if (!strcmp(s->pri_indev, s->outdev) ||
>>>> +               !strcmp(s->sec_indev, s->outdev) ||
>>>> +               !strcmp(s->pri_indev, s->sec_indev)) {
>>>> +        error_setg(errp, "'indev' and 'outdev' could not be same "
>>>> +                   "for compare module");
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    s->chr_pri_in = qemu_chr_find(s->pri_indev);
>>>> +    if (s->chr_pri_in == NULL) {
>>>> +        error_setg(errp, "Primary IN Device '%s' not found",
>>>> +                   s->pri_indev);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    s->chr_sec_in = qemu_chr_find(s->sec_indev);
>>>> +    if (s->chr_sec_in == NULL) {
>>>> +        error_setg(errp, "Secondary IN Device '%s' not found",
>>>> +                   s->sec_indev);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    s->chr_out = qemu_chr_find(s->outdev);
>>>> +    if (s->chr_out == NULL) {
>>>> +        error_setg(errp, "OUT Device '%s' not found", s->outdev);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    qemu_chr_fe_claim_no_fail(s->chr_pri_in);
>>>> +    qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read,
>>>> +                          compare_pri_chr_in, NULL, s);
>>>> +
>>>> +    qemu_chr_fe_claim_no_fail(s->chr_sec_in);
>>>> +    qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read,
>>>> +                          compare_sec_chr_in, NULL, s);
>>>> +
>> Btw, what's the reason of handling this in main loop? I thought it would
>> be better to do this in colo thread? Otherwise, you need lots of extra
>> synchronizations?
>
> Do you mean we should start/stop/do checkpoint it by colo-frame?

I mean we probably want to handle pri_in and sec_in in colo compare
thread. Through this way, there's no need for extra synchronization with
main loop.

>
>>
>>>> +    qemu_chr_fe_claim_no_fail(s->chr_out);
>>>> +    QTAILQ_INSERT_TAIL(&net_compares, s, next);
>>>> +
>>>> +    return;
>>>> +}
>>
>>
>> .
>>
>

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

[...]

On 04/28/2016 04:17 PM, Jason Wang wrote:
>
> On 04/28/2016 03:55 PM, Zhang Chen wrote:
>>
>> On 04/28/2016 03:16 PM, Jason Wang wrote:
>>> On 04/28/2016 02:53 PM, Jason Wang wrote:
>>>> +static void compare_set_outdev(Object *obj, const char *value,
>>>> Error **errp)
>>>>> +{
>>>>> +    CompareState *s = COLO_COMPARE(obj);
>>>>> +
>>>>> +    g_free(s->outdev);
>>>>> +    s->outdev = g_strdup(value);
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * called from the main thread on the primary
>>>>> + * to setup colo-compare.
>>>>> + */
>>>>> +static void colo_compare_complete(UserCreatable *uc, Error **errp)
>>>>> +{
>>>>> +    CompareState *s = COLO_COMPARE(uc);
>>>>> +
>>>>> +    if (!s->pri_indev || !s->sec_indev || !s->outdev) {
>>>>> +        error_setg(errp, "colo compare needs 'primary_in' ,"
>>>>> +                   "'secondary_in','outdev' property set");
>>>>> +        return;
>>>>> +    } else if (!strcmp(s->pri_indev, s->outdev) ||
>>>>> +               !strcmp(s->sec_indev, s->outdev) ||
>>>>> +               !strcmp(s->pri_indev, s->sec_indev)) {
>>>>> +        error_setg(errp, "'indev' and 'outdev' could not be same "
>>>>> +                   "for compare module");
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    s->chr_pri_in = qemu_chr_find(s->pri_indev);
>>>>> +    if (s->chr_pri_in == NULL) {
>>>>> +        error_setg(errp, "Primary IN Device '%s' not found",
>>>>> +                   s->pri_indev);
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    s->chr_sec_in = qemu_chr_find(s->sec_indev);
>>>>> +    if (s->chr_sec_in == NULL) {
>>>>> +        error_setg(errp, "Secondary IN Device '%s' not found",
>>>>> +                   s->sec_indev);
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    s->chr_out = qemu_chr_find(s->outdev);
>>>>> +    if (s->chr_out == NULL) {
>>>>> +        error_setg(errp, "OUT Device '%s' not found", s->outdev);
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    qemu_chr_fe_claim_no_fail(s->chr_pri_in);
>>>>> +    qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read,
>>>>> +                          compare_pri_chr_in, NULL, s);
>>>>> +
>>>>> +    qemu_chr_fe_claim_no_fail(s->chr_sec_in);
>>>>> +    qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read,
>>>>> +                          compare_sec_chr_in, NULL, s);
>>>>> +
>>> Btw, what's the reason of handling this in main loop? I thought it would
>>> be better to do this in colo thread? Otherwise, you need lots of extra
>>> synchronizations?
>> Do you mean we should start/stop/do checkpoint it by colo-frame?
> I mean we probably want to handle pri_in and sec_in in colo compare
> thread. Through this way, there's no need for extra synchronization with
> main loop.

I get your point, but how to do this.
Now, we use qemu_chr_add_handlers to do this job.


Thanks
zhangchen

>
>>>>> +    qemu_chr_fe_claim_no_fail(s->chr_out);
>>>>> +    QTAILQ_INSERT_TAIL(&net_compares, s, next);
>>>>> +
>>>>> +    return;
>>>>> +}
>>>
>>> .
>>>
>
>
> .
>

On 04/18/2016 05:11 AM, Zhang Chen wrote:

s/initlization/initialization/ in the subject

> packet come from primary char indev will be send to
> outdev - packet come from secondary char dev will be drop

Grammar suggestion:

Packets coming from the primary char indev will be sent to outdev;
packets coming from the secondary char dev will be dropped


> 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>
> ---

> +/*
> + * called from the main thread on the primary
> + * for get packets

s/get/getting/

> +++ b/qemu-options.hx
> @@ -3866,6 +3866,12 @@ Dump the network traffic on netdev @var{dev} to the file specified by
>  The file format is libpcap, so it can be analyzed with tools such as tcpdump
>  or Wireshark.
>  
> +@item -object colo-compare,id=@var{id},primary_in=@var{chardevid},secondary_in=@var{chardevid},
> +outdev=@var{chardevid}
> +
> +Colo-compare get packet from primary_in@var{chardevid} and secondary_in@var{chardevid},

s/get/gets/

> +and output to outdev@var{chardevid}, we can use it with the help of filter-mirror and filter-redirector.

s/output/outputs/

Worth any better hints here about _how_ to use it with filter-mirror and
filter-redirector, or should the last phrase just be dropped?

On 04/29/2016 04:55 AM, Eric Blake wrote:
> On 04/18/2016 05:11 AM, Zhang Chen wrote:
>
> s/initlization/initialization/ in the subject

OK~

>> packet come from primary char indev will be send to
>> outdev - packet come from secondary char dev will be drop
> Grammar suggestion:
>
> Packets coming from the primary char indev will be sent to outdev;
> packets coming from the secondary char dev will be dropped
>

I will fix it in next version.

>> 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>
>> ---
>> +/*
>> + * called from the main thread on the primary
>> + * for get packets
> s/get/getting/

OK

>> +++ b/qemu-options.hx
>> @@ -3866,6 +3866,12 @@ Dump the network traffic on netdev @var{dev} to the file specified by
>>   The file format is libpcap, so it can be analyzed with tools such as tcpdump
>>   or Wireshark.
>>   
>> +@item -object colo-compare,id=@var{id},primary_in=@var{chardevid},secondary_in=@var{chardevid},
>> +outdev=@var{chardevid}
>> +
>> +Colo-compare get packet from primary_in@var{chardevid} and secondary_in@var{chardevid},
> s/get/gets/

OK

>> +and output to outdev@var{chardevid}, we can use it with the help of filter-mirror and filter-redirector.
> s/output/outputs/
>
> Worth any better hints here about _how_ to use it with filter-mirror and
> filter-redirector, or should the last phrase just be dropped?

I will add how_to in next version.

Thanks
Zhang Chen

>

On 04/28/2016 05:04 PM, Zhang Chen wrote:
>
>
> On 04/28/2016 04:17 PM, Jason Wang wrote:
>>
>> On 04/28/2016 03:55 PM, Zhang Chen wrote:
>>>
>>> On 04/28/2016 03:16 PM, Jason Wang wrote:
>>>> On 04/28/2016 02:53 PM, Jason Wang wrote:
>>>>> +static void compare_set_outdev(Object *obj, const char *value,
>>>>> Error **errp)
>>>>>> +{
>>>>>> +    CompareState *s = COLO_COMPARE(obj);
>>>>>> +
>>>>>> +    g_free(s->outdev);
>>>>>> +    s->outdev = g_strdup(value);
>>>>>> +}
>>>>>> +
>>>>>> +/*
>>>>>> + * called from the main thread on the primary
>>>>>> + * to setup colo-compare.
>>>>>> + */
>>>>>> +static void colo_compare_complete(UserCreatable *uc, Error **errp)
>>>>>> +{
>>>>>> +    CompareState *s = COLO_COMPARE(uc);
>>>>>> +
>>>>>> +    if (!s->pri_indev || !s->sec_indev || !s->outdev) {
>>>>>> +        error_setg(errp, "colo compare needs 'primary_in' ,"
>>>>>> +                   "'secondary_in','outdev' property set");
>>>>>> +        return;
>>>>>> +    } else if (!strcmp(s->pri_indev, s->outdev) ||
>>>>>> +               !strcmp(s->sec_indev, s->outdev) ||
>>>>>> +               !strcmp(s->pri_indev, s->sec_indev)) {
>>>>>> +        error_setg(errp, "'indev' and 'outdev' could not be same "
>>>>>> +                   "for compare module");
>>>>>> +        return;
>>>>>> +    }
>>>>>> +
>>>>>> +    s->chr_pri_in = qemu_chr_find(s->pri_indev);
>>>>>> +    if (s->chr_pri_in == NULL) {
>>>>>> +        error_setg(errp, "Primary IN Device '%s' not found",
>>>>>> +                   s->pri_indev);
>>>>>> +        return;
>>>>>> +    }
>>>>>> +
>>>>>> +    s->chr_sec_in = qemu_chr_find(s->sec_indev);
>>>>>> +    if (s->chr_sec_in == NULL) {
>>>>>> +        error_setg(errp, "Secondary IN Device '%s' not found",
>>>>>> +                   s->sec_indev);
>>>>>> +        return;
>>>>>> +    }
>>>>>> +
>>>>>> +    s->chr_out = qemu_chr_find(s->outdev);
>>>>>> +    if (s->chr_out == NULL) {
>>>>>> +        error_setg(errp, "OUT Device '%s' not found", s->outdev);
>>>>>> +        return;
>>>>>> +    }
>>>>>> +
>>>>>> +    qemu_chr_fe_claim_no_fail(s->chr_pri_in);
>>>>>> +    qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read,
>>>>>> +                          compare_pri_chr_in, NULL, s);
>>>>>> +
>>>>>> +    qemu_chr_fe_claim_no_fail(s->chr_sec_in);
>>>>>> +    qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read,
>>>>>> +                          compare_sec_chr_in, NULL, s);
>>>>>> +
>>>> Btw, what's the reason of handling this in main loop? I thought it
>>>> would
>>>> be better to do this in colo thread? Otherwise, you need lots of extra
>>>> synchronizations?
>>> Do you mean we should start/stop/do checkpoint it by colo-frame?
>> I mean we probably want to handle pri_in and sec_in in colo compare
>> thread. Through this way, there's no need for extra synchronization with
>> main loop.
>
> I get your point, but how to do this.
> Now, we use qemu_chr_add_handlers to do this job.

You probably want to start a new main loop in colo comparing thread.

>
>
> Thanks
> zhangchen

On 04/29/2016 10:03 AM, Jason Wang wrote:
>
> On 04/28/2016 05:04 PM, Zhang Chen wrote:
>>
>> On 04/28/2016 04:17 PM, Jason Wang wrote:
>>> On 04/28/2016 03:55 PM, Zhang Chen wrote:
>>>> On 04/28/2016 03:16 PM, Jason Wang wrote:
>>>>> On 04/28/2016 02:53 PM, Jason Wang wrote:
>>>>>> +static void compare_set_outdev(Object *obj, const char *value,
>>>>>> Error **errp)
>>>>>>> +{
>>>>>>> +    CompareState *s = COLO_COMPARE(obj);
>>>>>>> +
>>>>>>> +    g_free(s->outdev);
>>>>>>> +    s->outdev = g_strdup(value);
>>>>>>> +}
>>>>>>> +
>>>>>>> +/*
>>>>>>> + * called from the main thread on the primary
>>>>>>> + * to setup colo-compare.
>>>>>>> + */
>>>>>>> +static void colo_compare_complete(UserCreatable *uc, Error **errp)
>>>>>>> +{
>>>>>>> +    CompareState *s = COLO_COMPARE(uc);
>>>>>>> +
>>>>>>> +    if (!s->pri_indev || !s->sec_indev || !s->outdev) {
>>>>>>> +        error_setg(errp, "colo compare needs 'primary_in' ,"
>>>>>>> +                   "'secondary_in','outdev' property set");
>>>>>>> +        return;
>>>>>>> +    } else if (!strcmp(s->pri_indev, s->outdev) ||
>>>>>>> +               !strcmp(s->sec_indev, s->outdev) ||
>>>>>>> +               !strcmp(s->pri_indev, s->sec_indev)) {
>>>>>>> +        error_setg(errp, "'indev' and 'outdev' could not be same "
>>>>>>> +                   "for compare module");
>>>>>>> +        return;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    s->chr_pri_in = qemu_chr_find(s->pri_indev);
>>>>>>> +    if (s->chr_pri_in == NULL) {
>>>>>>> +        error_setg(errp, "Primary IN Device '%s' not found",
>>>>>>> +                   s->pri_indev);
>>>>>>> +        return;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    s->chr_sec_in = qemu_chr_find(s->sec_indev);
>>>>>>> +    if (s->chr_sec_in == NULL) {
>>>>>>> +        error_setg(errp, "Secondary IN Device '%s' not found",
>>>>>>> +                   s->sec_indev);
>>>>>>> +        return;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    s->chr_out = qemu_chr_find(s->outdev);
>>>>>>> +    if (s->chr_out == NULL) {
>>>>>>> +        error_setg(errp, "OUT Device '%s' not found", s->outdev);
>>>>>>> +        return;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    qemu_chr_fe_claim_no_fail(s->chr_pri_in);
>>>>>>> +    qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read,
>>>>>>> +                          compare_pri_chr_in, NULL, s);
>>>>>>> +
>>>>>>> +    qemu_chr_fe_claim_no_fail(s->chr_sec_in);
>>>>>>> +    qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read,
>>>>>>> +                          compare_sec_chr_in, NULL, s);
>>>>>>> +
>>>>> Btw, what's the reason of handling this in main loop? I thought it
>>>>> would
>>>>> be better to do this in colo thread? Otherwise, you need lots of extra
>>>>> synchronizations?
>>>> Do you mean we should start/stop/do checkpoint it by colo-frame?
>>> I mean we probably want to handle pri_in and sec_in in colo compare
>>> thread. Through this way, there's no need for extra synchronization with
>>> main loop.
>> I get your point, but how to do this.
>> Now, we use qemu_chr_add_handlers to do this job.
> You probably want to start a new main loop in colo comparing thread.

Get it~ I will fix it in next version~
Thanks~~
Zhang Chen


>
>>
>> Thanks
>> zhangchen
>
>
> .
>

On 04/29/2016 10:03 AM, Jason Wang wrote:
>
> On 04/28/2016 05:04 PM, Zhang Chen wrote:
>>
>> On 04/28/2016 04:17 PM, Jason Wang wrote:
>>> On 04/28/2016 03:55 PM, Zhang Chen wrote:
>>>> On 04/28/2016 03:16 PM, Jason Wang wrote:
>>>>> On 04/28/2016 02:53 PM, Jason Wang wrote:
>>>>>> +static void compare_set_outdev(Object *obj, const char *value,
>>>>>> Error **errp)
>>>>>>> +{
>>>>>>> +    CompareState *s = COLO_COMPARE(obj);
>>>>>>> +
>>>>>>> +    g_free(s->outdev);
>>>>>>> +    s->outdev = g_strdup(value);
>>>>>>> +}
>>>>>>> +
>>>>>>> +/*
>>>>>>> + * called from the main thread on the primary
>>>>>>> + * to setup colo-compare.
>>>>>>> + */
>>>>>>> +static void colo_compare_complete(UserCreatable *uc, Error **errp)
>>>>>>> +{
>>>>>>> +    CompareState *s = COLO_COMPARE(uc);
>>>>>>> +
>>>>>>> +    if (!s->pri_indev || !s->sec_indev || !s->outdev) {
>>>>>>> +        error_setg(errp, "colo compare needs 'primary_in' ,"
>>>>>>> +                   "'secondary_in','outdev' property set");
>>>>>>> +        return;
>>>>>>> +    } else if (!strcmp(s->pri_indev, s->outdev) ||
>>>>>>> +               !strcmp(s->sec_indev, s->outdev) ||
>>>>>>> +               !strcmp(s->pri_indev, s->sec_indev)) {
>>>>>>> +        error_setg(errp, "'indev' and 'outdev' could not be same "
>>>>>>> +                   "for compare module");
>>>>>>> +        return;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    s->chr_pri_in = qemu_chr_find(s->pri_indev);
>>>>>>> +    if (s->chr_pri_in == NULL) {
>>>>>>> +        error_setg(errp, "Primary IN Device '%s' not found",
>>>>>>> +                   s->pri_indev);
>>>>>>> +        return;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    s->chr_sec_in = qemu_chr_find(s->sec_indev);
>>>>>>> +    if (s->chr_sec_in == NULL) {
>>>>>>> +        error_setg(errp, "Secondary IN Device '%s' not found",
>>>>>>> +                   s->sec_indev);
>>>>>>> +        return;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    s->chr_out = qemu_chr_find(s->outdev);
>>>>>>> +    if (s->chr_out == NULL) {
>>>>>>> +        error_setg(errp, "OUT Device '%s' not found", s->outdev);
>>>>>>> +        return;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    qemu_chr_fe_claim_no_fail(s->chr_pri_in);
>>>>>>> +    qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read,
>>>>>>> +                          compare_pri_chr_in, NULL, s);
>>>>>>> +
>>>>>>> +    qemu_chr_fe_claim_no_fail(s->chr_sec_in);
>>>>>>> +    qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read,
>>>>>>> +                          compare_sec_chr_in, NULL, s);
>>>>>>> +
>>>>> Btw, what's the reason of handling this in main loop? I thought it
>>>>> would
>>>>> be better to do this in colo thread? Otherwise, you need lots of extra
>>>>> synchronizations?
>>>> Do you mean we should start/stop/do checkpoint it by colo-frame?
>>> I mean we probably want to handle pri_in and sec_in in colo compare
>>> thread. Through this way, there's no need for extra synchronization with
>>> main loop.
>> I get your point, but how to do this.
>> Now, we use qemu_chr_add_handlers to do this job.
> You probably want to start a new main loop in colo comparing thread.


IIUC, do you mean
- remove char device read_handler

  ↓at colo comparing thread↓
while (true) {
- blocking read packet from char device with select(2)/poll(2)...
- compare packet
}

This solution will lead comparing packet and reading packet in serial.
But i don't know if this will have a good performance.

>>
>> Thanks
>> zhangchen
>
>
> .
>

On 05/06/2016 01:42 PM, Zhang Chen wrote:
>
>
> On 04/29/2016 10:03 AM, Jason Wang wrote:
>>
>> On 04/28/2016 05:04 PM, Zhang Chen wrote:
>>>
>>> On 04/28/2016 04:17 PM, Jason Wang wrote:
>>>> On 04/28/2016 03:55 PM, Zhang Chen wrote:
>>>>> On 04/28/2016 03:16 PM, Jason Wang wrote:
>>>>>> On 04/28/2016 02:53 PM, Jason Wang wrote:
>>>>>>> +static void compare_set_outdev(Object *obj, const char *value,
>>>>>>> Error **errp)
>>>>>>>> +{
>>>>>>>> +    CompareState *s = COLO_COMPARE(obj);
>>>>>>>> +
>>>>>>>> +    g_free(s->outdev);
>>>>>>>> +    s->outdev = g_strdup(value);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +/*
>>>>>>>> + * called from the main thread on the primary
>>>>>>>> + * to setup colo-compare.
>>>>>>>> + */
>>>>>>>> +static void colo_compare_complete(UserCreatable *uc, Error
>>>>>>>> **errp)
>>>>>>>> +{
>>>>>>>> +    CompareState *s = COLO_COMPARE(uc);
>>>>>>>> +
>>>>>>>> +    if (!s->pri_indev || !s->sec_indev || !s->outdev) {
>>>>>>>> +        error_setg(errp, "colo compare needs 'primary_in' ,"
>>>>>>>> +                   "'secondary_in','outdev' property set");
>>>>>>>> +        return;
>>>>>>>> +    } else if (!strcmp(s->pri_indev, s->outdev) ||
>>>>>>>> +               !strcmp(s->sec_indev, s->outdev) ||
>>>>>>>> +               !strcmp(s->pri_indev, s->sec_indev)) {
>>>>>>>> +        error_setg(errp, "'indev' and 'outdev' could not be
>>>>>>>> same "
>>>>>>>> +                   "for compare module");
>>>>>>>> +        return;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    s->chr_pri_in = qemu_chr_find(s->pri_indev);
>>>>>>>> +    if (s->chr_pri_in == NULL) {
>>>>>>>> +        error_setg(errp, "Primary IN Device '%s' not found",
>>>>>>>> +                   s->pri_indev);
>>>>>>>> +        return;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    s->chr_sec_in = qemu_chr_find(s->sec_indev);
>>>>>>>> +    if (s->chr_sec_in == NULL) {
>>>>>>>> +        error_setg(errp, "Secondary IN Device '%s' not found",
>>>>>>>> +                   s->sec_indev);
>>>>>>>> +        return;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    s->chr_out = qemu_chr_find(s->outdev);
>>>>>>>> +    if (s->chr_out == NULL) {
>>>>>>>> +        error_setg(errp, "OUT Device '%s' not found", s->outdev);
>>>>>>>> +        return;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    qemu_chr_fe_claim_no_fail(s->chr_pri_in);
>>>>>>>> +    qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read,
>>>>>>>> +                          compare_pri_chr_in, NULL, s);
>>>>>>>> +
>>>>>>>> +    qemu_chr_fe_claim_no_fail(s->chr_sec_in);
>>>>>>>> +    qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read,
>>>>>>>> +                          compare_sec_chr_in, NULL, s);
>>>>>>>> +
>>>>>> Btw, what's the reason of handling this in main loop? I thought it
>>>>>> would
>>>>>> be better to do this in colo thread? Otherwise, you need lots of
>>>>>> extra
>>>>>> synchronizations?
>>>>> Do you mean we should start/stop/do checkpoint it by colo-frame?
>>>> I mean we probably want to handle pri_in and sec_in in colo compare
>>>> thread. Through this way, there's no need for extra synchronization
>>>> with
>>>> main loop.
>>> I get your point, but how to do this.
>>> Now, we use qemu_chr_add_handlers to do this job.
>> You probably want to start a new main loop in colo comparing thread.
>
>
> IIUC, do you mean
> - remove char device read_handler
>
>  ↓at colo comparing thread↓
> while (true) {
> - blocking read packet from char device with select(2)/poll(2)...
> - compare packet
> }

Yes, something like this.

>
> This solution will lead comparing packet and reading packet in serial.
> But i don't know if this will have a good performance.

This probably won't have the best performance but it simplify lots of
things. Actually doing it in main loop will slow down all other I/O
processing. Consider colo can only handling userspace network traffic
now, we could start from this. For performance, it needs lots of other
stuff: I think the most important thing is to add vhost support.

Thanks

>
>>>
>>> Thanks
>>> zhangchen
>>
>>
>> .
>>
>

> +
> +    s->chr_sec_in = qemu_chr_find(s->sec_indev);
> +    if (s->chr_sec_in == NULL) {
> +        error_setg(errp, "Secondary IN Device '%s' not found",
> +                   s->sec_indev);
> +        return;
> +    }
> +
> +    s->chr_out = qemu_chr_find(s->outdev);
> +    if (s->chr_out == NULL) {
> +        error_setg(errp, "OUT Device '%s' not found", s->outdev);
> +        return;
> +    }
> +
> +    qemu_chr_fe_claim_no_fail(s->chr_pri_in);
> +    qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read,
> +                          compare_pri_chr_in, NULL, s);
> +
> +    qemu_chr_fe_claim_no_fail(s->chr_sec_in);
> +    qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read,
> +                          compare_sec_chr_in, NULL, s);
> +
>>>>>>> Btw, what's the reason of handling this in main loop? I thought it
>>>>>>> would
>>>>>>> be better to do this in colo thread? Otherwise, you need lots of
>>>>>>> extra
>>>>>>> synchronizations?
>>>>>> Do you mean we should start/stop/do checkpoint it by colo-frame?
>>>>> I mean we probably want to handle pri_in and sec_in in colo compare
>>>>> thread. Through this way, there's no need for extra synchronization
>>>>> with
>>>>> main loop.
>>>> I get your point, but how to do this.
>>>> Now, we use qemu_chr_add_handlers to do this job.
>>> You probably want to start a new main loop in colo comparing thread.
>>
>> IIUC, do you mean
>> - remove char device read_handler
>>
>>   ↓at colo comparing thread↓
>> while (true) {
>> - blocking read packet from char device with select(2)/poll(2)...
>> - compare packet
>> }
> Yes, something like this.
>

But remove qemu_chr_add_handlers I can't get fd to select/poll.

How to get fd from all kinds of chardev?

Thanks
Zhang Chen

>> This solution will lead comparing packet and reading packet in serial.
>> But i don't know if this will have a good performance.
> This probably won't have the best performance but it simplify lots of
> things. Actually doing it in main loop will slow down all other I/O
> processing. Consider colo can only handling userspace network traffic
> now, we could start from this. For performance, it needs lots of other
> stuff: I think the most important thing is to add vhost support.
>
> Thanks
>
>>>> Thanks
>>>> zhangchen
>>>
>>> .
>>>
>
>
> .
>

On 05/09/2016 06:49 PM, Zhang Chen wrote:
>
>> +
>> +    s->chr_sec_in = qemu_chr_find(s->sec_indev);
>> +    if (s->chr_sec_in == NULL) {
>> +        error_setg(errp, "Secondary IN Device '%s' not found",
>> +                   s->sec_indev);
>> +        return;
>> +    }
>> +
>> +    s->chr_out = qemu_chr_find(s->outdev);
>> +    if (s->chr_out == NULL) {
>> +        error_setg(errp, "OUT Device '%s' not found", s->outdev);
>> +        return;
>> +    }
>> +
>> +    qemu_chr_fe_claim_no_fail(s->chr_pri_in);
>> +    qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read,
>> +                          compare_pri_chr_in, NULL, s);
>> +
>> +    qemu_chr_fe_claim_no_fail(s->chr_sec_in);
>> +    qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read,
>> +                          compare_sec_chr_in, NULL, s);
>> +
>>>>>>>> Btw, what's the reason of handling this in main loop? I thought it
>>>>>>>> would
>>>>>>>> be better to do this in colo thread? Otherwise, you need lots of
>>>>>>>> extra
>>>>>>>> synchronizations?
>>>>>>> Do you mean we should start/stop/do checkpoint it by colo-frame?
>>>>>> I mean we probably want to handle pri_in and sec_in in colo compare
>>>>>> thread. Through this way, there's no need for extra synchronization
>>>>>> with
>>>>>> main loop.
>>>>> I get your point, but how to do this.
>>>>> Now, we use qemu_chr_add_handlers to do this job.
>>>> You probably want to start a new main loop in colo comparing thread.
>>>
>>> IIUC, do you mean
>>> - remove char device read_handler
>>>
>>>   ↓at colo comparing thread↓
>>> while (true) {
>>> - blocking read packet from char device with select(2)/poll(2)...
>>> - compare packet
>>> }
>> Yes, something like this.
>>
>
> But remove qemu_chr_add_handlers I can't get fd to select/poll.
>
> How to get fd from all kinds of chardev?
>

Hi~ jason.

If we use chardev socket the fd save in QIOChannelSocket.

and if we use chardev file the fd save in QIOChannelFile.

Have any common method to get fd?

> Thanks
> Zhang Chen
>
>>> This solution will lead comparing packet and reading packet in serial.
>>> But i don't know if this will have a good performance.
>> This probably won't have the best performance but it simplify lots of
>> things. Actually doing it in main loop will slow down all other I/O
>> processing. Consider colo can only handling userspace network traffic
>> now, we could start from this. For performance, it needs lots of other
>> stuff: I think the most important thing is to add vhost support.
>>
>> Thanks
>>
>>>>> Thanks
>>>>> zhangchen
>>>>
>>>> .
>>>>
>>
>>
>> .
>>
>

On 2016年05月12日 14:49, Zhang Chen wrote:
>
>
> On 05/09/2016 06:49 PM, Zhang Chen wrote:
>>
>>> +
>>> +    s->chr_sec_in = qemu_chr_find(s->sec_indev);
>>> +    if (s->chr_sec_in == NULL) {
>>> +        error_setg(errp, "Secondary IN Device '%s' not found",
>>> +                   s->sec_indev);
>>> +        return;
>>> +    }
>>> +
>>> +    s->chr_out = qemu_chr_find(s->outdev);
>>> +    if (s->chr_out == NULL) {
>>> +        error_setg(errp, "OUT Device '%s' not found", s->outdev);
>>> +        return;
>>> +    }
>>> +
>>> +    qemu_chr_fe_claim_no_fail(s->chr_pri_in);
>>> +    qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read,
>>> +                          compare_pri_chr_in, NULL, s);
>>> +
>>> +    qemu_chr_fe_claim_no_fail(s->chr_sec_in);
>>> +    qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read,
>>> +                          compare_sec_chr_in, NULL, s);
>>> +
>>>>>>>>> Btw, what's the reason of handling this in main loop? I 
>>>>>>>>> thought it
>>>>>>>>> would
>>>>>>>>> be better to do this in colo thread? Otherwise, you need lots of
>>>>>>>>> extra
>>>>>>>>> synchronizations?
>>>>>>>> Do you mean we should start/stop/do checkpoint it by colo-frame?
>>>>>>> I mean we probably want to handle pri_in and sec_in in colo compare
>>>>>>> thread. Through this way, there's no need for extra synchronization
>>>>>>> with
>>>>>>> main loop.
>>>>>> I get your point, but how to do this.
>>>>>> Now, we use qemu_chr_add_handlers to do this job.
>>>>> You probably want to start a new main loop in colo comparing thread.
>>>>
>>>> IIUC, do you mean
>>>> - remove char device read_handler
>>>>
>>>>   ↓at colo comparing thread↓
>>>> while (true) {
>>>> - blocking read packet from char device with select(2)/poll(2)...
>>>> - compare packet
>>>> }
>>> Yes, something like this.
>>>
>>
>> But remove qemu_chr_add_handlers I can't get fd to select/poll.
>>
>> How to get fd from all kinds of chardev?
>>
>
> Hi~ jason.
>
> If we use chardev socket the fd save in QIOChannelSocket.
>
> and if we use chardev file the fd save in QIOChannelFile.
>
> Have any common method to get fd?

I'm not sure I get the question. But you probably can call 
qemu_chr_add_handlers() in colo comparing thread to solve this I think?

>
>> Thanks
>> Zhang Chen
>>
>>>> This solution will lead comparing packet and reading packet in serial.
>>>> But i don't know if this will have a good performance.
>>> This probably won't have the best performance but it simplify lots of
>>> things. Actually doing it in main loop will slow down all other I/O
>>> processing. Consider colo can only handling userspace network traffic
>>> now, we could start from this. For performance, it needs lots of other
>>> stuff: I think the most important thing is to add vhost support.
>>>
>>> Thanks
>>>
>>>>>> Thanks
>>>>>> zhangchen
>>>>>
>>>>> .
>>>>>
>>>
>>>
>>> .
>>>
>>
>

On 05/12/2016 04:01 PM, Jason Wang wrote:
>
>
> On 2016年05月12日 14:49, Zhang Chen wrote:
>>
>>
>> On 05/09/2016 06:49 PM, Zhang Chen wrote:
>>>
>>>> +
>>>> +    s->chr_sec_in = qemu_chr_find(s->sec_indev);
>>>> +    if (s->chr_sec_in == NULL) {
>>>> +        error_setg(errp, "Secondary IN Device '%s' not found",
>>>> +                   s->sec_indev);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    s->chr_out = qemu_chr_find(s->outdev);
>>>> +    if (s->chr_out == NULL) {
>>>> +        error_setg(errp, "OUT Device '%s' not found", s->outdev);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    qemu_chr_fe_claim_no_fail(s->chr_pri_in);
>>>> +    qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read,
>>>> +                          compare_pri_chr_in, NULL, s);
>>>> +
>>>> +    qemu_chr_fe_claim_no_fail(s->chr_sec_in);
>>>> +    qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read,
>>>> +                          compare_sec_chr_in, NULL, s);
>>>> +
>>>>>>>>>> Btw, what's the reason of handling this in main loop? I 
>>>>>>>>>> thought it
>>>>>>>>>> would
>>>>>>>>>> be better to do this in colo thread? Otherwise, you need lots of
>>>>>>>>>> extra
>>>>>>>>>> synchronizations?
>>>>>>>>> Do you mean we should start/stop/do checkpoint it by colo-frame?
>>>>>>>> I mean we probably want to handle pri_in and sec_in in colo 
>>>>>>>> compare
>>>>>>>> thread. Through this way, there's no need for extra 
>>>>>>>> synchronization
>>>>>>>> with
>>>>>>>> main loop.
>>>>>>> I get your point, but how to do this.
>>>>>>> Now, we use qemu_chr_add_handlers to do this job.
>>>>>> You probably want to start a new main loop in colo comparing thread.
>>>>>
>>>>> IIUC, do you mean
>>>>> - remove char device read_handler
>>>>>
>>>>>   ↓at colo comparing thread↓
>>>>> while (true) {
>>>>> - blocking read packet from char device with select(2)/poll(2)...
>>>>> - compare packet
>>>>> }
>>>> Yes, something like this.
>>>>
>>>
>>> But remove qemu_chr_add_handlers I can't get fd to select/poll.
>>>
>>> How to get fd from all kinds of chardev?
>>>
>>
>> Hi~ jason.
>>
>> If we use chardev socket the fd save in QIOChannelSocket.
>>
>> and if we use chardev file the fd save in QIOChannelFile.
>>
>> Have any common method to get fd?
>
> I'm not sure I get the question. But you probably can call 
> qemu_chr_add_handlers() in colo comparing thread to solve this I think?
>

I have tested call qemu_chr_add_handlers() in colo comparing thread, but 
when data come,
the handler always running in main loop.

Thanks
Zhang Chen

>>
>>> Thanks
>>> Zhang Chen
>>>
>>>>> This solution will lead comparing packet and reading packet in 
>>>>> serial.
>>>>> But i don't know if this will have a good performance.
>>>> This probably won't have the best performance but it simplify lots of
>>>> things. Actually doing it in main loop will slow down all other I/O
>>>> processing. Consider colo can only handling userspace network traffic
>>>> now, we could start from this. For performance, it needs lots of other
>>>> stuff: I think the most important thing is to add vhost support.
>>>>
>>>> Thanks
>>>>
>>>>>>> Thanks
>>>>>>> zhangchen
>>>>>>
>>>>>> .
>>>>>>
>>>>
>>>>
>>>> .
>>>>
>>>
>>
>
>
>
> .
>

On 2016年05月12日 16:16, Zhang Chen wrote:
>
>
> On 05/12/2016 04:01 PM, Jason Wang wrote:
>>
>>
>> On 2016年05月12日 14:49, Zhang Chen wrote:
>>>
>>>
>>> On 05/09/2016 06:49 PM, Zhang Chen wrote:
>>>>
>>>>> +
>>>>> +    s->chr_sec_in = qemu_chr_find(s->sec_indev);
>>>>> +    if (s->chr_sec_in == NULL) {
>>>>> +        error_setg(errp, "Secondary IN Device '%s' not found",
>>>>> +                   s->sec_indev);
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    s->chr_out = qemu_chr_find(s->outdev);
>>>>> +    if (s->chr_out == NULL) {
>>>>> +        error_setg(errp, "OUT Device '%s' not found", s->outdev);
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    qemu_chr_fe_claim_no_fail(s->chr_pri_in);
>>>>> +    qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read,
>>>>> +                          compare_pri_chr_in, NULL, s);
>>>>> +
>>>>> +    qemu_chr_fe_claim_no_fail(s->chr_sec_in);
>>>>> +    qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read,
>>>>> +                          compare_sec_chr_in, NULL, s);
>>>>> +
>>>>>>>>>>> Btw, what's the reason of handling this in main loop? I 
>>>>>>>>>>> thought it
>>>>>>>>>>> would
>>>>>>>>>>> be better to do this in colo thread? Otherwise, you need 
>>>>>>>>>>> lots of
>>>>>>>>>>> extra
>>>>>>>>>>> synchronizations?
>>>>>>>>>> Do you mean we should start/stop/do checkpoint it by colo-frame?
>>>>>>>>> I mean we probably want to handle pri_in and sec_in in colo 
>>>>>>>>> compare
>>>>>>>>> thread. Through this way, there's no need for extra 
>>>>>>>>> synchronization
>>>>>>>>> with
>>>>>>>>> main loop.
>>>>>>>> I get your point, but how to do this.
>>>>>>>> Now, we use qemu_chr_add_handlers to do this job.
>>>>>>> You probably want to start a new main loop in colo comparing 
>>>>>>> thread.
>>>>>>
>>>>>> IIUC, do you mean
>>>>>> - remove char device read_handler
>>>>>>
>>>>>>   ↓at colo comparing thread↓
>>>>>> while (true) {
>>>>>> - blocking read packet from char device with select(2)/poll(2)...
>>>>>> - compare packet
>>>>>> }
>>>>> Yes, something like this.
>>>>>
>>>>
>>>> But remove qemu_chr_add_handlers I can't get fd to select/poll.
>>>>
>>>> How to get fd from all kinds of chardev?
>>>>
>>>
>>> Hi~ jason.
>>>
>>> If we use chardev socket the fd save in QIOChannelSocket.
>>>
>>> and if we use chardev file the fd save in QIOChannelFile.
>>>
>>> Have any common method to get fd?
>>
>> I'm not sure I get the question. But you probably can call 
>> qemu_chr_add_handlers() in colo comparing thread to solve this I think?
>>
>
> I have tested call qemu_chr_add_handlers() in colo comparing thread, 
> but when data come,
> the handler always running in main loop.
>
> Thanks
> Zhang Chen 

Cc Amit for the help.

Amit, we want to poll and handle chardev in another thread other than 
main loop. But looks like qemu_chr_add_handlers() can only work for 
default context other than thread default context. Any other solution 
for this?

Thanks

On 2016年05月13日 11:48, Jason Wang wrote:
>
>
> On 2016年05月12日 16:16, Zhang Chen wrote:
>>
>>
>> On 05/12/2016 04:01 PM, Jason Wang wrote:
>>>
>>>
>>> On 2016年05月12日 14:49, Zhang Chen wrote:
>>>>
>>>>
>>>> On 05/09/2016 06:49 PM, Zhang Chen wrote:
>>>>>
>>>>>> +
>>>>>> +    s->chr_sec_in = qemu_chr_find(s->sec_indev);
>>>>>> +    if (s->chr_sec_in == NULL) {
>>>>>> +        error_setg(errp, "Secondary IN Device '%s' not found",
>>>>>> +                   s->sec_indev);
>>>>>> +        return;
>>>>>> +    }
>>>>>> +
>>>>>> +    s->chr_out = qemu_chr_find(s->outdev);
>>>>>> +    if (s->chr_out == NULL) {
>>>>>> +        error_setg(errp, "OUT Device '%s' not found", s->outdev);
>>>>>> +        return;
>>>>>> +    }
>>>>>> +
>>>>>> +    qemu_chr_fe_claim_no_fail(s->chr_pri_in);
>>>>>> +    qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read,
>>>>>> +                          compare_pri_chr_in, NULL, s);
>>>>>> +
>>>>>> +    qemu_chr_fe_claim_no_fail(s->chr_sec_in);
>>>>>> +    qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read,
>>>>>> +                          compare_sec_chr_in, NULL, s);
>>>>>> +
>>>>>>>>>>>> Btw, what's the reason of handling this in main loop? I 
>>>>>>>>>>>> thought it
>>>>>>>>>>>> would
>>>>>>>>>>>> be better to do this in colo thread? Otherwise, you need 
>>>>>>>>>>>> lots of
>>>>>>>>>>>> extra
>>>>>>>>>>>> synchronizations?
>>>>>>>>>>> Do you mean we should start/stop/do checkpoint it by 
>>>>>>>>>>> colo-frame?
>>>>>>>>>> I mean we probably want to handle pri_in and sec_in in colo 
>>>>>>>>>> compare
>>>>>>>>>> thread. Through this way, there's no need for extra 
>>>>>>>>>> synchronization
>>>>>>>>>> with
>>>>>>>>>> main loop.
>>>>>>>>> I get your point, but how to do this.
>>>>>>>>> Now, we use qemu_chr_add_handlers to do this job.
>>>>>>>> You probably want to start a new main loop in colo comparing 
>>>>>>>> thread.
>>>>>>>
>>>>>>> IIUC, do you mean
>>>>>>> - remove char device read_handler
>>>>>>>
>>>>>>>   ↓at colo comparing thread↓
>>>>>>> while (true) {
>>>>>>> - blocking read packet from char device with select(2)/poll(2)...
>>>>>>> - compare packet
>>>>>>> }
>>>>>> Yes, something like this.
>>>>>>
>>>>>
>>>>> But remove qemu_chr_add_handlers I can't get fd to select/poll.
>>>>>
>>>>> How to get fd from all kinds of chardev?
>>>>>
>>>>
>>>> Hi~ jason.
>>>>
>>>> If we use chardev socket the fd save in QIOChannelSocket.
>>>>
>>>> and if we use chardev file the fd save in QIOChannelFile.
>>>>
>>>> Have any common method to get fd?
>>>
>>> I'm not sure I get the question. But you probably can call 
>>> qemu_chr_add_handlers() in colo comparing thread to solve this I think?
>>>
>>
>> I have tested call qemu_chr_add_handlers() in colo comparing thread, 
>> but when data come,
>> the handler always running in main loop.
>>
>> Thanks
>> Zhang Chen 
>
> Cc Amit for the help.
>
> Amit, we want to poll and handle chardev in another thread other than 
> main loop. But looks like qemu_chr_add_handlers() can only work for 
> default context other than thread default context. Any other solution 
> for this?
>
> Thanks
>

Cc Fam for more thought.

On Fri, 05/20 10:46, Jason Wang wrote:
> 
> 
> On 2016年05月13日 11:48, Jason Wang wrote:
> > 
> > 
> > On 2016年05月12日 16:16, Zhang Chen wrote:
> > > 
> > > 
> > > On 05/12/2016 04:01 PM, Jason Wang wrote:
> > > > 
> > > > 
> > > > On 2016年05月12日 14:49, Zhang Chen wrote:
> > > > > 
> > > > > 
> > > > > On 05/09/2016 06:49 PM, Zhang Chen wrote:
> > > > > > 
> > > > > > > +
> > > > > > > +    s->chr_sec_in = qemu_chr_find(s->sec_indev);
> > > > > > > +    if (s->chr_sec_in == NULL) {
> > > > > > > +        error_setg(errp, "Secondary IN Device '%s' not found",
> > > > > > > +                   s->sec_indev);
> > > > > > > +        return;
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    s->chr_out = qemu_chr_find(s->outdev);
> > > > > > > +    if (s->chr_out == NULL) {
> > > > > > > +        error_setg(errp, "OUT Device '%s' not found", s->outdev);
> > > > > > > +        return;
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    qemu_chr_fe_claim_no_fail(s->chr_pri_in);
> > > > > > > +    qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read,
> > > > > > > +                          compare_pri_chr_in, NULL, s);
> > > > > > > +
> > > > > > > +    qemu_chr_fe_claim_no_fail(s->chr_sec_in);
> > > > > > > +    qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read,
> > > > > > > +                          compare_sec_chr_in, NULL, s);
> > > > > > > +
> > > > > > > > > > > > > Btw, what's the reason of
> > > > > > > > > > > > > handling this in main loop?
> > > > > > > > > > > > > I thought it
> > > > > > > > > > > > > would
> > > > > > > > > > > > > be better to do this in colo
> > > > > > > > > > > > > thread? Otherwise, you need
> > > > > > > > > > > > > lots of
> > > > > > > > > > > > > extra
> > > > > > > > > > > > > synchronizations?
> > > > > > > > > > > > Do you mean we should
> > > > > > > > > > > > start/stop/do checkpoint it by
> > > > > > > > > > > > colo-frame?
> > > > > > > > > > > I mean we probably want to handle
> > > > > > > > > > > pri_in and sec_in in colo compare
> > > > > > > > > > > thread. Through this way, there's no
> > > > > > > > > > > need for extra synchronization
> > > > > > > > > > > with
> > > > > > > > > > > main loop.
> > > > > > > > > > I get your point, but how to do this.
> > > > > > > > > > Now, we use qemu_chr_add_handlers to do this job.
> > > > > > > > > You probably want to start a new main loop
> > > > > > > > > in colo comparing thread.
> > > > > > > > 
> > > > > > > > IIUC, do you mean
> > > > > > > > - remove char device read_handler
> > > > > > > > 
> > > > > > > >   ↓at colo comparing thread↓
> > > > > > > > while (true) {
> > > > > > > > - blocking read packet from char device with select(2)/poll(2)...
> > > > > > > > - compare packet
> > > > > > > > }
> > > > > > > Yes, something like this.
> > > > > > > 
> > > > > > 
> > > > > > But remove qemu_chr_add_handlers I can't get fd to select/poll.
> > > > > > 
> > > > > > How to get fd from all kinds of chardev?
> > > > > > 
> > > > > 
> > > > > Hi~ jason.
> > > > > 
> > > > > If we use chardev socket the fd save in QIOChannelSocket.
> > > > > 
> > > > > and if we use chardev file the fd save in QIOChannelFile.
> > > > > 
> > > > > Have any common method to get fd?
> > > > 
> > > > I'm not sure I get the question. But you probably can call
> > > > qemu_chr_add_handlers() in colo comparing thread to solve this I
> > > > think?
> > > > 
> > > 
> > > I have tested call qemu_chr_add_handlers() in colo comparing thread,
> > > but when data come,
> > > the handler always running in main loop.
> > > 
> > > Thanks
> > > Zhang Chen
> > 
> > Cc Amit for the help.
> > 
> > Amit, we want to poll and handle chardev in another thread other than
> > main loop. But looks like qemu_chr_add_handlers() can only work for
> > default context other than thread default context. Any other solution
> > for this?
> > 
> > Thanks
> > 
> 
> Cc Fam for more thought.
> 

Unfortunately QIOChannel in chardev uses GSource, so there is no easy way to
move that to another thread, at least I don't think any code in QEMU has ever
tried.

One possibility is in the colo compare thread, call
g_main_context_push_thread_default() before setting up the chr handler, but I'm
not sure how well that would work.

Fam