Re: [Qemu-devel] [PATCH V2 1/3] colo-compare: introduce colo compare initlization

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* 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