[V2,1/3] 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

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 | 344 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 vl.c               |   3 +-
 3 files changed, 347 insertions(+), 1 deletion(-)
 create mode 100644 net/colo-compare.c

* Zhang Chen (zhangchen.fnst@cn.fujitsu.com) wrote:
> packet come from primary char indev will be send to
> outdev - packet come from secondary char dev will be drop

Please put in the description an example of how you invoke
the filter on the primary and secondary.

> 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 | 344 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  vl.c               |   3 +-
>  3 files changed, 347 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..62c66df
> --- /dev/null
> +++ b/net/colo-compare.c
> @@ -0,0 +1,344 @@
> +/*
> + * 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 "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"
> +
> +#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);
> +
> +typedef struct ReadState {
> +    int state; /* 0 = getting length, 1 = getting data */
> +    unsigned int index;
> +    unsigned int packet_len;

Please make packet_len and index  uint32_t, since they're sent over the wire
as 32bit.

> +    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;
> +} CompareState;
> +
> +static int compare_chr_send(CharDriverState *out, const uint8_t *buf, int size)
> +{
> +    int ret = 0;
> +    uint32_t len = htonl(size);
> +

Similarly, make 'size' uint32_t - everything that gets converted into a uint32_t
it's probably best to make a uint32_t.

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

You can make this slightly simpler and save the return 0;

> +    return 0;
> +
> +err:
> +    return ret < 0 ? ret : -EIO;

err:
       return ret <= 0 ? ret : -EIO;

> +}
> +
> +static int compare_chr_can_read(void *opaque)
> +{
> +    return COMPARE_READ_LEN_MAX;
> +}
> +
> +/* 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("serious error: oversized packet received.");

Isn't it easier to do this check above in the 'got length' if ?
Instead of 'serious error' say where it's coming from;
  'colo-compare: Received oversized packet over socket'

that makes it a lot easier when people see the error for the first time.
Also, should you check for the error case of receiving a packet where
packet_len == 0 ?

> +                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;
> +}
> +
> +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 */
> +        compare_chr_send(s->chr_out, buf, size);
> +    } else if (ret == -1) {
> +        qemu_chr_add_handlers(s->chr_pri_in, NULL, NULL, NULL, NULL);
> +    }
> +}
> +
> +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*/
> +    } else if (ret == -1) {
> +        qemu_chr_add_handlers(s->chr_sec_in, NULL, NULL, NULL, NULL);
> +    }
> +}
> +
> +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);
> +}
> +
> +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_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,

I think error_set seems to be discouraged these days, just use error_setg
(see the comment in include/qapi/error.h just above error_set).

> +                  "IN Device '%s' not found", s->pri_indev);
> +        goto out;
> +    }
> +
> +    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);
> +
> +    s->chr_sec_in = qemu_chr_find(s->sec_indev);
> +    if (s->chr_sec_in == NULL) {
> +        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> +                  "IN Device '%s' not found", s->sec_indev);
> +        goto out;
> +    }

Can you explain/give an example of the way you create one of these
filters?
Would you ever have a pri_indev and sec_indev on the same filter?
If not, then why not just have an 'indev' option rather than the
two separate configs.
If you cna have both then you need to change hte error 'IN Device'
to say either 'Primary IN device' or secondary.

> +    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);
> +
> +    s->chr_out = qemu_chr_find(s->outdev);
> +    if (s->chr_out == NULL) {
> +        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> +                  "OUT Device '%s' not found", s->outdev);
> +        goto out;
> +    }
> +    qemu_chr_fe_claim_no_fail(s->chr_out);
> +
> +    QTAILQ_INSERT_TAIL(&net_compares, s, next);
> +
> +    return;
> +
> +out:
> +    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);
> +        s->chr_pri_in = NULL;
> +    }
> +    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);
> +        s->chr_pri_in = NULL;
> +    }

Can't you avoid this by making the code:

     s->chr_pri_in = qemu_chr_find(...)
     if (s->chr_pri_in == NULL) {
        error (...)
     }
     s->chr_sec_in = qemu_chr_find(...)
     if (s->chr_sec_in == NULL) {
        error (...)
     }
     s->chr_out = qemu_chr_find(...)
     if (s->chr_out == NULL) {
        error (...)
     }

     qemu_chr_fe_claim_no_fail(pri)
     add_handlers(pri...)
     qemu_chr_fe_claim_no_fail(sec)
     add_handlers(sec...)
     qemu_chr_fe_claim_no_fail(out)
     add_handlers(out...)

so you don't have to clean up the handlers/release in the out: ?

> +}
> +
> +static void colo_compare_class_init(ObjectClass *oc, void *data)
> +{
> +    UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
> +
> +    ucc->complete = colo_compare_complete;
> +}
> +
> +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(CompareState),
> +    .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/vl.c b/vl.c
> index dc6e63a..70064ad 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2842,7 +2842,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;
>      }
>  
> -- 
> 1.9.1
> 
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

On 03/30/2016 05:25 PM, Dr. David Alan Gilbert wrote:
> * Zhang Chen (zhangchen.fnst@cn.fujitsu.com) wrote:
>> packet come from primary char indev will be send to
>> outdev - packet come from secondary char dev will be drop
> Please put in the description an example of how you invoke
> the filter on the primary and secondary.

OK.
colo-compare get packet from chardev(primary_in,secondary_in),
and output to other chardev(outdev), so, we can use it with the
help of filter-mirror and filter-redirector. like that:

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 | 344 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   vl.c               |   3 +-
>>   3 files changed, 347 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..62c66df
>> --- /dev/null
>> +++ b/net/colo-compare.c
>> @@ -0,0 +1,344 @@
>> +/*
>> + * 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 "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"
>> +
>> +#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);
>> +
>> +typedef struct ReadState {
>> +    int state; /* 0 = getting length, 1 = getting data */
>> +    unsigned int index;
>> +    unsigned int packet_len;
> Please make packet_len and index  uint32_t, since they're sent over the wire
> as 32bit.
>
>> +    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;
>> +} CompareState;
>> +
>> +static int compare_chr_send(CharDriverState *out, const uint8_t *buf, int size)
>> +{
>> +    int ret = 0;
>> +    uint32_t len = htonl(size);
>> +
> Similarly, make 'size' uint32_t - everything that gets converted into a uint32_t
> it's probably best to make a uint32_t.
>
>> +    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;
>> +    }
>> +
> You can make this slightly simpler and save the return 0;
>
>> +    return 0;
>> +
>> +err:
>> +    return ret < 0 ? ret : -EIO;
> err:
>         return ret <= 0 ? ret : -EIO;
>
>> +}
>> +
>> +static int compare_chr_can_read(void *opaque)
>> +{
>> +    return COMPARE_READ_LEN_MAX;
>> +}
>> +
>> +/* 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("serious error: oversized packet received.");
> Isn't it easier to do this check above in the 'got length' if ?
> Instead of 'serious error' say where it's coming from;
>    'colo-compare: Received oversized packet over socket'
>
> that makes it a lot easier when people see the error for the first time.
> Also, should you check for the error case of receiving a packet where
> packet_len == 0 ?
>
>> +                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;
>> +}
>> +
>> +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 */
>> +        compare_chr_send(s->chr_out, buf, size);
>> +    } else if (ret == -1) {
>> +        qemu_chr_add_handlers(s->chr_pri_in, NULL, NULL, NULL, NULL);
>> +    }
>> +}
>> +
>> +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*/
>> +    } else if (ret == -1) {
>> +        qemu_chr_add_handlers(s->chr_sec_in, NULL, NULL, NULL, NULL);
>> +    }
>> +}
>> +
>> +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);
>> +}
>> +
>> +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_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> I think error_set seems to be discouraged these days, just use error_setg
> (see the comment in include/qapi/error.h just above error_set).
>
>> +                  "IN Device '%s' not found", s->pri_indev);
>> +        goto out;
>> +    }
>> +
>> +    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);
>> +
>> +    s->chr_sec_in = qemu_chr_find(s->sec_indev);
>> +    if (s->chr_sec_in == NULL) {
>> +        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
>> +                  "IN Device '%s' not found", s->sec_indev);
>> +        goto out;
>> +    }
> Can you explain/give an example of the way you create one of these
> filters?
> Would you ever have a pri_indev and sec_indev on the same filter?
> If not, then why not just have an 'indev' option rather than the
> two separate configs.
> If you cna have both then you need to change hte error 'IN Device'
> to say either 'Primary IN device' or secondary.
>
>> +    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);
>> +
>> +    s->chr_out = qemu_chr_find(s->outdev);
>> +    if (s->chr_out == NULL) {
>> +        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
>> +                  "OUT Device '%s' not found", s->outdev);
>> +        goto out;
>> +    }
>> +    qemu_chr_fe_claim_no_fail(s->chr_out);
>> +
>> +    QTAILQ_INSERT_TAIL(&net_compares, s, next);
>> +
>> +    return;
>> +
>> +out:
>> +    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);
>> +        s->chr_pri_in = NULL;
>> +    }
>> +    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);
>> +        s->chr_pri_in = NULL;
>> +    }
> Can't you avoid this by making the code:
>
>       s->chr_pri_in = qemu_chr_find(...)
>       if (s->chr_pri_in == NULL) {
>          error (...)
>       }
>       s->chr_sec_in = qemu_chr_find(...)
>       if (s->chr_sec_in == NULL) {
>          error (...)
>       }
>       s->chr_out = qemu_chr_find(...)
>       if (s->chr_out == NULL) {
>          error (...)
>       }
>
>       qemu_chr_fe_claim_no_fail(pri)
>       add_handlers(pri...)
>       qemu_chr_fe_claim_no_fail(sec)
>       add_handlers(sec...)
>       qemu_chr_fe_claim_no_fail(out)
>       add_handlers(out...)
>
> so you don't have to clean up the handlers/release in the out: ?
>
>> +}
>> +
>> +static void colo_compare_class_init(ObjectClass *oc, void *data)
>> +{
>> +    UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
>> +
>> +    ucc->complete = colo_compare_complete;
>> +}
>> +
>> +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(CompareState),
>> +    .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/vl.c b/vl.c
>> index dc6e63a..70064ad 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -2842,7 +2842,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;
>>       }
>>   
>> -- 
>> 1.9.1
>>
>>
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
>
> .
>

On 03/31/2016 09:41 AM, Zhang Chen wrote:
>
>
> On 03/30/2016 05:25 PM, Dr. David Alan Gilbert wrote:
>> * Zhang Chen (zhangchen.fnst@cn.fujitsu.com) wrote:
>>> packet come from primary char indev will be send to
>>> outdev - packet come from secondary char dev will be drop
>> Please put in the description an example of how you invoke
>> the filter on the primary and secondary.
>
> OK.
> colo-compare get packet from chardev(primary_in,secondary_in),
> and output to other chardev(outdev), so, we can use it with the
> help of filter-mirror and filter-redirector. like that:
>
> 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 | 344 
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   vl.c               |   3 +-
>>>   3 files changed, 347 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..62c66df
>>> --- /dev/null
>>> +++ b/net/colo-compare.c
>>> @@ -0,0 +1,344 @@
>>> +/*
>>> + * 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 "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"
>>> +
>>> +#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);
>>> +
>>> +typedef struct ReadState {
>>> +    int state; /* 0 = getting length, 1 = getting data */
>>> +    unsigned int index;
>>> +    unsigned int packet_len;
>> Please make packet_len and index  uint32_t, since they're sent over 
>> the wire
>> as 32bit.

OK~ will fix.

>>
>>> +    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;
>>> +} CompareState;
>>> +
>>> +static int compare_chr_send(CharDriverState *out, const uint8_t 
>>> *buf, int size)
>>> +{
>>> +    int ret = 0;
>>> +    uint32_t len = htonl(size);
>>> +
>> Similarly, make 'size' uint32_t - everything that gets converted into 
>> a uint32_t
>> it's probably best to make a uint32_t.

OK

>>
>>> +    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;
>>> +    }
>>> +
>> You can make this slightly simpler and save the return 0;
>>

will fix~~

>>> +    return 0;
>>> +
>>> +err:
>>> +    return ret < 0 ? ret : -EIO;
>> err:
>>         return ret <= 0 ? ret : -EIO;
>>
>>> +}
>>> +
>>> +static int compare_chr_can_read(void *opaque)
>>> +{
>>> +    return COMPARE_READ_LEN_MAX;
>>> +}
>>> +
>>> +/* 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("serious error: oversized packet 
>>> received.");
>> Isn't it easier to do this check above in the 'got length' if ?
>> Instead of 'serious error' say where it's coming from;
>>    'colo-compare: Received oversized packet over socket'
>>
>> that makes it a lot easier when people see the error for the first time.
>> Also, should you check for the error case of receiving a packet where
>> packet_len == 0 ?

Yes , will fix it in next version.

>>
>>> +                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;
>>> +}
>>> +
>>> +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 */
>>> +        compare_chr_send(s->chr_out, buf, size);
>>> +    } else if (ret == -1) {
>>> +        qemu_chr_add_handlers(s->chr_pri_in, NULL, NULL, NULL, NULL);
>>> +    }
>>> +}
>>> +
>>> +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*/
>>> +    } else if (ret == -1) {
>>> +        qemu_chr_add_handlers(s->chr_sec_in, NULL, NULL, NULL, NULL);
>>> +    }
>>> +}
>>> +
>>> +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);
>>> +}
>>> +
>>> +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_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
>> I think error_set seems to be discouraged these days, just use 
>> error_setg
>> (see the comment in include/qapi/error.h just above error_set).
>>

OK

>>> +                  "IN Device '%s' not found", s->pri_indev);
>>> +        goto out;
>>> +    }
>>> +
>>> +    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);
>>> +
>>> +    s->chr_sec_in = qemu_chr_find(s->sec_indev);
>>> +    if (s->chr_sec_in == NULL) {
>>> +        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
>>> +                  "IN Device '%s' not found", s->sec_indev);
>>> +        goto out;
>>> +    }
>> Can you explain/give an example of the way you create one of these
>> filters?

Yes, the way on top of this mail.

>> Would you ever have a pri_indev and sec_indev on the same filter?

Not same one.

>> If not, then why not just have an 'indev' option rather than the
>> two separate configs.
>> If you cna have both then you need to change hte error 'IN Device'
>> to say either 'Primary IN device' or secondary.

will fix


>>
>>> + 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);
>>> +
>>> +    s->chr_out = qemu_chr_find(s->outdev);
>>> +    if (s->chr_out == NULL) {
>>> +        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
>>> +                  "OUT Device '%s' not found", s->outdev);
>>> +        goto out;
>>> +    }
>>> +    qemu_chr_fe_claim_no_fail(s->chr_out);
>>> +
>>> +    QTAILQ_INSERT_TAIL(&net_compares, s, next);
>>> +
>>> +    return;
>>> +
>>> +out:
>>> +    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);
>>> +        s->chr_pri_in = NULL;
>>> +    }
>>> +    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);
>>> +        s->chr_pri_in = NULL;
>>> +    }
>> Can't you avoid this by making the code:
>>
>>       s->chr_pri_in = qemu_chr_find(...)
>>       if (s->chr_pri_in == NULL) {
>>          error (...)
>>       }
>>       s->chr_sec_in = qemu_chr_find(...)
>>       if (s->chr_sec_in == NULL) {
>>          error (...)
>>       }
>>       s->chr_out = qemu_chr_find(...)
>>       if (s->chr_out == NULL) {
>>          error (...)
>>       }
>>
>>       qemu_chr_fe_claim_no_fail(pri)
>>       add_handlers(pri...)
>>       qemu_chr_fe_claim_no_fail(sec)
>>       add_handlers(sec...)
>>       qemu_chr_fe_claim_no_fail(out)
>>       add_handlers(out...)
>>
>> so you don't have to clean up the handlers/release in the out: ?

I will fix code style to this.
we don't need set a handler for out, because we just send packet to out.
so we don't have to clean up out.

Thanks
Zhang Chen

>>
>>> +}
>>> +
>>> +static void colo_compare_class_init(ObjectClass *oc, void *data)
>>> +{
>>> +    UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
>>> +
>>> +    ucc->complete = colo_compare_complete;
>>> +}
>>> +
>>> +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(CompareState),
>>> +    .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/vl.c b/vl.c
>>> index dc6e63a..70064ad 100644
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -2842,7 +2842,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;
>>>       }
>>>   --
>>> 1.9.1
>>>
>>>
>>>
>> -- 
>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>>
>>
>> .
>>
>

* Zhang Chen (zhangchen.fnst@cn.fujitsu.com) wrote:
> 
> 
> On 03/30/2016 05:25 PM, Dr. David Alan Gilbert wrote:
> >* Zhang Chen (zhangchen.fnst@cn.fujitsu.com) wrote:
> >>packet come from primary char indev will be send to
> >>outdev - packet come from secondary char dev will be drop
> >Please put in the description an example of how you invoke
> >the filter on the primary and secondary.
> 
> OK.
> colo-compare get packet from chardev(primary_in,secondary_in),
> and output to other chardev(outdev), so, we can use it with the
> help of filter-mirror and filter-redirector. like that:

Wow, this is a bit more complicated than I expected - I was expecting just one
socket.  So let me draw this out; see comments below.

> 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

            ----> mirror0: socket:secondary:9003
            |
        mirror-m0     <-- e1000
            |
            v
        redirector-redire1 ---> compare0 socket:primary:9001 (to compare0-0)
                          
            -----< compare0-0 socket:primary:9001 (to compare0)
            |  primary_in
            |
        compare-comp0       -----> compare_out0 socket:primary:9005
            |
            |  secondary_in
            -----< compare1   socket:secondary:9004

tap <-- redirector-redire0 <--- compare_out socket:primary:9005 (from 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

            ----> red0 socket::9003
            |
tap <-- redirector-f1 <--
                           e1000
    --> redirector-f2 -->
            |
            ----< red1 socket::9004

OK, so I think we need to find a way to fix two things:
   a) There must be an easier way of connecting two filters within the same
      qemu than going up to the kernel's socket code, around it's TCP stack
      and back.  Is there a better type of chardev to use we can short circuit
      with - it shouldn't need to leave QEMU (although I can see it's easier
      for debugging like this).  Jason/Dan - any ideas?

   b) We should only need one socket for the connection between primary and
      secondary - I'm not sure how to change it to let it do that.

   c) You need to be able to turn off nagling (socket no delay) on the sockets;
     I found a noticeable benefit of doing this on the connection between primary
     and secondary in my world.

Dave

> 
> 
> 
> >
> >>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 | 344 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  vl.c               |   3 +-
> >>  3 files changed, 347 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..62c66df
> >>--- /dev/null
> >>+++ b/net/colo-compare.c
> >>@@ -0,0 +1,344 @@
> >>+/*
> >>+ * 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 "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"
> >>+
> >>+#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);
> >>+
> >>+typedef struct ReadState {
> >>+    int state; /* 0 = getting length, 1 = getting data */
> >>+    unsigned int index;
> >>+    unsigned int packet_len;
> >Please make packet_len and index  uint32_t, since they're sent over the wire
> >as 32bit.
> >
> >>+    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;
> >>+} CompareState;
> >>+
> >>+static int compare_chr_send(CharDriverState *out, const uint8_t *buf, int size)
> >>+{
> >>+    int ret = 0;
> >>+    uint32_t len = htonl(size);
> >>+
> >Similarly, make 'size' uint32_t - everything that gets converted into a uint32_t
> >it's probably best to make a uint32_t.
> >
> >>+    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;
> >>+    }
> >>+
> >You can make this slightly simpler and save the return 0;
> >
> >>+    return 0;
> >>+
> >>+err:
> >>+    return ret < 0 ? ret : -EIO;
> >err:
> >        return ret <= 0 ? ret : -EIO;
> >
> >>+}
> >>+
> >>+static int compare_chr_can_read(void *opaque)
> >>+{
> >>+    return COMPARE_READ_LEN_MAX;
> >>+}
> >>+
> >>+/* 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("serious error: oversized packet received.");
> >Isn't it easier to do this check above in the 'got length' if ?
> >Instead of 'serious error' say where it's coming from;
> >   'colo-compare: Received oversized packet over socket'
> >
> >that makes it a lot easier when people see the error for the first time.
> >Also, should you check for the error case of receiving a packet where
> >packet_len == 0 ?
> >
> >>+                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;
> >>+}
> >>+
> >>+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 */
> >>+        compare_chr_send(s->chr_out, buf, size);
> >>+    } else if (ret == -1) {
> >>+        qemu_chr_add_handlers(s->chr_pri_in, NULL, NULL, NULL, NULL);
> >>+    }
> >>+}
> >>+
> >>+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*/
> >>+    } else if (ret == -1) {
> >>+        qemu_chr_add_handlers(s->chr_sec_in, NULL, NULL, NULL, NULL);
> >>+    }
> >>+}
> >>+
> >>+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);
> >>+}
> >>+
> >>+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_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> >I think error_set seems to be discouraged these days, just use error_setg
> >(see the comment in include/qapi/error.h just above error_set).
> >
> >>+                  "IN Device '%s' not found", s->pri_indev);
> >>+        goto out;
> >>+    }
> >>+
> >>+    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);
> >>+
> >>+    s->chr_sec_in = qemu_chr_find(s->sec_indev);
> >>+    if (s->chr_sec_in == NULL) {
> >>+        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> >>+                  "IN Device '%s' not found", s->sec_indev);
> >>+        goto out;
> >>+    }
> >Can you explain/give an example of the way you create one of these
> >filters?
> >Would you ever have a pri_indev and sec_indev on the same filter?
> >If not, then why not just have an 'indev' option rather than the
> >two separate configs.
> >If you cna have both then you need to change hte error 'IN Device'
> >to say either 'Primary IN device' or secondary.
> >
> >>+    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);
> >>+
> >>+    s->chr_out = qemu_chr_find(s->outdev);
> >>+    if (s->chr_out == NULL) {
> >>+        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> >>+                  "OUT Device '%s' not found", s->outdev);
> >>+        goto out;
> >>+    }
> >>+    qemu_chr_fe_claim_no_fail(s->chr_out);
> >>+
> >>+    QTAILQ_INSERT_TAIL(&net_compares, s, next);
> >>+
> >>+    return;
> >>+
> >>+out:
> >>+    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);
> >>+        s->chr_pri_in = NULL;
> >>+    }
> >>+    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);
> >>+        s->chr_pri_in = NULL;
> >>+    }
> >Can't you avoid this by making the code:
> >
> >      s->chr_pri_in = qemu_chr_find(...)
> >      if (s->chr_pri_in == NULL) {
> >         error (...)
> >      }
> >      s->chr_sec_in = qemu_chr_find(...)
> >      if (s->chr_sec_in == NULL) {
> >         error (...)
> >      }
> >      s->chr_out = qemu_chr_find(...)
> >      if (s->chr_out == NULL) {
> >         error (...)
> >      }
> >
> >      qemu_chr_fe_claim_no_fail(pri)
> >      add_handlers(pri...)
> >      qemu_chr_fe_claim_no_fail(sec)
> >      add_handlers(sec...)
> >      qemu_chr_fe_claim_no_fail(out)
> >      add_handlers(out...)
> >
> >so you don't have to clean up the handlers/release in the out: ?
> >
> >>+}
> >>+
> >>+static void colo_compare_class_init(ObjectClass *oc, void *data)
> >>+{
> >>+    UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
> >>+
> >>+    ucc->complete = colo_compare_complete;
> >>+}
> >>+
> >>+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(CompareState),
> >>+    .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/vl.c b/vl.c
> >>index dc6e63a..70064ad 100644
> >>--- a/vl.c
> >>+++ b/vl.c
> >>@@ -2842,7 +2842,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;
> >>      }
> >>-- 
> >>1.9.1
> >>
> >>
> >>
> >--
> >Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >
> >
> >.
> >
> 
> -- 
> Thanks
> zhangchen
> 
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

On 03/31/2016 05:24 PM, Dr. David Alan Gilbert wrote:
> * Zhang Chen (zhangchen.fnst@cn.fujitsu.com) wrote:
>>
>> On 03/30/2016 05:25 PM, Dr. David Alan Gilbert wrote:
>>> * Zhang Chen (zhangchen.fnst@cn.fujitsu.com) wrote:
>>>> packet come from primary char indev will be send to
>>>> outdev - packet come from secondary char dev will be drop
>>> Please put in the description an example of how you invoke
>>> the filter on the primary and secondary.
>> OK.
>> colo-compare get packet from chardev(primary_in,secondary_in),
>> and output to other chardev(outdev), so, we can use it with the
>> help of filter-mirror and filter-redirector. like that:
> Wow, this is a bit more complicated than I expected - I was expecting just one
> socket.  So let me draw this out; see comments below.
>
>> 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
>             ----> mirror0: socket:secondary:9003
>             |
>         mirror-m0     <-- e1000
>             |
>             v
>         redirector-redire1 ---> compare0 socket:primary:9001 (to compare0-0)
>                           
>             -----< compare0-0 socket:primary:9001 (to compare0)
>             |  primary_in
>             |
>         compare-comp0       -----> compare_out0 socket:primary:9005
>             |
>             |  secondary_in
>             -----< compare1   socket:secondary:9004
>
> tap <-- redirector-redire0 <--- compare_out socket:primary:9005 (from 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
>             ----> red0 socket::9003
>             |
> tap <-- redirector-f1 <--
>                            e1000
>     --> redirector-f2 -->
>             |
>             ----< red1 socket::9004
>
> OK, so I think we need to find a way to fix two things:
>    a) There must be an easier way of connecting two filters within the same
>       qemu than going up to the kernel's socket code, around it's TCP stack
>       and back.  Is there a better type of chardev to use we can short circuit
>       with - it shouldn't need to leave QEMU (although I can see it's easier
>       for debugging like this).  Jason/Dan - any ideas?

Looks like there's no such type of chardev. I think we could start with
tcp socket chardev first and do necessary optimization on top. In fact,
there's also advantages, with tcp sockets, the colo-compare codes could
even be an independent program out of qemu.

For the chardev type, this also reminds me one thing. Since
mirror/redirector can only work for tcp socket, we may need inspect the
chardev types and fail if not a tcp socket like what vhost-user did.


>
>    b) We should only need one socket for the connection between primary and
>       secondary - I'm not sure how to change it to let it do that.

Looks like we can (e.g for secondary):

-device e1000,netdev=hn0,mac=52:a4:00:12:78:66
-chardev socket,id=red0,host=3.3.3.3,port=9003
-object filter-redirector,id=f1,netdev=hn0,queue=tx,indev=red0
-object filter-redirector,id=f2,netdev=hn0,queue=rx,outdev=red0

If not, probably a bug.

>    c) You need to be able to turn off nagling (socket no delay) on the sockets;
>      I found a noticeable benefit of doing this on the connection between primary
>      and secondary in my world.

Right.

>
> Dave
>

On 04/01/2016 01:11 PM, Jason Wang wrote:
>
>
> On 03/31/2016 05:24 PM, Dr. David Alan Gilbert wrote:
>> * Zhang Chen (zhangchen.fnst@cn.fujitsu.com) wrote:
>>>
>>> On 03/30/2016 05:25 PM, Dr. David Alan Gilbert wrote:
>>>> * Zhang Chen (zhangchen.fnst@cn.fujitsu.com) wrote:
>>>>> packet come from primary char indev will be send to
>>>>> outdev - packet come from secondary char dev will be drop
>>>> Please put in the description an example of how you invoke
>>>> the filter on the primary and secondary.
>>> OK.
>>> colo-compare get packet from chardev(primary_in,secondary_in),
>>> and output to other chardev(outdev), so, we can use it with the
>>> help of filter-mirror and filter-redirector. like that:
>> Wow, this is a bit more complicated than I expected - I was expecting just one
>> socket.  So let me draw this out; see comments below.
>>
>>> 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
>>              ----> mirror0: socket:secondary:9003
>>              |
>>          mirror-m0     <-- e1000
>>              |
>>              v
>>          redirector-redire1 ---> compare0 socket:primary:9001 (to compare0-0)
>>
>>              -----< compare0-0 socket:primary:9001 (to compare0)
>>              |  primary_in
>>              |
>>          compare-comp0       -----> compare_out0 socket:primary:9005
>>              |
>>              |  secondary_in
>>              -----< compare1   socket:secondary:9004
>>
>> tap <-- redirector-redire0 <--- compare_out socket:primary:9005 (from 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
>>              ----> red0 socket::9003
>>              |
>> tap <-- redirector-f1 <--
>>                             e1000
>>      --> redirector-f2 -->
>>              |
>>              ----< red1 socket::9004
>>
>> OK, so I think we need to find a way to fix two things:
>>     a) There must be an easier way of connecting two filters within the same
>>        qemu than going up to the kernel's socket code, around it's TCP stack
>>        and back.  Is there a better type of chardev to use we can short circuit
>>        with - it shouldn't need to leave QEMU (although I can see it's easier
>>        for debugging like this).  Jason/Dan - any ideas?
>
> Looks like there's no such type of chardev. I think we could start with
> tcp socket chardev first and do necessary optimization on top. In fact,
> there's also advantages, with tcp sockets, the colo-compare codes could
> even be an independent program out of qemu.
>
> For the chardev type, this also reminds me one thing. Since
> mirror/redirector can only work for tcp socket, we may need inspect the
> chardev types and fail if not a tcp socket like what vhost-user did.
>
>
>>
>>     b) We should only need one socket for the connection between primary and
>>        secondary - I'm not sure how to change it to let it do that.
>
> Looks like we can (e.g for secondary):
>
> -device e1000,netdev=hn0,mac=52:a4:00:12:78:66
> -chardev socket,id=red0,host=3.3.3.3,port=9003
> -object filter-redirector,id=f1,netdev=hn0,queue=tx,indev=red0
> -object filter-redirector,id=f2,netdev=hn0,queue=rx,outdev=red0
>
> If not, probably a bug.

Right,
but firstly, 'chardev socket' need to enable 'mux=on' if we want two filter reference the
same socket.

Thanks

>
>>     c) You need to be able to turn off nagling (socket no delay) on the sockets;
>>       I found a noticeable benefit of doing this on the connection between primary
>>       and secondary in my world.
>
> Right.
>
>>
>> Dave
>>
>
>
>
>
> .
>

>> +    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;
>> +    }
>> +
> You can make this slightly simpler and save the return 0;

If we want to save the return 0 , the code will be changed like that:

err:
     return (ret < 0 || ret == size) ? ret : -EIO;

I think it too complex to be understood, so should we keep the original ?

>> +    return 0;
>> +
>> +err:
>> +    return ret < 0 ? ret : -EIO;
> err:
>         return ret <= 0 ? ret : -EIO;

This is wrong, if qemu_chr_fe_write_all success, ret will equal size.
return -EIO.

>> +}
>> +
>> +static int compare_chr_can_read(void *opaque)
>> +{
>> +    return COMPARE_READ_LEN_MAX;
>> +}
>>