[Qemu-devel] [PULL 00/14] Net patches

[Qemu-devel] [PULL 00/14] Net patches

[Jason Wang]

The following changes since commit 1464ad45cd6cdeb0b5c1a54d3d3791396e47e52f:

  Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2016-03-04' into staging (2016-03-06 11:53:27 +0000)

are available in the git repository at:

  https://github.com/jasowang/qemu.git tags/net-pull-request

for you to fetch changes up to a2f2e45c6edbba9e1961056fa77c696208b40c8e:

  net: check packet payload length (2016-03-07 10:15:48 +0800)

----------------------------------------------------------------

- a new netfilter implementation: mirror
- netfilter could be disabled and enabled through qom-set now
- fix netfilter crash when specifiying wrong parameters
- rocker switch now can allow user to specifiy world
- fix OOB access for ne2000

----------------------------------------------------------------
Jason Wang (1):
      net: filter: correctly remove filter from the list during finalization

Jiri Pirko (4):
      rocker: forbid to change world type
      rocker: return -ENOMEM in case of some world alloc fails
      rocker: add name field into WorldOps ale let world specify its name
      rocker: allow user to specify rocker world by property

Paolo Bonzini (1):
      net: simplify net_init_tap_one logic

Prasad J Pandit (2):
      net: ne2000: check ring buffer control registers
      net: check packet payload length

Thomas Huth (1):
      MAINTAINERS: Add entries for include/net/ files

Vincenzo Maffione (1):
      net: netmap: probe netmap interface for virtio-net header

Zhang Chen (2):
      net/filter-mirror:Add filter-mirror
      tests/test-filter-mirror:add filter-mirror unit test

zhanghailiang (2):
      filter: Add 'status' property for filter object
      filter-buffer: Add status_changed callback processing

 MAINTAINERS                   |   2 +
 hw/net/ne2000.c               |   4 +
 hw/net/rocker/rocker.c        |  38 ++++++++-
 hw/net/rocker/rocker_fp.c     |   5 ++
 hw/net/rocker/rocker_fp.h     |   1 +
 hw/net/rocker/rocker_of_dpa.c |   1 +
 hw/net/rocker/rocker_world.c  |   7 +-
 hw/net/rocker/rocker_world.h  |   1 +
 include/net/filter.h          |   4 +
 net/Makefile.objs             |   1 +
 net/checksum.c                |  10 ++-
 net/filter-buffer.c           |  34 ++++++--
 net/filter-mirror.c           | 182 ++++++++++++++++++++++++++++++++++++++++++
 net/filter.c                  |  44 +++++++++-
 net/netmap.c                  |  59 +++++++++-----
 net/tap.c                     |   4 +-
 qemu-options.hx               |   9 ++-
 tests/.gitignore              |   1 +
 tests/Makefile                |   2 +
 tests/test-filter-mirror.c    |  90 +++++++++++++++++++++
 vl.c                          |   3 +-
 21 files changed, 460 insertions(+), 42 deletions(-)
 create mode 100644 net/filter-mirror.c
 create mode 100644 tests/test-filter-mirror.c

[Qemu-devel] [PULL 01/14] net: ne2000: check ring buffer control registers

[Jason Wang]

From: Prasad J Pandit <pjp@fedoraproject.org>

Ne2000 NIC uses ring buffer of NE2000_MEM_SIZE(49152)
bytes to process network packets. Registers PSTART & PSTOP
define ring buffer size & location. Setting these registers
to invalid values could lead to infinite loop or OOB r/w
access issues. Add check to avoid it.

Reported-by: Yang Hongke <yanghongke@huawei.com>
Tested-by: Yang Hongke <yanghongke@huawei.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/net/ne2000.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/net/ne2000.c b/hw/net/ne2000.c
index e408083..f0feaf9 100644
--- a/hw/net/ne2000.c
+++ b/hw/net/ne2000.c
@@ -155,6 +155,10 @@ static int ne2000_buffer_full(NE2000State *s)
 {
     int avail, index, boundary;
 
+    if (s->stop <= s->start) {
+        return 1;
+    }
+
     index = s->curpag << 8;
     boundary = s->boundary << 8;
     if (index < boundary)
-- 
2.5.0

[Qemu-devel] [PULL 02/14] net: filter: correctly remove filter from the list during finalization

[Jason Wang]

Qemu may crash when we want to add two filters on the same netdev but
the initialization of second fails (e.g missing parameters):

./qemu-system-x86_64 -netdev user,id=un0 \
 -object filter-buffer,id=f0,netdev=un0,interval=10 \
 -object filter-buffer,id=f1,netdev=un0
Segmentation fault (core dumped)

This is because we don't check whether or not the filter was in the
list of netdev. This patch fixes this.

Cc: Yang Hongyang <hongyang.yang@easystack.cn>
Reviewed-by: Yang Hongyang <hongyang.yang@easystack.cn>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 net/filter.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/filter.c b/net/filter.c
index d2a514e..7cdbc6c 100644
--- a/net/filter.c
+++ b/net/filter.c
@@ -196,7 +196,8 @@ static void netfilter_finalize(Object *obj)
         nfc->cleanup(nf);
     }
 
-    if (nf->netdev && !QTAILQ_EMPTY(&nf->netdev->filters)) {
+    if (nf->netdev && !QTAILQ_EMPTY(&nf->netdev->filters) &&
+        nf->next.tqe_prev) {
         QTAILQ_REMOVE(&nf->netdev->filters, nf, next);
     }
     g_free(nf->netdev_id);
-- 
2.5.0

[Qemu-devel] [PULL 03/14] MAINTAINERS: Add entries for include/net/ files

[Jason Wang]

From: Thomas Huth <thuth@redhat.com>

The include/net/ files correspond to the files in the net/ directory,
thus there should be corresponding entries in the MAINTAINERS file.

Signed-off-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 2f5a338..1fa9753 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1122,6 +1122,7 @@ Network device backends
 M: Jason Wang <jasowang@redhat.com>
 S: Maintained
 F: net/
+F: include/net/
 T: git git://github.com/jasowang/qemu.git net
 
 Netmap network backend
@@ -1221,6 +1222,7 @@ M: Jan Kiszka <jan.kiszka@siemens.com>
 S: Maintained
 F: slirp/
 F: net/slirp.c
+F: include/net/slirp.h
 T: git git://git.kiszka.org/qemu.git queues/slirp
 
 Tracing
-- 
2.5.0

[Qemu-devel] [PULL 04/14] net: simplify net_init_tap_one logic

[Jason Wang]

From: Paolo Bonzini <pbonzini@redhat.com>

net_init_tap_one receives in vhostfdname a fd name from vhostfd= or
vhostfds=, or NULL if there is no vhostfd=/vhostfds=.  It is simpler
to just check vhostfdname, than it is to check for vhostfd= or
vhostfds=.  This also calms down Coverity, which otherwise thinks
that monitor_fd_param could dereference a NULL vhostfdname.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 net/tap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/tap.c b/net/tap.c
index cfb6831..cd7a7fc 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -662,7 +662,7 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
         options.backend_type = VHOST_BACKEND_TYPE_KERNEL;
         options.net_backend = &s->nc;
 
-        if (tap->has_vhostfd || tap->has_vhostfds) {
+        if (vhostfdname) {
             vhostfd = monitor_fd_param(cur_mon, vhostfdname, &err);
             if (vhostfd == -1) {
                 error_propagate(errp, err);
@@ -684,7 +684,7 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
                        "vhost-net requested but could not be initialized");
             return;
         }
-    } else if (tap->has_vhostfd || tap->has_vhostfds) {
+    } else if (vhostfdname) {
         error_setg(errp, "vhostfd= is not valid without vhost");
     }
 }
-- 
2.5.0

[Qemu-devel] [PULL 05/14] net/filter-mirror:Add filter-mirror

[Jason Wang]

From: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>

Filter-mirror is a netfilter plugin.
It gives qemu the ability to mirror
packets to a chardev.

usage:

-netdev tap,id=hn0
-chardev socket,id=mirror0,host=ip_primary,port=X,server,nowait
-filter-mirror,id=m0,netdev=hn0,queue=tx/rx/all,outdev=mirror0

Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Reviewed-by: Yang Hongyang <hongyang.yang@easystack.cn>
Reviewed-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 net/Makefile.objs   |   1 +
 net/filter-mirror.c | 182 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu-options.hx     |   5 ++
 vl.c                |   3 +-
 4 files changed, 190 insertions(+), 1 deletion(-)
 create mode 100644 net/filter-mirror.c

diff --git a/net/Makefile.objs b/net/Makefile.objs
index 5fa2f97..b7c22fd 100644
--- a/net/Makefile.objs
+++ b/net/Makefile.objs
@@ -15,3 +15,4 @@ common-obj-$(CONFIG_VDE) += vde.o
 common-obj-$(CONFIG_NETMAP) += netmap.o
 common-obj-y += filter.o
 common-obj-y += filter-buffer.o
+common-obj-y += filter-mirror.o
diff --git a/net/filter-mirror.c b/net/filter-mirror.c
new file mode 100644
index 0000000..625b940
--- /dev/null
+++ b/net/filter-mirror.c
@@ -0,0 +1,182 @@
+/*
+ * Copyright (c) 2016 HUAWEI TECHNOLOGIES CO., LTD.
+ * Copyright (c) 2016 FUJITSU LIMITED
+ * Copyright (c) 2016 Intel Corporation
+ *
+ * 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 "net/filter.h"
+#include "net/net.h"
+#include "qemu-common.h"
+#include "qapi/qmp/qerror.h"
+#include "qapi-visit.h"
+#include "qom/object.h"
+#include "qemu/main-loop.h"
+#include "qemu/error-report.h"
+#include "trace.h"
+#include "sysemu/char.h"
+#include "qemu/iov.h"
+#include "qemu/sockets.h"
+
+#define FILTER_MIRROR(obj) \
+    OBJECT_CHECK(MirrorState, (obj), TYPE_FILTER_MIRROR)
+
+#define TYPE_FILTER_MIRROR "filter-mirror"
+
+typedef struct MirrorState {
+    NetFilterState parent_obj;
+    char *outdev;
+    CharDriverState *chr_out;
+} MirrorState;
+
+static int filter_mirror_send(NetFilterState *nf,
+                                   const struct iovec *iov,
+                                   int iovcnt)
+{
+    MirrorState *s = FILTER_MIRROR(nf);
+    int ret = 0;
+    ssize_t size = 0;
+    uint32_t len =  0;
+    char *buf;
+
+    size = iov_size(iov, iovcnt);
+    if (!size) {
+        return 0;
+    }
+
+    len = htonl(size);
+    ret = qemu_chr_fe_write_all(s->chr_out, (uint8_t *)&len, sizeof(len));
+    if (ret != sizeof(len)) {
+        goto err;
+    }
+
+    buf = g_malloc(size);
+    iov_to_buf(iov, iovcnt, 0, buf, size);
+    ret = qemu_chr_fe_write_all(s->chr_out, (uint8_t *)buf, size);
+    g_free(buf);
+    if (ret != size) {
+        goto err;
+    }
+
+    return 0;
+
+err:
+    return ret < 0 ? ret : -EIO;
+}
+
+static ssize_t filter_mirror_receive_iov(NetFilterState *nf,
+                                         NetClientState *sender,
+                                         unsigned flags,
+                                         const struct iovec *iov,
+                                         int iovcnt,
+                                         NetPacketSent *sent_cb)
+{
+    int ret;
+
+    ret = filter_mirror_send(nf, iov, iovcnt);
+    if (ret) {
+        error_report("filter_mirror_send failed(%s)", strerror(-ret));
+    }
+
+    /*
+     * we don't hope this error interrupt the normal
+     * path of net packet, so we always return zero.
+     */
+    return 0;
+}
+
+static void filter_mirror_cleanup(NetFilterState *nf)
+{
+    MirrorState *s = FILTER_MIRROR(nf);
+
+    if (s->chr_out) {
+        qemu_chr_fe_release(s->chr_out);
+    }
+}
+
+static void filter_mirror_setup(NetFilterState *nf, Error **errp)
+{
+    MirrorState *s = FILTER_MIRROR(nf);
+
+    if (!s->outdev) {
+        error_setg(errp, "filter filter mirror needs 'outdev' "
+                "property set");
+        return;
+    }
+
+    s->chr_out = qemu_chr_find(s->outdev);
+    if (s->chr_out == NULL) {
+        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
+                  "Device '%s' not found", s->outdev);
+        return;
+    }
+
+    if (qemu_chr_fe_claim(s->chr_out) != 0) {
+        error_setg(errp, QERR_DEVICE_IN_USE, s->outdev);
+        return;
+    }
+}
+
+static void filter_mirror_class_init(ObjectClass *oc, void *data)
+{
+    NetFilterClass *nfc = NETFILTER_CLASS(oc);
+
+    nfc->setup = filter_mirror_setup;
+    nfc->cleanup = filter_mirror_cleanup;
+    nfc->receive_iov = filter_mirror_receive_iov;
+}
+
+static char *filter_mirror_get_outdev(Object *obj, Error **errp)
+{
+    MirrorState *s = FILTER_MIRROR(obj);
+
+    return g_strdup(s->outdev);
+}
+
+static void
+filter_mirror_set_outdev(Object *obj, const char *value, Error **errp)
+{
+    MirrorState *s = FILTER_MIRROR(obj);
+
+    g_free(s->outdev);
+    s->outdev = g_strdup(value);
+    if (!s->outdev) {
+        error_setg(errp, "filter filter mirror needs 'outdev' "
+                "property set");
+        return;
+    }
+}
+
+static void filter_mirror_init(Object *obj)
+{
+    object_property_add_str(obj, "outdev", filter_mirror_get_outdev,
+                            filter_mirror_set_outdev, NULL);
+}
+
+static void filter_mirror_fini(Object *obj)
+{
+    MirrorState *s = FILTER_MIRROR(obj);
+
+    g_free(s->outdev);
+}
+
+static const TypeInfo filter_mirror_info = {
+    .name = TYPE_FILTER_MIRROR,
+    .parent = TYPE_NETFILTER,
+    .class_init = filter_mirror_class_init,
+    .instance_init = filter_mirror_init,
+    .instance_finalize = filter_mirror_fini,
+    .instance_size = sizeof(MirrorState),
+};
+
+static void register_types(void)
+{
+    type_register_static(&filter_mirror_info);
+}
+
+type_init(register_types);
diff --git a/qemu-options.hx b/qemu-options.hx
index 144e6a9..9922cc5 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3805,6 +3805,11 @@ queue @var{all|rx|tx} is an option that can be applied to any netfilter.
 @option{tx}: the filter is attached to the transmit queue of the netdev,
              where it will receive packets sent by the netdev.
 
+@item -object filter-mirror,id=@var{id},netdev=@var{netdevid},outdev=@var{chardevid}[,queue=@var{all|rx|tx}]
+
+filter-mirror on netdev @var{netdevid},mirror net packet to chardev
+@var{chardevid}
+
 @item -object filter-dump,id=@var{id},netdev=@var{dev},file=@var{filename}][,maxlen=@var{len}]
 
 Dump the network traffic on netdev @var{dev} to the file specified by
diff --git a/vl.c b/vl.c
index adeddd9..c09e9ab 100644
--- a/vl.c
+++ b/vl.c
@@ -2839,7 +2839,8 @@ static bool object_create_initial(const char *type)
      * they depend on netdevs already existing
      */
     if (g_str_equal(type, "filter-buffer") ||
-        g_str_equal(type, "filter-dump")) {
+        g_str_equal(type, "filter-dump") ||
+        g_str_equal(type, "filter-mirror")) {
         return false;
     }
 
-- 
2.5.0

[Qemu-devel] [PULL 06/14] tests/test-filter-mirror:add filter-mirror unit test

[Jason Wang]

From: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>

In this unit test we will test the mirror function.

start qemu with:
     -netdev socket,id=qtest-bn0,fd=sockfd
     -device e1000,netdev=qtest-bn0,id=qtest-e0
     -chardev socket,id=mirror0,path=/tmp/filter-mirror-test.sock,server,nowait
     -object filter-mirror,id=qtest-f0,netdev=qtest-bn0,queue=tx,outdev=mirror0

We inject packet through socket netdev(qtest-bn0),
filter-mirror(qtest-f0) will mirror the packet to chardev mirror0.
Then we try read packet from mirror0 and then compare it with what
we've injected.

Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 tests/.gitignore           |  1 +
 tests/Makefile             |  2 ++
 tests/test-filter-mirror.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 93 insertions(+)
 create mode 100644 tests/test-filter-mirror.c

diff --git a/tests/.gitignore b/tests/.gitignore
index 787c95c..10df017 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -63,5 +63,6 @@ test-write-threshold
 test-x86-cpuid
 test-xbzrle
 test-netfilter
+test-filter-mirror
 *-test
 qapi-schema/*.test.*
diff --git a/tests/Makefile b/tests/Makefile
index cd4bbd4..5a8f590 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -213,6 +213,7 @@ ifeq ($(CONFIG_VHOST_NET_TEST_i386),)
 check-qtest-x86_64-$(CONFIG_VHOST_NET_TEST_x86_64) += tests/vhost-user-test$(EXESUF)
 endif
 check-qtest-i386-y += tests/test-netfilter$(EXESUF)
+check-qtest-i386-y += tests/test-filter-mirror$(EXESUF)
 check-qtest-x86_64-y = $(check-qtest-i386-y)
 gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c
 gcov-files-x86_64-y = $(subst i386-softmmu/,x86_64-softmmu/,$(gcov-files-i386-y))
@@ -566,6 +567,7 @@ tests/qemu-iotests/socket_scm_helper$(EXESUF): tests/qemu-iotests/socket_scm_hel
 tests/test-qemu-opts$(EXESUF): tests/test-qemu-opts.o $(test-util-obj-y)
 tests/test-write-threshold$(EXESUF): tests/test-write-threshold.o $(test-block-obj-y)
 tests/test-netfilter$(EXESUF): tests/test-netfilter.o $(qtest-obj-y)
+tests/test-filter-mirror$(EXESUF): tests/test-filter-mirror.o $(qtest-obj-y)
 tests/ivshmem-test$(EXESUF): tests/ivshmem-test.o contrib/ivshmem-server/ivshmem-server.o $(libqos-pc-obj-y)
 tests/vhost-user-bridge$(EXESUF): tests/vhost-user-bridge.o
 
diff --git a/tests/test-filter-mirror.c b/tests/test-filter-mirror.c
new file mode 100644
index 0000000..a68dc62
--- /dev/null
+++ b/tests/test-filter-mirror.c
@@ -0,0 +1,90 @@
+/*
+ * QTest testcase for filter-mirror
+ *
+ * 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 <glib.h>
+#include "libqtest.h"
+#include "qemu/iov.h"
+#include "qemu/sockets.h"
+#include "qemu/error-report.h"
+#include "qemu/main-loop.h"
+
+static void test_mirror(void)
+{
+#ifndef _WIN32
+/* socketpair(PF_UNIX) which does not exist on windows */
+
+    int send_sock[2], recv_sock;
+    char *cmdline;
+    uint32_t ret = 0, len = 0;
+    char send_buf[] = "Hello! filter-mirror~";
+    char sock_path[] = "filter-mirror.XXXXXX";
+    char *recv_buf;
+    uint32_t size = sizeof(send_buf);
+    size = htonl(size);
+
+    ret = socketpair(PF_UNIX, SOCK_STREAM, 0, send_sock);
+    g_assert_cmpint(ret, !=, -1);
+
+    ret = mkstemp(sock_path);
+    g_assert_cmpint(ret, !=, -1);
+
+    cmdline = g_strdup_printf("-netdev socket,id=qtest-bn0,fd=%d "
+                 "-device e1000,netdev=qtest-bn0,id=qtest-e0 "
+                 "-chardev socket,id=mirror0,path=%s,server,nowait "
+                 "-object filter-mirror,id=qtest-f0,netdev=qtest-bn0,queue=tx,outdev=mirror0 "
+                 , send_sock[1], sock_path);
+    qtest_start(cmdline);
+    g_free(cmdline);
+
+    recv_sock = unix_connect(sock_path, NULL);
+    g_assert_cmpint(recv_sock, !=, -1);
+
+    struct iovec iov[] = {
+        {
+            .iov_base = &size,
+            .iov_len = sizeof(size),
+        }, {
+            .iov_base = send_buf,
+            .iov_len = sizeof(send_buf),
+        },
+    };
+    ret = iov_send(send_sock[0], iov, 2, 0, sizeof(size) + sizeof(send_buf));
+    g_assert_cmpint(ret, ==, sizeof(send_buf) + sizeof(size));
+    close(send_sock[0]);
+
+    ret = qemu_recv(recv_sock, &len, sizeof(len), 0);
+    g_assert_cmpint(ret, ==, sizeof(len));
+    len = ntohl(len);
+
+    g_assert_cmpint(len, ==, sizeof(send_buf));
+    recv_buf = g_malloc(len);
+    ret = qemu_recv(recv_sock, recv_buf, len, 0);
+    g_assert_cmpstr(recv_buf, ==, send_buf);
+
+    g_free(recv_buf);
+    close(recv_sock);
+    unlink(sock_path);
+
+#endif
+}
+
+int main(int argc, char **argv)
+{
+    int ret;
+
+    g_test_init(&argc, &argv, NULL);
+
+    qtest_add_func("/netfilter/mirror", test_mirror);
+    ret = g_test_run();
+    qtest_end();
+
+    return ret;
+}
-- 
2.5.0

[Qemu-devel] [PULL 07/14] net: netmap: probe netmap interface for virtio-net header

[Jason Wang]

From: Vincenzo Maffione <v.maffione@gmail.com>

Previous implementation of has_ufo, has_vnet_hdr, has_vnet_hdr_len, etc.
did not really probe for virtio-net header support for the netmap
interface attached to the backend. These callbacks were correct for
VALE ports, but incorrect for hardware NICs, pipes, monitors, etc.

This patch fixes the implementation to work properly with all kinds
of netmap ports.

Signed-off-by: Vincenzo Maffione <v.maffione@gmail.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 net/netmap.c | 59 ++++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 38 insertions(+), 21 deletions(-)

diff --git a/net/netmap.c b/net/netmap.c
index 9710321..1b42728 100644
--- a/net/netmap.c
+++ b/net/netmap.c
@@ -323,20 +323,47 @@ static void netmap_cleanup(NetClientState *nc)
 }
 
 /* Offloading manipulation support callbacks. */
-static bool netmap_has_ufo(NetClientState *nc)
+static int netmap_fd_set_vnet_hdr_len(NetmapState *s, int len)
 {
-    return true;
+    struct nmreq req;
+
+    /* Issue a NETMAP_BDG_VNET_HDR command to change the virtio-net header
+     * length for the netmap adapter associated to 's->ifname'.
+     */
+    memset(&req, 0, sizeof(req));
+    pstrcpy(req.nr_name, sizeof(req.nr_name), s->ifname);
+    req.nr_version = NETMAP_API;
+    req.nr_cmd = NETMAP_BDG_VNET_HDR;
+    req.nr_arg1 = len;
+
+    return ioctl(s->nmd->fd, NIOCREGIF, &req);
 }
 
-static bool netmap_has_vnet_hdr(NetClientState *nc)
+static bool netmap_has_vnet_hdr_len(NetClientState *nc, int len)
 {
+    NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
+    int prev_len = s->vnet_hdr_len;
+
+    /* Check that we can set the new length. */
+    if (netmap_fd_set_vnet_hdr_len(s, len)) {
+        return false;
+    }
+
+    /* Restore the previous length. */
+    if (netmap_fd_set_vnet_hdr_len(s, prev_len)) {
+        error_report("Failed to restore vnet-hdr length %d on %s: %s",
+                     prev_len, s->ifname, strerror(errno));
+        abort();
+    }
+
     return true;
 }
 
-static bool netmap_has_vnet_hdr_len(NetClientState *nc, int len)
+/* A netmap interface that supports virtio-net headers always
+ * supports UFO, so we use this callback also for the has_ufo hook. */
+static bool netmap_has_vnet_hdr(NetClientState *nc)
 {
-    return len == 0 || len == sizeof(struct virtio_net_hdr) ||
-                len == sizeof(struct virtio_net_hdr_mrg_rxbuf);
+    return netmap_has_vnet_hdr_len(nc, sizeof(struct virtio_net_hdr));
 }
 
 static void netmap_using_vnet_hdr(NetClientState *nc, bool enable)
@@ -347,20 +374,11 @@ static void netmap_set_vnet_hdr_len(NetClientState *nc, int len)
 {
     NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
     int err;
-    struct nmreq req;
 
-    /* Issue a NETMAP_BDG_VNET_HDR command to change the virtio-net header
-     * length for the netmap adapter associated to 's->ifname'.
-     */
-    memset(&req, 0, sizeof(req));
-    pstrcpy(req.nr_name, sizeof(req.nr_name), s->ifname);
-    req.nr_version = NETMAP_API;
-    req.nr_cmd = NETMAP_BDG_VNET_HDR;
-    req.nr_arg1 = len;
-    err = ioctl(s->nmd->fd, NIOCREGIF, &req);
+    err = netmap_fd_set_vnet_hdr_len(s, len);
     if (err) {
-        error_report("Unable to execute NETMAP_BDG_VNET_HDR on %s: %s",
-                     s->ifname, strerror(errno));
+        error_report("Unable to set vnet-hdr length %d on %s: %s",
+                     len, s->ifname, strerror(errno));
     } else {
         /* Keep track of the current length. */
         s->vnet_hdr_len = len;
@@ -373,8 +391,7 @@ static void netmap_set_offload(NetClientState *nc, int csum, int tso4, int tso6,
     NetmapState *s = DO_UPCAST(NetmapState, nc, nc);
 
     /* Setting a virtio-net header length greater than zero automatically
-     * enables the offloadings.
-     */
+     * enables the offloadings. */
     if (!s->vnet_hdr_len) {
         netmap_set_vnet_hdr_len(nc, sizeof(struct virtio_net_hdr));
     }
@@ -388,7 +405,7 @@ static NetClientInfo net_netmap_info = {
     .receive_iov = netmap_receive_iov,
     .poll = netmap_poll,
     .cleanup = netmap_cleanup,
-    .has_ufo = netmap_has_ufo,
+    .has_ufo = netmap_has_vnet_hdr,
     .has_vnet_hdr = netmap_has_vnet_hdr,
     .has_vnet_hdr_len = netmap_has_vnet_hdr_len,
     .using_vnet_hdr = netmap_using_vnet_hdr,
-- 
2.5.0

[Qemu-devel] [PULL 08/14] rocker: forbid to change world type

[Jason Wang]

From: Jiri Pirko <jiri@mellanox.com>

Port to world assignment should be permitted only by qemu user. Driver
should not be able to do it, so forbid that possibility.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/net/rocker/rocker.c    | 8 +++++++-
 hw/net/rocker/rocker_fp.c | 5 +++++
 hw/net/rocker/rocker_fp.h | 1 +
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
index f3e994d..a1d921d 100644
--- a/hw/net/rocker/rocker.c
+++ b/hw/net/rocker/rocker.c
@@ -400,7 +400,13 @@ static int cmd_set_port_settings(Rocker *r,
 
     if (tlvs[ROCKER_TLV_CMD_PORT_SETTINGS_MODE]) {
         mode = rocker_tlv_get_u8(tlvs[ROCKER_TLV_CMD_PORT_SETTINGS_MODE]);
-        fp_port_set_world(fp_port, r->worlds[mode]);
+        if (mode >= ROCKER_WORLD_TYPE_MAX) {
+            return -ROCKER_EINVAL;
+        }
+        /* We don't support world change. */
+        if (!fp_port_check_world(fp_port, r->worlds[mode])) {
+            return -ROCKER_EINVAL;
+        }
     }
 
     if (tlvs[ROCKER_TLV_CMD_PORT_SETTINGS_LEARNING]) {
diff --git a/hw/net/rocker/rocker_fp.c b/hw/net/rocker/rocker_fp.c
index af37fef..0149899 100644
--- a/hw/net/rocker/rocker_fp.c
+++ b/hw/net/rocker/rocker_fp.c
@@ -186,6 +186,11 @@ void fp_port_set_world(FpPort *port, World *world)
     port->world = world;
 }
 
+bool fp_port_check_world(FpPort *port, World *world)
+{
+    return port->world == world;
+}
+
 bool fp_port_enabled(FpPort *port)
 {
     return port->enabled;
diff --git a/hw/net/rocker/rocker_fp.h b/hw/net/rocker/rocker_fp.h
index ab80fd8..04592bb 100644
--- a/hw/net/rocker/rocker_fp.h
+++ b/hw/net/rocker/rocker_fp.h
@@ -40,6 +40,7 @@ int fp_port_set_settings(FpPort *port, uint32_t speed,
 bool fp_port_from_pport(uint32_t pport, uint32_t *port);
 World *fp_port_get_world(FpPort *port);
 void fp_port_set_world(FpPort *port, World *world);
+bool fp_port_check_world(FpPort *port, World *world);
 bool fp_port_enabled(FpPort *port);
 void fp_port_enable(FpPort *port);
 void fp_port_disable(FpPort *port);
-- 
2.5.0

[Qemu-devel] [PULL 09/14] rocker: return -ENOMEM in case of some world alloc fails

[Jason Wang]

From: Jiri Pirko <jiri@mellanox.com>

Until now, 0 is returned in this error case. Fix it ro return -ENOMEM.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/net/rocker/rocker.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
index a1d921d..104c097 100644
--- a/hw/net/rocker/rocker.c
+++ b/hw/net/rocker/rocker.c
@@ -1301,6 +1301,7 @@ static int pci_rocker_init(PCIDevice *dev)
 
     for (i = 0; i < ROCKER_WORLD_TYPE_MAX; i++) {
         if (!r->worlds[i]) {
+            err = -ENOMEM;
             goto err_world_alloc;
         }
     }
-- 
2.5.0

[Qemu-devel] [PULL 10/14] rocker: add name field into WorldOps ale let world specify its name

[Jason Wang]

From: Jiri Pirko <jiri@mellanox.com>

Also use this in world_name getter function.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/net/rocker/rocker_of_dpa.c | 1 +
 hw/net/rocker/rocker_world.c  | 7 +------
 hw/net/rocker/rocker_world.h  | 1 +
 3 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/hw/net/rocker/rocker_of_dpa.c b/hw/net/rocker/rocker_of_dpa.c
index da3fc54..0a134eb 100644
--- a/hw/net/rocker/rocker_of_dpa.c
+++ b/hw/net/rocker/rocker_of_dpa.c
@@ -2614,6 +2614,7 @@ RockerOfDpaGroupList *qmp_query_rocker_of_dpa_groups(const char *name,
 }
 
 static WorldOps of_dpa_ops = {
+    .name = "ofdpa",
     .init = of_dpa_init,
     .uninit = of_dpa_uninit,
     .ig = of_dpa_ig,
diff --git a/hw/net/rocker/rocker_world.c b/hw/net/rocker/rocker_world.c
index 1ed0fcd..89777e9 100644
--- a/hw/net/rocker/rocker_world.c
+++ b/hw/net/rocker/rocker_world.c
@@ -98,10 +98,5 @@ enum rocker_world_type world_type(World *world)
 
 const char *world_name(World *world)
 {
-    switch (world->type) {
-    case ROCKER_WORLD_TYPE_OF_DPA:
-        return "OF_DPA";
-    default:
-        return "unknown";
-    }
+    return world->ops->name;
 }
diff --git a/hw/net/rocker/rocker_world.h b/hw/net/rocker/rocker_world.h
index 18d277b..58ade47 100644
--- a/hw/net/rocker/rocker_world.h
+++ b/hw/net/rocker/rocker_world.h
@@ -33,6 +33,7 @@ typedef int (world_cmd)(World *world, DescInfo *info,
                         RockerTlv *cmd_info_tlv);
 
 typedef struct world_ops {
+    const char *name;
     world_init *init;
     world_uninit *uninit;
     world_ig *ig;
-- 
2.5.0

[Qemu-devel] [PULL 11/14] rocker: allow user to specify rocker world by property

[Jason Wang]

From: Jiri Pirko <jiri@mellanox.com>

Add property to specify rocker world. All ports will be assigned to this
world.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/net/rocker/rocker.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/hw/net/rocker/rocker.c b/hw/net/rocker/rocker.c
index 104c097..30f2ce4 100644
--- a/hw/net/rocker/rocker.c
+++ b/hw/net/rocker/rocker.c
@@ -43,6 +43,7 @@ struct rocker {
 
     /* switch configuration */
     char *name;                  /* switch name */
+    char *world_name;            /* world name */
     uint32_t fp_ports;           /* front-panel port count */
     NICPeers *fp_ports_peers;
     MACAddr fp_start_macaddr;    /* front-panel port 0 mac addr */
@@ -1286,6 +1287,18 @@ static void rocker_msix_uninit(Rocker *r)
     rocker_msix_vectors_unuse(r, ROCKER_MSIX_VEC_COUNT(r->fp_ports));
 }
 
+static World *rocker_world_type_by_name(Rocker *r, const char *name)
+{
+    int i;
+
+    for (i = 0; i < ROCKER_WORLD_TYPE_MAX; i++) {
+        if (strcmp(name, world_name(r->worlds[i])) == 0) {
+            return r->worlds[i];
+	}
+    }
+    return NULL;
+}
+
 static int pci_rocker_init(PCIDevice *dev)
 {
     Rocker *r = to_rocker(dev);
@@ -1297,7 +1310,6 @@ static int pci_rocker_init(PCIDevice *dev)
     /* allocate worlds */
 
     r->worlds[ROCKER_WORLD_TYPE_OF_DPA] = of_dpa_world_alloc(r);
-    r->world_dflt = r->worlds[ROCKER_WORLD_TYPE_OF_DPA];
 
     for (i = 0; i < ROCKER_WORLD_TYPE_MAX; i++) {
         if (!r->worlds[i]) {
@@ -1306,6 +1318,19 @@ static int pci_rocker_init(PCIDevice *dev)
         }
     }
 
+    if (!r->world_name) {
+        r->world_name = g_strdup(world_name(r->worlds[ROCKER_WORLD_TYPE_OF_DPA]));
+    }
+
+    r->world_dflt = rocker_world_type_by_name(r, r->world_name);
+    if (!r->world_dflt) {
+        fprintf(stderr,
+                "rocker: requested world \"%s\" does not exist\n",
+                r->world_name);
+        err = -EINVAL;
+        goto err_world_type_by_name;
+    }
+
     /* set up memory-mapped region at BAR0 */
 
     memory_region_init_io(&r->mmio, OBJECT(r), &rocker_mmio_ops, r,
@@ -1439,6 +1464,7 @@ err_duplicate:
 err_msix_init:
     object_unparent(OBJECT(&r->msix_bar));
     object_unparent(OBJECT(&r->mmio));
+err_world_type_by_name:
 err_world_alloc:
     for (i = 0; i < ROCKER_WORLD_TYPE_MAX; i++) {
         if (r->worlds[i]) {
@@ -1510,6 +1536,7 @@ static void rocker_reset(DeviceState *dev)
 
 static Property rocker_properties[] = {
     DEFINE_PROP_STRING("name", Rocker, name),
+    DEFINE_PROP_STRING("world", Rocker, world_name),
     DEFINE_PROP_MACADDR("fp_start_macaddr", Rocker,
                         fp_start_macaddr),
     DEFINE_PROP_UINT64("switch_id", Rocker,
-- 
2.5.0

[Qemu-devel] [PULL 12/14] filter: Add 'status' property for filter object

[Jason Wang]

From: zhanghailiang <zhang.zhanghailiang@huawei.com>

With this property, users can control if this filter is 'on'
or 'off'. The default behavior for filter is 'on'.

For some types of filters, they may need to react to status changing,
So here, we introduced status changing callback/notifier for filter class.

We will skip the disabled ('off') filter when delivering packets in net layer.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Yang Hongyang <hongyang.yang@easystack.cn>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 include/net/filter.h |  4 ++++
 net/filter.c         | 41 +++++++++++++++++++++++++++++++++++++++++
 qemu-options.hx      |  4 +++-
 3 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/include/net/filter.h b/include/net/filter.h
index 5639976..cfb1172 100644
--- a/include/net/filter.h
+++ b/include/net/filter.h
@@ -36,12 +36,15 @@ typedef ssize_t (FilterReceiveIOV)(NetFilterState *nc,
                                    int iovcnt,
                                    NetPacketSent *sent_cb);
 
+typedef void (FilterStatusChanged) (NetFilterState *nf, Error **errp);
+
 typedef struct NetFilterClass {
     ObjectClass parent_class;
 
     /* optional */
     FilterSetup *setup;
     FilterCleanup *cleanup;
+    FilterStatusChanged *status_changed;
     /* mandatory */
     FilterReceiveIOV *receive_iov;
 } NetFilterClass;
@@ -55,6 +58,7 @@ struct NetFilterState {
     char *netdev_id;
     NetClientState *netdev;
     NetFilterDirection direction;
+    bool on;
     QTAILQ_ENTRY(NetFilterState) next;
 };
 
diff --git a/net/filter.c b/net/filter.c
index 7cdbc6c..a08ef68 100644
--- a/net/filter.c
+++ b/net/filter.c
@@ -17,6 +17,11 @@
 #include "qom/object_interfaces.h"
 #include "qemu/iov.h"
 
+static inline bool qemu_can_skip_netfilter(NetFilterState *nf)
+{
+    return !nf->on;
+}
+
 ssize_t qemu_netfilter_receive(NetFilterState *nf,
                                NetFilterDirection direction,
                                NetClientState *sender,
@@ -25,6 +30,9 @@ ssize_t qemu_netfilter_receive(NetFilterState *nf,
                                int iovcnt,
                                NetPacketSent *sent_cb)
 {
+    if (qemu_can_skip_netfilter(nf)) {
+        return 0;
+    }
     if (nf->direction == direction ||
         nf->direction == NET_FILTER_DIRECTION_ALL) {
         return NETFILTER_GET_CLASS(OBJECT(nf))->receive_iov(
@@ -134,8 +142,38 @@ static void netfilter_set_direction(Object *obj, int direction, Error **errp)
     nf->direction = direction;
 }
 
+static char *netfilter_get_status(Object *obj, Error **errp)
+{
+    NetFilterState *nf = NETFILTER(obj);
+
+    return nf->on ? g_strdup("on") : g_strdup("off");
+}
+
+static void netfilter_set_status(Object *obj, const char *str, Error **errp)
+{
+    NetFilterState *nf = NETFILTER(obj);
+    NetFilterClass *nfc = NETFILTER_GET_CLASS(obj);
+
+    if (strcmp(str, "on") && strcmp(str, "off")) {
+        error_setg(errp, "Invalid value for netfilter status, "
+                         "should be 'on' or 'off'");
+        return;
+    }
+    if (nf->on == !strcmp(str, "on")) {
+        return;
+    }
+    nf->on = !nf->on;
+    if (nfc->status_changed) {
+        nfc->status_changed(nf, errp);
+    }
+}
+
 static void netfilter_init(Object *obj)
 {
+    NetFilterState *nf = NETFILTER(obj);
+
+    nf->on = true;
+
     object_property_add_str(obj, "netdev",
                             netfilter_get_netdev_id, netfilter_set_netdev_id,
                             NULL);
@@ -143,6 +181,9 @@ static void netfilter_init(Object *obj)
                              NetFilterDirection_lookup,
                              netfilter_get_direction, netfilter_set_direction,
                              NULL);
+    object_property_add_str(obj, "status",
+                            netfilter_get_status, netfilter_set_status,
+                            NULL);
 }
 
 static void netfilter_complete(UserCreatable *uc, Error **errp)
diff --git a/qemu-options.hx b/qemu-options.hx
index 9922cc5..9bf09ed 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3788,11 +3788,13 @@ version by providing the @var{passwordid} parameter. This provides
 the ID of a previously created @code{secret} object containing the
 password for decryption.
 
-@item -object filter-buffer,id=@var{id},netdev=@var{netdevid},interval=@var{t}[,queue=@var{all|rx|tx}]
+@item -object filter-buffer,id=@var{id},netdev=@var{netdevid},interval=@var{t}[,queue=@var{all|rx|tx}][,status=@var{on|off}]
 
 Interval @var{t} can't be 0, this filter batches the packet delivery: all
 packets arriving in a given interval on netdev @var{netdevid} are delayed
 until the end of the interval. Interval is in microseconds.
+@option{status} is optional that indicate whether the netfilter is
+on (enabled) or off (disabled), the default status for netfilter will be 'on'.
 
 queue @var{all|rx|tx} is an option that can be applied to any netfilter.
 
-- 
2.5.0

[Qemu-devel] [PULL 13/14] filter-buffer: Add status_changed callback processing

[Jason Wang]

From: zhanghailiang <zhang.zhanghailiang@huawei.com>

While the status of filter-buffer changing from 'on' to 'off',
it need to release all the buffered packets, and delete the related
timer, while switch from 'off' to 'on', it need to resume the release
packets timer.

Here, we extract the process of setup timer into a new helper,
which will be used in the new status_changed callback.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Yang Hongyang <hongyang.yang@easystack.cn>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 net/filter-buffer.c | 34 ++++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/net/filter-buffer.c b/net/filter-buffer.c
index 12ad2e3..972177b 100644
--- a/net/filter-buffer.c
+++ b/net/filter-buffer.c
@@ -100,6 +100,19 @@ static void filter_buffer_cleanup(NetFilterState *nf)
     }
 }
 
+static void filter_buffer_setup_timer(NetFilterState *nf)
+{
+    FilterBufferState *s = FILTER_BUFFER(nf);
+
+    if (s->interval) {
+        timer_init_us(&s->release_timer, QEMU_CLOCK_VIRTUAL,
+                      filter_buffer_release_timer, nf);
+        /* Timer armed to fire in s->interval microseconds. */
+        timer_mod(&s->release_timer,
+                  qemu_clock_get_us(QEMU_CLOCK_VIRTUAL) + s->interval);
+    }
+}
+
 static void filter_buffer_setup(NetFilterState *nf, Error **errp)
 {
     FilterBufferState *s = FILTER_BUFFER(nf);
@@ -115,12 +128,20 @@ static void filter_buffer_setup(NetFilterState *nf, Error **errp)
     }
 
     s->incoming_queue = qemu_new_net_queue(qemu_netfilter_pass_to_next, nf);
-    if (s->interval) {
-        timer_init_us(&s->release_timer, QEMU_CLOCK_VIRTUAL,
-                      filter_buffer_release_timer, nf);
-        /* Timer armed to fire in s->interval microseconds. */
-        timer_mod(&s->release_timer,
-                  qemu_clock_get_us(QEMU_CLOCK_VIRTUAL) + s->interval);
+    filter_buffer_setup_timer(nf);
+}
+
+static void filter_buffer_status_changed(NetFilterState *nf, Error **errp)
+{
+    FilterBufferState *s = FILTER_BUFFER(nf);
+
+    if (!nf->on) {
+        if (s->interval) {
+            timer_del(&s->release_timer);
+        }
+        filter_buffer_flush(nf);
+    } else {
+        filter_buffer_setup_timer(nf);
     }
 }
 
@@ -131,6 +152,7 @@ static void filter_buffer_class_init(ObjectClass *oc, void *data)
     nfc->setup = filter_buffer_setup;
     nfc->cleanup = filter_buffer_cleanup;
     nfc->receive_iov = filter_buffer_receive_iov;
+    nfc->status_changed = filter_buffer_status_changed;
 }
 
 static void filter_buffer_get_interval(Object *obj, Visitor *v,
-- 
2.5.0

[Qemu-devel] [PULL 14/14] net: check packet payload length

[Jason Wang]

From: Prasad J Pandit <pjp@fedoraproject.org>

While computing IP checksum, 'net_checksum_calculate' reads
payload length from the packet. It could exceed the given 'data'
buffer size. Add a check to avoid it.

Reported-by: Liu Ling <liuling-it@360.cn>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 net/checksum.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/checksum.c b/net/checksum.c
index b5016ab..d0fa424 100644
--- a/net/checksum.c
+++ b/net/checksum.c
@@ -60,6 +60,11 @@ void net_checksum_calculate(uint8_t *data, int length)
     int hlen, plen, proto, csum_offset;
     uint16_t csum;
 
+    /* Ensure data has complete L2 & L3 headers. */
+    if (length < 14 + 20) {
+        return;
+    }
+
     if ((data[14] & 0xf0) != 0x40)
 	return; /* not IPv4 */
     hlen  = (data[14] & 0x0f) * 4;
@@ -77,8 +82,9 @@ void net_checksum_calculate(uint8_t *data, int length)
 	return;
     }
 
-    if (plen < csum_offset+2)
-	return;
+    if (plen < csum_offset + 2 || 14 + hlen + plen > length) {
+        return;
+    }
 
     data[14+hlen+csum_offset]   = 0;
     data[14+hlen+csum_offset+1] = 0;
-- 
2.5.0

Re: [Qemu-devel] [PULL 00/14] Net patches

[Peter Maydell]

On 7 March 2016 at 10:12, Jason Wang <jasowang@redhat.com> wrote:
> The following changes since commit 1464ad45cd6cdeb0b5c1a54d3d3791396e47e52f:
>
>   Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2016-03-04' into staging (2016-03-06 11:53:27 +0000)
>
> are available in the git repository at:
>
>   https://github.com/jasowang/qemu.git tags/net-pull-request
>
> for you to fetch changes up to a2f2e45c6edbba9e1961056fa77c696208b40c8e:
>
>   net: check packet payload length (2016-03-07 10:15:48 +0800)
>
> ----------------------------------------------------------------
>
> - a new netfilter implementation: mirror
> - netfilter could be disabled and enabled through qom-set now
> - fix netfilter crash when specifiying wrong parameters
> - rocker switch now can allow user to specifiy world
> - fix OOB access for ne2000

Hi; I'm afraid this makes "make check" hang for me (Linux, x86-64):

TEST: tests/test-netfilter... (pid=26854)
  /i386/netfilter/addremove_one:                                       OK
  /i386/netfilter/remove_netdev_one:                                   OK
  /i386/netfilter/addremove_multi:                                     OK
  /i386/netfilter/remove_netdev_multi:                                 OK
PASS: tests/test-netfilter
TEST: tests/test-filter-mirror... (pid=26858)
  /i386/netfilter/mirror:

(consistently, every time I run make check, on the same test).

thanks
-- PMM

Re: [Qemu-devel] [PULL 00/14] Net patches

[Jason Wang]

On 03/08/2016 12:51 PM, Peter Maydell wrote:
> On 7 March 2016 at 10:12, Jason Wang <jasowang@redhat.com> wrote:
>> The following changes since commit 1464ad45cd6cdeb0b5c1a54d3d3791396e47e52f:
>>
>>   Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2016-03-04' into staging (2016-03-06 11:53:27 +0000)
>>
>> are available in the git repository at:
>>
>>   https://github.com/jasowang/qemu.git tags/net-pull-request
>>
>> for you to fetch changes up to a2f2e45c6edbba9e1961056fa77c696208b40c8e:
>>
>>   net: check packet payload length (2016-03-07 10:15:48 +0800)
>>
>> ----------------------------------------------------------------
>>
>> - a new netfilter implementation: mirror
>> - netfilter could be disabled and enabled through qom-set now
>> - fix netfilter crash when specifiying wrong parameters
>> - rocker switch now can allow user to specifiy world
>> - fix OOB access for ne2000
> Hi; I'm afraid this makes "make check" hang for me (Linux, x86-64):
>
> TEST: tests/test-netfilter... (pid=26854)
>   /i386/netfilter/addremove_one:                                       OK
>   /i386/netfilter/remove_netdev_one:                                   OK
>   /i386/netfilter/addremove_multi:                                     OK
>   /i386/netfilter/remove_netdev_multi:                                 OK
> PASS: tests/test-netfilter
> TEST: tests/test-filter-mirror... (pid=26858)
>   /i386/netfilter/mirror:
>
> (consistently, every time I run make check, on the same test).
>
> thanks
> -- PMM

Sorry, it manages to pass on my machine before submitting the pull
request. But when I re-try this several times, it fails.

This probably means we have bug in mirror implementation. Chen and
Congyang, please try to fix this bug and resubmit a new version of the
patch.

Will drop mirror from this pull request and submit a V2.

Thanks

Re: [Qemu-devel] [PULL 00/14] Net patches

[Wen Congyang]

On 03/08/2016 03:33 PM, Jason Wang wrote:
> 
> 
> On 03/08/2016 12:51 PM, Peter Maydell wrote:
>> On 7 March 2016 at 10:12, Jason Wang <jasowang@redhat.com> wrote:
>>> The following changes since commit 1464ad45cd6cdeb0b5c1a54d3d3791396e47e52f:
>>>
>>>   Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2016-03-04' into staging (2016-03-06 11:53:27 +0000)
>>>
>>> are available in the git repository at:
>>>
>>>   https://github.com/jasowang/qemu.git tags/net-pull-request
>>>
>>> for you to fetch changes up to a2f2e45c6edbba9e1961056fa77c696208b40c8e:
>>>
>>>   net: check packet payload length (2016-03-07 10:15:48 +0800)
>>>
>>> ----------------------------------------------------------------
>>>
>>> - a new netfilter implementation: mirror
>>> - netfilter could be disabled and enabled through qom-set now
>>> - fix netfilter crash when specifiying wrong parameters
>>> - rocker switch now can allow user to specifiy world
>>> - fix OOB access for ne2000
>> Hi; I'm afraid this makes "make check" hang for me (Linux, x86-64):
>>
>> TEST: tests/test-netfilter... (pid=26854)
>>   /i386/netfilter/addremove_one:                                       OK
>>   /i386/netfilter/remove_netdev_one:                                   OK
>>   /i386/netfilter/addremove_multi:                                     OK
>>   /i386/netfilter/remove_netdev_multi:                                 OK
>> PASS: tests/test-netfilter
>> TEST: tests/test-filter-mirror... (pid=26858)
>>   /i386/netfilter/mirror:
>>
>> (consistently, every time I run make check, on the same test).
>>
>> thanks
>> -- PMM
> 
> Sorry, it manages to pass on my machine before submitting the pull
> request. But when I re-try this several times, it fails.
> 
> This probably means we have bug in mirror implementation. Chen and
> Congyang, please try to fix this bug and resubmit a new version of the
> patch.
> 
> Will drop mirror from this pull request and submit a V2.

OK. what is the version of the kernel that you use?

Thanks
Wen Congyang

> 
> Thanks
> 
> 
> .
> 

Re: [Qemu-devel] [PULL 00/14] Net patches

[Jason Wang]

On 03/08/2016 03:50 PM, Wen Congyang wrote:
> On 03/08/2016 03:33 PM, Jason Wang wrote:
>>
>> On 03/08/2016 12:51 PM, Peter Maydell wrote:
>>> On 7 March 2016 at 10:12, Jason Wang <jasowang@redhat.com> wrote:
>>>> The following changes since commit 1464ad45cd6cdeb0b5c1a54d3d3791396e47e52f:
>>>>
>>>>   Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2016-03-04' into staging (2016-03-06 11:53:27 +0000)
>>>>
>>>> are available in the git repository at:
>>>>
>>>>   https://github.com/jasowang/qemu.git tags/net-pull-request
>>>>
>>>> for you to fetch changes up to a2f2e45c6edbba9e1961056fa77c696208b40c8e:
>>>>
>>>>   net: check packet payload length (2016-03-07 10:15:48 +0800)
>>>>
>>>> ----------------------------------------------------------------
>>>>
>>>> - a new netfilter implementation: mirror
>>>> - netfilter could be disabled and enabled through qom-set now
>>>> - fix netfilter crash when specifiying wrong parameters
>>>> - rocker switch now can allow user to specifiy world
>>>> - fix OOB access for ne2000
>>> Hi; I'm afraid this makes "make check" hang for me (Linux, x86-64):
>>>
>>> TEST: tests/test-netfilter... (pid=26854)
>>>   /i386/netfilter/addremove_one:                                       OK
>>>   /i386/netfilter/remove_netdev_one:                                   OK
>>>   /i386/netfilter/addremove_multi:                                     OK
>>>   /i386/netfilter/remove_netdev_multi:                                 OK
>>> PASS: tests/test-netfilter
>>> TEST: tests/test-filter-mirror... (pid=26858)
>>>   /i386/netfilter/mirror:
>>>
>>> (consistently, every time I run make check, on the same test).
>>>
>>> thanks
>>> -- PMM
>> Sorry, it manages to pass on my machine before submitting the pull
>> request. But when I re-try this several times, it fails.
>>
>> This probably means we have bug in mirror implementation. Chen and
>> Congyang, please try to fix this bug and resubmit a new version of the
>> patch.
>>
>> Will drop mirror from this pull request and submit a V2.
> OK. what is the version of the kernel that you use?

4.2 but probably unrelated. Gdb shows the test wait at recv().

>
> Thanks
> Wen Congyang
>
>> Thanks
>>
>>
>> .
>>
>
>

Re: [Qemu-devel] [PULL 00/14] Net patches

[Zhang Chen]

On 03/08/2016 03:56 PM, Jason Wang wrote:
>
> On 03/08/2016 03:50 PM, Wen Congyang wrote:
>> On 03/08/2016 03:33 PM, Jason Wang wrote:
>>> On 03/08/2016 12:51 PM, Peter Maydell wrote:
>>>> On 7 March 2016 at 10:12, Jason Wang <jasowang@redhat.com> wrote:
>>>>> The following changes since commit 1464ad45cd6cdeb0b5c1a54d3d3791396e47e52f:
>>>>>
>>>>>    Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2016-03-04' into staging (2016-03-06 11:53:27 +0000)
>>>>>
>>>>> are available in the git repository at:
>>>>>
>>>>>    https://github.com/jasowang/qemu.git tags/net-pull-request
>>>>>
>>>>> for you to fetch changes up to a2f2e45c6edbba9e1961056fa77c696208b40c8e:
>>>>>
>>>>>    net: check packet payload length (2016-03-07 10:15:48 +0800)
>>>>>
>>>>> ----------------------------------------------------------------
>>>>>
>>>>> - a new netfilter implementation: mirror
>>>>> - netfilter could be disabled and enabled through qom-set now
>>>>> - fix netfilter crash when specifiying wrong parameters
>>>>> - rocker switch now can allow user to specifiy world
>>>>> - fix OOB access for ne2000
>>>> Hi; I'm afraid this makes "make check" hang for me (Linux, x86-64):
>>>>
>>>> TEST: tests/test-netfilter... (pid=26854)
>>>>    /i386/netfilter/addremove_one:                                       OK
>>>>    /i386/netfilter/remove_netdev_one:                                   OK
>>>>    /i386/netfilter/addremove_multi:                                     OK
>>>>    /i386/netfilter/remove_netdev_multi:                                 OK
>>>> PASS: tests/test-netfilter
>>>> TEST: tests/test-filter-mirror... (pid=26858)
>>>>    /i386/netfilter/mirror:
>>>>
>>>> (consistently, every time I run make check, on the same test).
>>>>
>>>> thanks
>>>> -- PMM
>>> Sorry, it manages to pass on my machine before submitting the pull
>>> request. But when I re-try this several times, it fails.
>>>
>>> This probably means we have bug in mirror implementation. Chen and
>>> Congyang, please try to fix this bug and resubmit a new version of the
>>> patch.
>>>
>>> Will drop mirror from this pull request and submit a V2.
>> OK. what is the version of the kernel that you use?
> 4.2 but probably unrelated. Gdb shows the test wait at recv().

Hi~ Jason.

I found the reason for this problem is that
unix_connect() have not connect to sock_path before iov_send().
It need time to establish connection. so can we fix it with usleep()
like this:

     recv_sock = unix_connect(sock_path, NULL);
     g_assert_cmpint(recv_sock, !=, -1);
+    usleep(1000);

     ret = iov_send(send_sock[0], iov, 2, 0, sizeof(size) + 
sizeof(send_buf));
     g_assert_cmpint(ret, ==, sizeof(send_buf) + sizeof(size));
     close(send_sock[0]);

     ret = qemu_recv(recv_sock, &len, sizeof(len), 0);



>> Thanks
>> Wen Congyang
>>
>>> Thanks
>>>
>>>
>>> .
>>>
>>
>
>
> .
>

-- 
Thanks
zhangchen

Re: [Qemu-devel] [PULL 00/14] Net patches

[Wen Congyang]

On 03/08/2016 05:06 PM, Zhang Chen wrote:
> 
> 
> On 03/08/2016 03:56 PM, Jason Wang wrote:
>>
>> On 03/08/2016 03:50 PM, Wen Congyang wrote:
>>> On 03/08/2016 03:33 PM, Jason Wang wrote:
>>>> On 03/08/2016 12:51 PM, Peter Maydell wrote:
>>>>> On 7 March 2016 at 10:12, Jason Wang <jasowang@redhat.com> wrote:
>>>>>> The following changes since commit 1464ad45cd6cdeb0b5c1a54d3d3791396e47e52f:
>>>>>>
>>>>>>    Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2016-03-04' into staging (2016-03-06 11:53:27 +0000)
>>>>>>
>>>>>> are available in the git repository at:
>>>>>>
>>>>>>    https://github.com/jasowang/qemu.git tags/net-pull-request
>>>>>>
>>>>>> for you to fetch changes up to a2f2e45c6edbba9e1961056fa77c696208b40c8e:
>>>>>>
>>>>>>    net: check packet payload length (2016-03-07 10:15:48 +0800)
>>>>>>
>>>>>> ----------------------------------------------------------------
>>>>>>
>>>>>> - a new netfilter implementation: mirror
>>>>>> - netfilter could be disabled and enabled through qom-set now
>>>>>> - fix netfilter crash when specifiying wrong parameters
>>>>>> - rocker switch now can allow user to specifiy world
>>>>>> - fix OOB access for ne2000
>>>>> Hi; I'm afraid this makes "make check" hang for me (Linux, x86-64):
>>>>>
>>>>> TEST: tests/test-netfilter... (pid=26854)
>>>>>    /i386/netfilter/addremove_one:                                       OK
>>>>>    /i386/netfilter/remove_netdev_one:                                   OK
>>>>>    /i386/netfilter/addremove_multi:                                     OK
>>>>>    /i386/netfilter/remove_netdev_multi:                                 OK
>>>>> PASS: tests/test-netfilter
>>>>> TEST: tests/test-filter-mirror... (pid=26858)
>>>>>    /i386/netfilter/mirror:
>>>>>
>>>>> (consistently, every time I run make check, on the same test).
>>>>>
>>>>> thanks
>>>>> -- PMM
>>>> Sorry, it manages to pass on my machine before submitting the pull
>>>> request. But when I re-try this several times, it fails.
>>>>
>>>> This probably means we have bug in mirror implementation. Chen and
>>>> Congyang, please try to fix this bug and resubmit a new version of the
>>>> patch.
>>>>
>>>> Will drop mirror from this pull request and submit a V2.
>>> OK. what is the version of the kernel that you use?
>> 4.2 but probably unrelated. Gdb shows the test wait at recv().
> 
> Hi~ Jason.
> 
> I found the reason for this problem is that
> unix_connect() have not connect to sock_path before iov_send().

After unix_connect() returns, the connection is established.
qemu char device will call qemu_chr_accept() after the connection
is established. If we send data before qemu_chr_accept() is called,
the data will be dropped by qemu char device:
static int tcp_chr_write(CharDriverState *chr, const uint8_t *buf, int len)         
{                                                                               
    TCPCharDriver *s = chr->opaque;                                             
    if (s->connected) {                                                         
...
        return ret;                                                             
    } else {                                                                    
        /* XXX: indicate an error ? */                                          
        return len;                                                             
    }                                                                           
}                              

We should wait some to let qemu_chr_accept() is called before sending
data.

Thanks
Wen Congyang

> It need time to establish connection. so can we fix it with usleep()
> like this:
> 
>     recv_sock = unix_connect(sock_path, NULL);
>     g_assert_cmpint(recv_sock, !=, -1);
> +    usleep(1000);
> 
>     ret = iov_send(send_sock[0], iov, 2, 0, sizeof(size) + sizeof(send_buf));
>     g_assert_cmpint(ret, ==, sizeof(send_buf) + sizeof(size));
>     close(send_sock[0]);
> 
>     ret = qemu_recv(recv_sock, &len, sizeof(len), 0);
> 
> 
> 
>>> Thanks
>>> Wen Congyang
>>>
>>>> Thanks
>>>>
>>>>
>>>> .
>>>>
>>>
>>
>>
>> .
>>
> 

Re: [Qemu-devel] [PULL 00/14] Net patches

[Peter Maydell]

On 8 March 2016 at 16:06, Zhang Chen <zhangchen.fnst@cn.fujitsu.com> wrote:
> I found the reason for this problem is that
> unix_connect() have not connect to sock_path before iov_send().
> It need time to establish connection. so can we fix it with usleep()
> like this:
>
>     recv_sock = unix_connect(sock_path, NULL);
>     g_assert_cmpint(recv_sock, !=, -1);
> +    usleep(1000);
>
>     ret = iov_send(send_sock[0], iov, 2, 0, sizeof(size) +
> sizeof(send_buf));
>     g_assert_cmpint(ret, ==, sizeof(send_buf) + sizeof(size));
>     close(send_sock[0]);
>
>     ret = qemu_recv(recv_sock, &len, sizeof(len), 0);

I would prefer it if we could find a way to fix this race
reliably rather than just inserting a delay and hoping it
is sufficient. Otherwise the test is likely to be unreliable
if run on a heavily loaded or slow machine.

thanks
-- PMM

Re: [Qemu-devel] [PULL 00/14] Net patches

[Wen Congyang]

On 03/08/2016 05:54 PM, Peter Maydell wrote:
> On 8 March 2016 at 16:06, Zhang Chen <zhangchen.fnst@cn.fujitsu.com> wrote:
>> I found the reason for this problem is that
>> unix_connect() have not connect to sock_path before iov_send().
>> It need time to establish connection. so can we fix it with usleep()
>> like this:
>>
>>     recv_sock = unix_connect(sock_path, NULL);
>>     g_assert_cmpint(recv_sock, !=, -1);
>> +    usleep(1000);
>>
>>     ret = iov_send(send_sock[0], iov, 2, 0, sizeof(size) +
>> sizeof(send_buf));
>>     g_assert_cmpint(ret, ==, sizeof(send_buf) + sizeof(size));
>>     close(send_sock[0]);
>>
>>     ret = qemu_recv(recv_sock, &len, sizeof(len), 0);
> 
> I would prefer it if we could find a way to fix this race
> reliably rather than just inserting a delay and hoping it
> is sufficient. Otherwise the test is likely to be unreliable
> if run on a heavily loaded or slow machine.

Yes, but there is no way to know when tcp_chr_accept() is called. Add a event
to notify it?

Thanks
Wen Congyang

> 
> thanks
> -- PMM
> 
> 
> 

Re: [Qemu-devel] [PULL 00/14] Net patches

[Li Zhijian]

On 03/09/2016 09:36 AM, Wen Congyang wrote:
> On 03/08/2016 05:54 PM, Peter Maydell wrote:
>> On 8 March 2016 at 16:06, Zhang Chen <zhangchen.fnst@cn.fujitsu.com> wrote:
>>> I found the reason for this problem is that
>>> unix_connect() have not connect to sock_path before iov_send().
>>> It need time to establish connection. so can we fix it with usleep()
>>> like this:
>>>
>>>      recv_sock = unix_connect(sock_path, NULL);
>>>      g_assert_cmpint(recv_sock, !=, -1);
>>> +    usleep(1000);
>>>
>>>      ret = iov_send(send_sock[0], iov, 2, 0, sizeof(size) +
>>> sizeof(send_buf));
>>>      g_assert_cmpint(ret, ==, sizeof(send_buf) + sizeof(size));
>>>      close(send_sock[0]);
>>>
>>>      ret = qemu_recv(recv_sock, &len, sizeof(len), 0);
>>
>> I would prefer it if we could find a way to fix this race
>> reliably rather than just inserting a delay and hoping it
>> is sufficient. Otherwise the test is likely to be unreliable
>> if run on a heavily loaded or slow machine.
>
> Yes, but there is no way to know when tcp_chr_accept() is called. Add a event
> to notify it?
>
> Thanks
> Wen Congyang
>

Hi, Jason, PMM
As Congyang said that this is a bug of testcase instead of filter-mirror.
Maybe we should re-wrok the testcase, for example
- using -chardev pipe instead of -chardev socket, because we are
   intend to test the packet mirror fuction instead of -chardev socket

How about that ?


>>
>> thanks
>> -- PMM
>>
>>
>>
>
>
>
>
>

Re: [Qemu-devel] [PULL 00/14] Net patches

[Wen Congyang]

On 03/09/2016 12:26 PM, Li Zhijian wrote:
> 
> 
> On 03/09/2016 09:36 AM, Wen Congyang wrote:
>> On 03/08/2016 05:54 PM, Peter Maydell wrote:
>>> On 8 March 2016 at 16:06, Zhang Chen <zhangchen.fnst@cn.fujitsu.com> wrote:
>>>> I found the reason for this problem is that
>>>> unix_connect() have not connect to sock_path before iov_send().
>>>> It need time to establish connection. so can we fix it with usleep()
>>>> like this:
>>>>
>>>>      recv_sock = unix_connect(sock_path, NULL);
>>>>      g_assert_cmpint(recv_sock, !=, -1);
>>>> +    usleep(1000);
>>>>
>>>>      ret = iov_send(send_sock[0], iov, 2, 0, sizeof(size) +
>>>> sizeof(send_buf));
>>>>      g_assert_cmpint(ret, ==, sizeof(send_buf) + sizeof(size));
>>>>      close(send_sock[0]);
>>>>
>>>>      ret = qemu_recv(recv_sock, &len, sizeof(len), 0);
>>>
>>> I would prefer it if we could find a way to fix this race
>>> reliably rather than just inserting a delay and hoping it
>>> is sufficient. Otherwise the test is likely to be unreliable
>>> if run on a heavily loaded or slow machine.
>>
>> Yes, but there is no way to know when tcp_chr_accept() is called. Add a event
>> to notify it?
>>
>> Thanks
>> Wen Congyang
>>
> 
> Hi, Jason, PMM
> As Congyang said that this is a bug of testcase instead of filter-mirror.
> Maybe we should re-wrok the testcase, for example
> - using -chardev pipe instead of -chardev socket, because we are
>   intend to test the packet mirror fuction instead of -chardev socket

I think it is OK to change it.

Thanks
Wen Congyang

> 
> How about that ?
> 
> 
>>>
>>> thanks
>>> -- PMM
>>>
>>>
>>>
>>
>>
>>
>>
>>
> .
> 

Re: [Qemu-devel] [PULL 00/14] Net patches

[Jason Wang]

On 03/09/2016 12:26 PM, Li Zhijian wrote:
>
>
> On 03/09/2016 09:36 AM, Wen Congyang wrote:
>> On 03/08/2016 05:54 PM, Peter Maydell wrote:
>>> On 8 March 2016 at 16:06, Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
>>> wrote:
>>>> I found the reason for this problem is that
>>>> unix_connect() have not connect to sock_path before iov_send().
>>>> It need time to establish connection. so can we fix it with usleep()
>>>> like this:
>>>>
>>>>      recv_sock = unix_connect(sock_path, NULL);
>>>>      g_assert_cmpint(recv_sock, !=, -1);
>>>> +    usleep(1000);
>>>>
>>>>      ret = iov_send(send_sock[0], iov, 2, 0, sizeof(size) +
>>>> sizeof(send_buf));
>>>>      g_assert_cmpint(ret, ==, sizeof(send_buf) + sizeof(size));
>>>>      close(send_sock[0]);
>>>>
>>>>      ret = qemu_recv(recv_sock, &len, sizeof(len), 0);
>>>
>>> I would prefer it if we could find a way to fix this race
>>> reliably rather than just inserting a delay and hoping it
>>> is sufficient. Otherwise the test is likely to be unreliable
>>> if run on a heavily loaded or slow machine.
>>
>> Yes, but there is no way to know when tcp_chr_accept() is called. Add
>> a event
>> to notify it?
>>
>> Thanks
>> Wen Congyang
>>
>
> Hi, Jason, PMM
> As Congyang said that this is a bug of testcase instead of filter-mirror.
> Maybe we should re-wrok the testcase, for example
> - using -chardev pipe instead of -chardev socket, because we are
>   intend to test the packet mirror fuction instead of -chardev socket
>
> How about that ?

I still prefer to use socket since it will be more similar to its real
usage (e.g COLO).

Btw, I haven't investigated deeply, but at least qmp command trick
should work. But what's better is to passing the pre-connected fd (e.g
the socket created by socketpair()) to chardev, not sure we can do this.

Re: [Qemu-devel] [PULL 00/14] Net patches

[Jason Wang]

On 03/08/2016 05:54 PM, Peter Maydell wrote:
> On 8 March 2016 at 16:06, Zhang Chen <zhangchen.fnst@cn.fujitsu.com> wrote:
>> I found the reason for this problem is that
>> unix_connect() have not connect to sock_path before iov_send().
>> It need time to establish connection. so can we fix it with usleep()
>> like this:
>>
>>     recv_sock = unix_connect(sock_path, NULL);
>>     g_assert_cmpint(recv_sock, !=, -1);
>> +    usleep(1000);
>>
>>     ret = iov_send(send_sock[0], iov, 2, 0, sizeof(size) +
>> sizeof(send_buf));
>>     g_assert_cmpint(ret, ==, sizeof(send_buf) + sizeof(size));
>>     close(send_sock[0]);
>>
>>     ret = qemu_recv(recv_sock, &len, sizeof(len), 0);
> I would prefer it if we could find a way to fix this race
> reliably rather than just inserting a delay and hoping it
> is sufficient. Otherwise the test is likely to be unreliable
> if run on a heavily loaded or slow machine.
>
> thanks
> -- PMM
>

+1

To make sure the connected socket to be proceeded before iov_send(), you
could use some like  qmp("{ 'execute' : 'query-status'}") before
iov_send(). With this we are guaranteed that connected is setting to
true before iov_send().

Re: [Qemu-devel] [PULL 00/14] Net patches

[Li Zhijian]

On 03/10/2016 10:28 AM, Jason Wang wrote:
>
>
> On 03/08/2016 05:54 PM, Peter Maydell wrote:
>> On 8 March 2016 at 16:06, Zhang Chen <zhangchen.fnst@cn.fujitsu.com> wrote:
>>> I found the reason for this problem is that
>>> unix_connect() have not connect to sock_path before iov_send().
>>> It need time to establish connection. so can we fix it with usleep()
>>> like this:
>>>
>>>      recv_sock = unix_connect(sock_path, NULL);
>>>      g_assert_cmpint(recv_sock, !=, -1);
>>> +    usleep(1000);
>>>
>>>      ret = iov_send(send_sock[0], iov, 2, 0, sizeof(size) +
>>> sizeof(send_buf));
>>>      g_assert_cmpint(ret, ==, sizeof(send_buf) + sizeof(size));
>>>      close(send_sock[0]);
>>>
>>>      ret = qemu_recv(recv_sock, &len, sizeof(len), 0);
>> I would prefer it if we could find a way to fix this race
>> reliably rather than just inserting a delay and hoping it
>> is sufficient. Otherwise the test is likely to be unreliable
>> if run on a heavily loaded or slow machine.
>>
>> thanks
>> -- PMM
>>
>
> +1
>
> To make sure the connected socket to be proceeded before iov_send(), you
> could use some like  qmp("{ 'execute' : 'query-status'}") before
> iov_send(). With this we are guaranteed that connected is setting to
> true before iov_send().
>
>

it seem works, but i don't know.
Is this because that both qemu accepting the connection and qmp command are working under *iothread*,
so that when the qemu command returns, we can guaranteed the connection is accepted ?

Thanks
Li Zhijian
>
>
>

Re: [Qemu-devel] [PULL 00/14] Net patches

[Jason Wang]

On 03/10/2016 11:51 AM, Li Zhijian wrote:
>
>
> On 03/10/2016 10:28 AM, Jason Wang wrote:
>>
>>
>> On 03/08/2016 05:54 PM, Peter Maydell wrote:
>>> On 8 March 2016 at 16:06, Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
>>> wrote:
>>>> I found the reason for this problem is that
>>>> unix_connect() have not connect to sock_path before iov_send().
>>>> It need time to establish connection. so can we fix it with usleep()
>>>> like this:
>>>>
>>>>      recv_sock = unix_connect(sock_path, NULL);
>>>>      g_assert_cmpint(recv_sock, !=, -1);
>>>> +    usleep(1000);
>>>>
>>>>      ret = iov_send(send_sock[0], iov, 2, 0, sizeof(size) +
>>>> sizeof(send_buf));
>>>>      g_assert_cmpint(ret, ==, sizeof(send_buf) + sizeof(size));
>>>>      close(send_sock[0]);
>>>>
>>>>      ret = qemu_recv(recv_sock, &len, sizeof(len), 0);
>>> I would prefer it if we could find a way to fix this race
>>> reliably rather than just inserting a delay and hoping it
>>> is sufficient. Otherwise the test is likely to be unreliable
>>> if run on a heavily loaded or slow machine.
>>>
>>> thanks
>>> -- PMM
>>>
>>
>> +1
>>
>> To make sure the connected socket to be proceeded before iov_send(), you
>> could use some like  qmp("{ 'execute' : 'query-status'}") before
>> iov_send(). With this we are guaranteed that connected is setting to
>> true before iov_send().
>>
>>
>
> it seem works, but i don't know.
> Is this because that both qemu accepting the connection and qmp
> command are working under *iothread*,
> so that when the qemu command returns, we can guaranteed the
> connection is accepted ?

I think the problem is the race in main loop between the two handlers.
If the socket netdev read handler was proceed before chardev read
handler, since connected was false the packet will be dropped silently.
After we place whateve a qmp command in the middled, it's guaranteed
that all handlers were proceed after qmp command was executed, so we are
sure the connected is true when doing iov_send().

>
> Thanks
> Li Zhijian
>>
>>
>>
>
>
>

Re: [Qemu-devel] [PULL 00/14] Net patches

[Li Zhijian]

On 03/15/2016 11:07 AM, Jason Wang wrote:
>
>
> On 03/10/2016 11:51 AM, Li Zhijian wrote:
>>
>>
>> On 03/10/2016 10:28 AM, Jason Wang wrote:
>>>
>>>
>>> On 03/08/2016 05:54 PM, Peter Maydell wrote:
>>>> On 8 March 2016 at 16:06, Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
>>>> wrote:
>>>>> I found the reason for this problem is that
>>>>> unix_connect() have not connect to sock_path before iov_send().
>>>>> It need time to establish connection. so can we fix it with usleep()
>>>>> like this:
>>>>>
>>>>>       recv_sock = unix_connect(sock_path, NULL);
>>>>>       g_assert_cmpint(recv_sock, !=, -1);
>>>>> +    usleep(1000);
>>>>>
>>>>>       ret = iov_send(send_sock[0], iov, 2, 0, sizeof(size) +
>>>>> sizeof(send_buf));
>>>>>       g_assert_cmpint(ret, ==, sizeof(send_buf) + sizeof(size));
>>>>>       close(send_sock[0]);
>>>>>
>>>>>       ret = qemu_recv(recv_sock, &len, sizeof(len), 0);
>>>> I would prefer it if we could find a way to fix this race
>>>> reliably rather than just inserting a delay and hoping it
>>>> is sufficient. Otherwise the test is likely to be unreliable
>>>> if run on a heavily loaded or slow machine.
>>>>
>>>> thanks
>>>> -- PMM
>>>>
>>>
>>> +1
>>>
>>> To make sure the connected socket to be proceeded before iov_send(), you
>>> could use some like  qmp("{ 'execute' : 'query-status'}") before
>>> iov_send(). With this we are guaranteed that connected is setting to
>>> true before iov_send().
>>>
>>>
>>
>> it seem works, but i don't know.
>> Is this because that both qemu accepting the connection and qmp
>> command are working under *iothread*,
>> so that when the qemu command returns, we can guaranteed the
>> connection is accepted ?
>
> I think the problem is the race in main loop between the two handlers.
> If the socket netdev read handler was proceed before chardev read
> handler, since connected was false the packet will be dropped silently.
> After we place whateve a qmp command in the middled, it's guaranteed
> that all handlers were proceed after qmp command was executed, so we are
> sure the connected is true when doing iov_send().
>

Got it, thank for your explain.

we will send V9 with '-chardev socket' and add an qmp sync point soon.

Thanks
Li Zhijian

>>
>> Thanks
>> Li Zhijian
>>>
>>>
>>>
>>
>>
>>
>
>
>
> .
>