All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH V8 0/6] Passthrough specific network traffic in COLO
@ 2021-06-15 11:37 Zhang Chen
  2021-06-15 11:37 ` [PATCH V8 1/6] qapi/net: Add IPFlowSpec and QMP command for COLO passthrough Zhang Chen
                   ` (5 more replies)
  0 siblings, 6 replies; 25+ messages in thread
From: Zhang Chen @ 2021-06-15 11:37 UTC (permalink / raw)
  To: Jason Wang, qemu-dev, Eric Blake, Dr. David Alan Gilbert,
	Markus Armbruster, Daniel P. Berrangé,
	Gerd Hoffmann, Li Zhijian
  Cc: Zhang Chen, Lukas Straub, Zhang Chen

Current colo-compare and net-filters attached on chardev or netdev.
It still need more fine-grained network control based on IPFlowSpec. 
Due to some real user scenarios don't need to monitor all traffic.
And qemu net-filter also need function to more detailed flow control.
This series give user ability to passthrough kinds of COLO network stream.

For example, windows guest user want to enable windows remote desktop
to touch guest(UDP/TCP 3389), This case use UDP and TCP mixed, and the
tcp part payload always different caused by real desktop display
data(for guest time/ mouse display....).

Another case is some real user application will actively transmit information
include guest time part, primary guest send data with time 10:01.000,
At the same time secondary guest send data with time 10:01.001,
it will always trigger COLO checkpoint(live migrate) to drop guest performance.

  V8:
    - Add more comments about QAPI IPFlowSpec.
    - Fix grammar issue on colo-passthrough-add/delete.
    - Rebased on upstream.

  V7:
    - Keep some data structure stay in .c (patch 4/6).
    - Fix mutex init issue (patch 5/6).
    - Make the IPFlowSpec 'protocol' field optional (patch 1/6).
    - Add compare_passthrough_find function in net.c (patch 6/6).

  V6:
    - Change QAPI IPFlowSpec protocol from enum to str.
    - Use getprotobyname to handle the protocols.
    - Optimize code in net.

  V5:
    - Squash original 1-3 QAPI patches together.
    - Rename some data structures to avoid misunderstanding.
    - Reuse InetSocketAddressBase in IPFlowSpec.
    - Add new function in util/qemu-sockets.c to parse
      InetSocketAddressBase.
    - Update HMP command define to reuse current code.
    - Add more comments.

  V4:
    - Fix QAPI code conflict for V6.0 merged patches.
    - Note this feature for V6.1.




Zhang Chen (6):
  qapi/net: Add IPFlowSpec and QMP command for COLO passthrough
  util/qemu-sockets.c: Add inet_parse_base to handle
    InetSocketAddressBase
  hmp-commands: Add new HMP command for COLO passthrough
  net/colo-compare: Move data structure and define to .h file.
  net/colo-compare: Add passthrough list to CompareState
  net/net.c: Add handler for COLO passthrough connection

 hmp-commands.hx        |  26 +++++++
 include/monitor/hmp.h  |   2 +
 include/qemu/sockets.h |   1 +
 monitor/hmp-cmds.c     |  82 ++++++++++++++++++++
 net/colo-compare.c     | 160 +++++++++++++--------------------------
 net/colo-compare.h     |  98 ++++++++++++++++++++++++
 net/net.c              | 168 +++++++++++++++++++++++++++++++++++++++++
 qapi/net.json          |  74 ++++++++++++++++++
 util/qemu-sockets.c    |  14 ++++
 9 files changed, 516 insertions(+), 109 deletions(-)

-- 
2.25.1



^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH V8 1/6] qapi/net: Add IPFlowSpec and QMP command for COLO passthrough
  2021-06-15 11:37 [PATCH V8 0/6] Passthrough specific network traffic in COLO Zhang Chen
@ 2021-06-15 11:37 ` Zhang Chen
  2021-06-15 14:43   ` Markus Armbruster
                     ` (2 more replies)
  2021-06-15 11:37 ` [PATCH V8 2/6] util/qemu-sockets.c: Add inet_parse_base to handle InetSocketAddressBase Zhang Chen
                   ` (4 subsequent siblings)
  5 siblings, 3 replies; 25+ messages in thread
From: Zhang Chen @ 2021-06-15 11:37 UTC (permalink / raw)
  To: Jason Wang, qemu-dev, Eric Blake, Dr. David Alan Gilbert,
	Markus Armbruster, Daniel P. Berrangé,
	Gerd Hoffmann, Li Zhijian
  Cc: Zhang Chen, Lukas Straub, Zhang Chen

Since the real user scenario does not need COLO to monitor all traffic.
Add colo-passthrough-add and colo-passthrough-del to maintain
a COLO network passthrough list. Add IPFlowSpec struct for all QMP commands.
All the fields of IPFlowSpec are optional.

Signed-off-by: Zhang Chen <chen.zhang@intel.com>
---
 net/net.c     | 10 +++++++
 qapi/net.json | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 84 insertions(+)

diff --git a/net/net.c b/net/net.c
index 76bbb7c31b..f913e97983 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1195,6 +1195,16 @@ void qmp_netdev_del(const char *id, Error **errp)
     }
 }
 
+void qmp_colo_passthrough_add(IPFlowSpec *spec, Error **errp)
+{
+    /* TODO implement setup passthrough rule */
+}
+
+void qmp_colo_passthrough_del(IPFlowSpec *spec, Error **errp)
+{
+    /* TODO implement delete passthrough rule */
+}
+
 static void netfilter_print_info(Monitor *mon, NetFilterState *nf)
 {
     char *str;
diff --git a/qapi/net.json b/qapi/net.json
index 7fab2e7cd8..91f2e1495a 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -7,6 +7,7 @@
 ##
 
 { 'include': 'common.json' }
+{ 'include': 'sockets.json' }
 
 ##
 # @set_link:
@@ -696,3 +697,76 @@
 ##
 { 'event': 'FAILOVER_NEGOTIATED',
   'data': {'device-id': 'str'} }
+
+##
+# @IPFlowSpec:
+#
+# IP flow specification.
+#
+# @protocol: Transport layer protocol like TCP/UDP, etc. The protocol is the
+#            string instead of enum, because it can be passed to getprotobyname(3)
+#            and avoid duplication with /etc/protocols.
+#
+# @object-name: The @object-name means packet handler in Qemu. Because not
+#               all the network packet must pass the colo-compare module,
+#               the net-filters are same situation. There modules attach to
+#               netdev or chardev to work, VM can run multiple modules
+#               at the same time. So it needs the object-name to set
+#               the effective module.
+#
+# @source: Source address and port.
+#
+# @destination: Destination address and port.
+#
+# Since: 6.1
+##
+{ 'struct': 'IPFlowSpec',
+  'data': { '*protocol': 'str', '*object-name': 'str',
+    '*source': 'InetSocketAddressBase',
+    '*destination': 'InetSocketAddressBase' } }
+
+##
+# @colo-passthrough-add:
+#
+# Add passthrough entry IPFlowSpec to the COLO-compare instance.
+# The protocol and source/destination IP/ports are optional. if the user
+# only inputs part of the information, this will match all traffic.
+#
+# Returns: Nothing on success
+#
+# Since: 6.1
+#
+# Example:
+#
+# -> { "execute": "colo-passthrough-add",
+#      "arguments": { "protocol": "tcp", "object-name": "object0",
+#      "source": {"host": "192.168.1.1", "port": "1234"},
+#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
+# <- { "return": {} }
+#
+##
+{ 'command': 'colo-passthrough-add', 'boxed': true,
+     'data': 'IPFlowSpec' }
+
+##
+# @colo-passthrough-del:
+#
+# Delete passthrough entry IPFlowSpec to the COLO-compare instance.
+# The protocol and source/destination IP/ports are optional. if the user
+# only inputs part of the information, this will match all traffic.
+#
+# Returns: Nothing on success
+#
+# Since: 6.1
+#
+# Example:
+#
+# -> { "execute": "colo-passthrough-del",
+#      "arguments": { "protocol": "tcp", "object-name": "object0",
+#      "source": {"host": "192.168.1.1", "port": "1234"},
+#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
+# <- { "return": {} }
+#
+##
+{ 'command': 'colo-passthrough-del', 'boxed': true,
+     'data': 'IPFlowSpec' }
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH V8 2/6] util/qemu-sockets.c: Add inet_parse_base to handle InetSocketAddressBase
  2021-06-15 11:37 [PATCH V8 0/6] Passthrough specific network traffic in COLO Zhang Chen
  2021-06-15 11:37 ` [PATCH V8 1/6] qapi/net: Add IPFlowSpec and QMP command for COLO passthrough Zhang Chen
@ 2021-06-15 11:37 ` Zhang Chen
  2021-06-15 11:37 ` [PATCH V8 3/6] hmp-commands: Add new HMP command for COLO passthrough Zhang Chen
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 25+ messages in thread
From: Zhang Chen @ 2021-06-15 11:37 UTC (permalink / raw)
  To: Jason Wang, qemu-dev, Eric Blake, Dr. David Alan Gilbert,
	Markus Armbruster, Daniel P. Berrangé,
	Gerd Hoffmann, Li Zhijian
  Cc: Zhang Chen, Lukas Straub, Zhang Chen

No need to carry the flag all the time in many scenarios.

Signed-off-by: Zhang Chen <chen.zhang@intel.com>
---
 include/qemu/sockets.h |  1 +
 util/qemu-sockets.c    | 14 ++++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 7d1f813576..d5abc227eb 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -32,6 +32,7 @@ int socket_set_fast_reuse(int fd);
 int inet_ai_family_from_address(InetSocketAddress *addr,
                                 Error **errp);
 int inet_parse(InetSocketAddress *addr, const char *str, Error **errp);
+int inet_parse_base(InetSocketAddressBase *addr, const char *str, Error **errp);
 int inet_connect(const char *str, Error **errp);
 int inet_connect_saddr(InetSocketAddress *saddr, Error **errp);
 
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index c415c342c1..4951851d9d 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -713,6 +713,20 @@ int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)
     return 0;
 }
 
+int inet_parse_base(InetSocketAddressBase *base, const char *str, Error **errp)
+{
+    InetSocketAddress *addr;
+    int ret = 0;
+
+    addr = g_new0(InetSocketAddress, 1);
+    ret = inet_parse(addr, str, errp);
+
+    base->host = addr->host;
+    base->port = addr->port;
+
+    g_free(addr);
+    return ret;
+}
 
 /**
  * Create a blocking socket and connect it to an address.
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH V8 3/6] hmp-commands: Add new HMP command for COLO passthrough
  2021-06-15 11:37 [PATCH V8 0/6] Passthrough specific network traffic in COLO Zhang Chen
  2021-06-15 11:37 ` [PATCH V8 1/6] qapi/net: Add IPFlowSpec and QMP command for COLO passthrough Zhang Chen
  2021-06-15 11:37 ` [PATCH V8 2/6] util/qemu-sockets.c: Add inet_parse_base to handle InetSocketAddressBase Zhang Chen
@ 2021-06-15 11:37 ` Zhang Chen
  2021-06-15 11:37 ` [PATCH V8 4/6] net/colo-compare: Move data structure and define to .h file Zhang Chen
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 25+ messages in thread
From: Zhang Chen @ 2021-06-15 11:37 UTC (permalink / raw)
  To: Jason Wang, qemu-dev, Eric Blake, Dr. David Alan Gilbert,
	Markus Armbruster, Daniel P. Berrangé,
	Gerd Hoffmann, Li Zhijian
  Cc: Zhang Chen, Lukas Straub, Zhang Chen

Add hmp_colo_passthrough_add and hmp_colo_passthrough_del make user
can maintain COLO network passthrough list in human monitor

Signed-off-by: Zhang Chen <chen.zhang@intel.com>
---
 hmp-commands.hx       | 26 ++++++++++++++
 include/monitor/hmp.h |  2 ++
 monitor/hmp-cmds.c    | 82 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 110 insertions(+)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 8e45bce2cd..895e60a774 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1292,6 +1292,32 @@ SRST
   Remove host network device.
 ERST
 
+    {
+        .name       = "colo_passthrough_add",
+        .args_type  = "protocol:s,object-name:s?,src:s?,dst:s?",
+        .params     = "protocol [object-name] [src] [dst]",
+        .help       = "Add network stream to colo passthrough list",
+        .cmd        = hmp_colo_passthrough_add,
+    },
+
+SRST
+``colo_passthrough_add``
+  Add network stream to colo passthrough list.
+ERST
+
+    {
+        .name       = "colo_passthrough_del",
+        .args_type  = "protocol:s,object-name:s?,src:s?,dst:s?",
+        .params     = "protocol [object-name] [src] [dst]",
+        .help       = "Delete network stream from colo passthrough list",
+        .cmd        = hmp_colo_passthrough_del,
+    },
+
+SRST
+``colo_passthrough_del``
+  Delete network stream from colo passthrough list.
+ERST
+
     {
         .name       = "object_add",
         .args_type  = "object:S",
diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index 3baa1058e2..2e171dba0c 100644
--- a/include/monitor/hmp.h
+++ b/include/monitor/hmp.h
@@ -77,6 +77,8 @@ void hmp_device_del(Monitor *mon, const QDict *qdict);
 void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict);
 void hmp_netdev_add(Monitor *mon, const QDict *qdict);
 void hmp_netdev_del(Monitor *mon, const QDict *qdict);
+void hmp_colo_passthrough_add(Monitor *mon, const QDict *qdict);
+void hmp_colo_passthrough_del(Monitor *mon, const QDict *qdict);
 void hmp_getfd(Monitor *mon, const QDict *qdict);
 void hmp_closefd(Monitor *mon, const QDict *qdict);
 void hmp_sendkey(Monitor *mon, const QDict *qdict);
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index d10ee14110..9d3269b28b 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1634,6 +1634,88 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, err);
 }
 
+void hmp_colo_passthrough_add(Monitor *mon, const QDict *qdict)
+{
+    IPFlowSpec *spec = g_new0(IPFlowSpec, 1);
+    char *src, *dst;
+    Error *err = NULL;
+
+    spec->protocol = g_strdup(qdict_get_try_str(qdict, "protocol"));
+    spec->object_name = g_strdup(qdict_get_try_str(qdict, "object-name"));
+
+    src = g_strdup(qdict_get_try_str(qdict, "src"));
+    if (src) {
+        spec->source = g_new0(InetSocketAddressBase, 1);
+
+        if (inet_parse_base(spec->source, src, NULL)) {
+            monitor_printf(mon, "bad colo passthrough src address");
+            goto out;
+        }
+    }
+
+    dst = g_strdup(qdict_get_try_str(qdict, "dst"));
+    if (dst) {
+        spec->destination = g_new0(InetSocketAddressBase, 1);
+
+        if (inet_parse_base(spec->destination, dst, NULL)) {
+            monitor_printf(mon, "bad colo passthrough dst address");
+            goto out;
+        }
+    }
+
+    qmp_colo_passthrough_add(spec, &err);
+
+out:
+    g_free(src);
+    src = NULL;
+
+    g_free(dst);
+    dst = NULL;
+
+    hmp_handle_error(mon, err);
+}
+
+void hmp_colo_passthrough_del(Monitor *mon, const QDict *qdict)
+{
+    IPFlowSpec *spec = g_new0(IPFlowSpec, 1);
+    char *src, *dst;
+    Error *err = NULL;
+
+    spec->protocol = g_strdup(qdict_get_try_str(qdict, "protocol"));
+    spec->object_name = g_strdup(qdict_get_try_str(qdict, "object-name"));
+
+    src = g_strdup(qdict_get_try_str(qdict, "src"));
+    if (src) {
+        spec->source = g_new0(InetSocketAddressBase, 1);
+
+        if (inet_parse_base(spec->source, src, NULL)) {
+            monitor_printf(mon, "bad colo passthrough src address");
+            goto out;
+        }
+    }
+
+    dst = g_strdup(qdict_get_try_str(qdict, "dst"));
+    if (dst) {
+        spec->destination = g_new0(InetSocketAddressBase, 1);
+
+        if (inet_parse_base(spec->destination, dst, NULL)) {
+            monitor_printf(mon, "bad colo passthrough dst address");
+            goto out;
+        }
+    }
+
+    qmp_colo_passthrough_del(spec, &err);
+
+out:
+    g_free(src);
+    src = NULL;
+
+    g_free(dst);
+    dst = NULL;
+
+    hmp_handle_error(mon, err);
+}
+
 void hmp_object_add(Monitor *mon, const QDict *qdict)
 {
     const char *options = qdict_get_str(qdict, "object");
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH V8 4/6] net/colo-compare: Move data structure and define to .h file.
  2021-06-15 11:37 [PATCH V8 0/6] Passthrough specific network traffic in COLO Zhang Chen
                   ` (2 preceding siblings ...)
  2021-06-15 11:37 ` [PATCH V8 3/6] hmp-commands: Add new HMP command for COLO passthrough Zhang Chen
@ 2021-06-15 11:37 ` Zhang Chen
  2021-06-15 11:37 ` [PATCH V8 5/6] net/colo-compare: Add passthrough list to CompareState Zhang Chen
  2021-06-15 11:37 ` [PATCH V8 6/6] net/net.c: Add handler for COLO passthrough connection Zhang Chen
  5 siblings, 0 replies; 25+ messages in thread
From: Zhang Chen @ 2021-06-15 11:37 UTC (permalink / raw)
  To: Jason Wang, qemu-dev, Eric Blake, Dr. David Alan Gilbert,
	Markus Armbruster, Daniel P. Berrangé,
	Gerd Hoffmann, Li Zhijian
  Cc: Zhang Chen, Lukas Straub, Zhang Chen

Rename structure with COLO index and move it to .h file,
It make other modules can reuse COLO code.

Signed-off-by: Zhang Chen <chen.zhang@intel.com>
---
 net/colo-compare.c | 132 ++++++++-------------------------------------
 net/colo-compare.h |  86 +++++++++++++++++++++++++++++
 2 files changed, 109 insertions(+), 109 deletions(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index b100e7b51f..dcd24bb113 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -17,29 +17,18 @@
 #include "qemu/error-report.h"
 #include "trace.h"
 #include "qapi/error.h"
-#include "net/net.h"
 #include "net/eth.h"
 #include "qom/object_interfaces.h"
 #include "qemu/iov.h"
 #include "qom/object.h"
 #include "net/queue.h"
-#include "chardev/char-fe.h"
 #include "qemu/sockets.h"
-#include "colo.h"
-#include "sysemu/iothread.h"
 #include "net/colo-compare.h"
-#include "migration/colo.h"
-#include "migration/migration.h"
 #include "util.h"
 
 #include "block/aio-wait.h"
 #include "qemu/coroutine.h"
 
-#define TYPE_COLO_COMPARE "colo-compare"
-typedef struct CompareState CompareState;
-DECLARE_INSTANCE_CHECKER(CompareState, COLO_COMPARE,
-                         TYPE_COLO_COMPARE)
-
 static QTAILQ_HEAD(, CompareState) net_compares =
        QTAILQ_HEAD_INITIALIZER(net_compares);
 
@@ -47,13 +36,13 @@ static NotifierList colo_compare_notifiers =
     NOTIFIER_LIST_INITIALIZER(colo_compare_notifiers);
 
 #define COMPARE_READ_LEN_MAX NET_BUFSIZE
-#define MAX_QUEUE_SIZE 1024
+#define MAX_COLO_QUEUE_SIZE 1024
 
 #define COLO_COMPARE_FREE_PRIMARY     0x01
 #define COLO_COMPARE_FREE_SECONDARY   0x02
 
-#define REGULAR_PACKET_CHECK_MS 1000
-#define DEFAULT_TIME_OUT_MS 3000
+#define COLO_REGULAR_PACKET_CHECK_MS 1000
+#define COLO_DEFAULT_TIME_OUT_MS 3000
 
 /* #define DEBUG_COLO_PACKETS */
 
@@ -64,87 +53,6 @@ static QemuCond event_complete_cond;
 static int event_unhandled_count;
 static uint32_t max_queue_size;
 
-/*
- *  + CompareState ++
- *  |               |
- *  +---------------+   +---------------+         +---------------+
- *  |   conn list   + - >      conn     + ------- >      conn     + -- > ......
- *  +---------------+   +---------------+         +---------------+
- *  |               |     |           |             |          |
- *  +---------------+ +---v----+  +---v----+    +---v----+ +---v----+
- *                    |primary |  |secondary    |primary | |secondary
- *                    |packet  |  |packet  +    |packet  | |packet  +
- *                    +--------+  +--------+    +--------+ +--------+
- *                        |           |             |          |
- *                    +---v----+  +---v----+    +---v----+ +---v----+
- *                    |primary |  |secondary    |primary | |secondary
- *                    |packet  |  |packet  +    |packet  | |packet  +
- *                    +--------+  +--------+    +--------+ +--------+
- *                        |           |             |          |
- *                    +---v----+  +---v----+    +---v----+ +---v----+
- *                    |primary |  |secondary    |primary | |secondary
- *                    |packet  |  |packet  +    |packet  | |packet  +
- *                    +--------+  +--------+    +--------+ +--------+
- */
-
-typedef struct SendCo {
-    Coroutine *co;
-    struct CompareState *s;
-    CharBackend *chr;
-    GQueue send_list;
-    bool notify_remote_frame;
-    bool done;
-    int ret;
-} SendCo;
-
-typedef struct SendEntry {
-    uint32_t size;
-    uint32_t vnet_hdr_len;
-    uint8_t *buf;
-} SendEntry;
-
-struct CompareState {
-    Object parent;
-
-    char *pri_indev;
-    char *sec_indev;
-    char *outdev;
-    char *notify_dev;
-    CharBackend chr_pri_in;
-    CharBackend chr_sec_in;
-    CharBackend chr_out;
-    CharBackend chr_notify_dev;
-    SocketReadState pri_rs;
-    SocketReadState sec_rs;
-    SocketReadState notify_rs;
-    SendCo out_sendco;
-    SendCo notify_sendco;
-    bool vnet_hdr;
-    uint64_t compare_timeout;
-    uint32_t expired_scan_cycle;
-
-    /*
-     * Record the connection that through the NIC
-     * Element type: Connection
-     */
-    GQueue conn_list;
-    /* Record the connection without repetition */
-    GHashTable *connection_track_table;
-
-    IOThread *iothread;
-    GMainContext *worker_context;
-    QEMUTimer *packet_check_timer;
-
-    QEMUBH *event_bh;
-    enum colo_event event;
-
-    QTAILQ_ENTRY(CompareState) next;
-};
-
-typedef struct CompareClass {
-    ObjectClass parent_class;
-} CompareClass;
-
 enum {
     PRIMARY_IN = 0,
     SECONDARY_IN,
@@ -155,6 +63,12 @@ static const char *colo_mode[] = {
     [SECONDARY_IN] = "secondary",
 };
 
+typedef struct COLOSendEntry {
+    uint32_t size;
+    uint32_t vnet_hdr_len;
+    uint8_t *buf;
+} COLOSendEntry;
+
 static int compare_chr_send(CompareState *s,
                             uint8_t *buf,
                             uint32_t size,
@@ -724,19 +638,19 @@ static void colo_compare_connection(void *opaque, void *user_data)
 
 static void coroutine_fn _compare_chr_send(void *opaque)
 {
-    SendCo *sendco = opaque;
+    COLOSendCo *sendco = opaque;
     CompareState *s = sendco->s;
     int ret = 0;
 
     while (!g_queue_is_empty(&sendco->send_list)) {
-        SendEntry *entry = g_queue_pop_tail(&sendco->send_list);
+        COLOSendEntry *entry = g_queue_pop_tail(&sendco->send_list);
         uint32_t len = htonl(entry->size);
 
         ret = qemu_chr_fe_write_all(sendco->chr, (uint8_t *)&len, sizeof(len));
 
         if (ret != sizeof(len)) {
             g_free(entry->buf);
-            g_slice_free(SendEntry, entry);
+            g_slice_free(COLOSendEntry, entry);
             goto err;
         }
 
@@ -753,7 +667,7 @@ static void coroutine_fn _compare_chr_send(void *opaque)
 
             if (ret != sizeof(len)) {
                 g_free(entry->buf);
-                g_slice_free(SendEntry, entry);
+                g_slice_free(COLOSendEntry, entry);
                 goto err;
             }
         }
@@ -764,12 +678,12 @@ static void coroutine_fn _compare_chr_send(void *opaque)
 
         if (ret != entry->size) {
             g_free(entry->buf);
-            g_slice_free(SendEntry, entry);
+            g_slice_free(COLOSendEntry, entry);
             goto err;
         }
 
         g_free(entry->buf);
-        g_slice_free(SendEntry, entry);
+        g_slice_free(COLOSendEntry, entry);
     }
 
     sendco->ret = 0;
@@ -777,9 +691,9 @@ static void coroutine_fn _compare_chr_send(void *opaque)
 
 err:
     while (!g_queue_is_empty(&sendco->send_list)) {
-        SendEntry *entry = g_queue_pop_tail(&sendco->send_list);
+        COLOSendEntry *entry = g_queue_pop_tail(&sendco->send_list);
         g_free(entry->buf);
-        g_slice_free(SendEntry, entry);
+        g_slice_free(COLOSendEntry, entry);
     }
     sendco->ret = ret < 0 ? ret : -EIO;
 out:
@@ -795,8 +709,8 @@ static int compare_chr_send(CompareState *s,
                             bool notify_remote_frame,
                             bool zero_copy)
 {
-    SendCo *sendco;
-    SendEntry *entry;
+    COLOSendCo *sendco;
+    COLOSendEntry *entry;
 
     if (notify_remote_frame) {
         sendco = &s->notify_sendco;
@@ -808,7 +722,7 @@ static int compare_chr_send(CompareState *s,
         return 0;
     }
 
-    entry = g_slice_new(SendEntry);
+    entry = g_slice_new(COLOSendEntry);
     entry->size = size;
     entry->vnet_hdr_len = vnet_hdr_len;
     if (zero_copy) {
@@ -1261,17 +1175,17 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
 
     if (!s->compare_timeout) {
         /* Set default value to 3000 MS */
-        s->compare_timeout = DEFAULT_TIME_OUT_MS;
+        s->compare_timeout = COLO_DEFAULT_TIME_OUT_MS;
     }
 
     if (!s->expired_scan_cycle) {
         /* Set default value to 3000 MS */
-        s->expired_scan_cycle = REGULAR_PACKET_CHECK_MS;
+        s->expired_scan_cycle = COLO_REGULAR_PACKET_CHECK_MS;
     }
 
     if (!max_queue_size) {
         /* Set default queue size to 1024 */
-        max_queue_size = MAX_QUEUE_SIZE;
+        max_queue_size = MAX_COLO_QUEUE_SIZE;
     }
 
     if (find_and_check_chardev(&chr, s->pri_indev, errp) ||
diff --git a/net/colo-compare.h b/net/colo-compare.h
index b055270da2..031b627a2f 100644
--- a/net/colo-compare.h
+++ b/net/colo-compare.h
@@ -17,6 +17,92 @@
 #ifndef QEMU_COLO_COMPARE_H
 #define QEMU_COLO_COMPARE_H
 
+#include "net/net.h"
+#include "chardev/char-fe.h"
+#include "migration/colo.h"
+#include "migration/migration.h"
+#include "sysemu/iothread.h"
+#include "colo.h"
+
+#define TYPE_COLO_COMPARE "colo-compare"
+typedef struct CompareState CompareState;
+DECLARE_INSTANCE_CHECKER(CompareState, COLO_COMPARE,
+                         TYPE_COLO_COMPARE)
+
+typedef struct COLOSendCo {
+    Coroutine *co;
+    struct CompareState *s;
+    CharBackend *chr;
+    GQueue send_list;
+    bool notify_remote_frame;
+    bool done;
+    int ret;
+} COLOSendCo;
+
+/*
+ *  + CompareState ++
+ *  |               |
+ *  +---------------+   +---------------+         +---------------+
+ *  |   conn list   + - >      conn     + ------- >      conn     + -- > ......
+ *  +---------------+   +---------------+         +---------------+
+ *  |               |     |           |             |          |
+ *  +---------------+ +---v----+  +---v----+    +---v----+ +---v----+
+ *                    |primary |  |secondary    |primary | |secondary
+ *                    |packet  |  |packet  +    |packet  | |packet  +
+ *                    +--------+  +--------+    +--------+ +--------+
+ *                        |           |             |          |
+ *                    +---v----+  +---v----+    +---v----+ +---v----+
+ *                    |primary |  |secondary    |primary | |secondary
+ *                    |packet  |  |packet  +    |packet  | |packet  +
+ *                    +--------+  +--------+    +--------+ +--------+
+ *                        |           |             |          |
+ *                    +---v----+  +---v----+    +---v----+ +---v----+
+ *                    |primary |  |secondary    |primary | |secondary
+ *                    |packet  |  |packet  +    |packet  | |packet  +
+ *                    +--------+  +--------+    +--------+ +--------+
+ */
+struct CompareState {
+    Object parent;
+
+    char *pri_indev;
+    char *sec_indev;
+    char *outdev;
+    char *notify_dev;
+    CharBackend chr_pri_in;
+    CharBackend chr_sec_in;
+    CharBackend chr_out;
+    CharBackend chr_notify_dev;
+    SocketReadState pri_rs;
+    SocketReadState sec_rs;
+    SocketReadState notify_rs;
+    COLOSendCo out_sendco;
+    COLOSendCo notify_sendco;
+    bool vnet_hdr;
+    uint64_t compare_timeout;
+    uint32_t expired_scan_cycle;
+
+    /*
+     * Record the connection that through the NIC
+     * Element type: Connection
+     */
+    GQueue conn_list;
+    /* Record the connection without repetition */
+    GHashTable *connection_track_table;
+
+    IOThread *iothread;
+    GMainContext *worker_context;
+    QEMUTimer *packet_check_timer;
+
+    QEMUBH *event_bh;
+    enum colo_event event;
+
+    QTAILQ_ENTRY(CompareState) next;
+};
+
+typedef struct CompareClass {
+    ObjectClass parent_class;
+} CompareClass;
+
 void colo_notify_compares_event(void *opaque, int event, Error **errp);
 void colo_compare_register_notifier(Notifier *notify);
 void colo_compare_unregister_notifier(Notifier *notify);
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH V8 5/6] net/colo-compare: Add passthrough list to CompareState
  2021-06-15 11:37 [PATCH V8 0/6] Passthrough specific network traffic in COLO Zhang Chen
                   ` (3 preceding siblings ...)
  2021-06-15 11:37 ` [PATCH V8 4/6] net/colo-compare: Move data structure and define to .h file Zhang Chen
@ 2021-06-15 11:37 ` Zhang Chen
  2021-06-15 11:37 ` [PATCH V8 6/6] net/net.c: Add handler for COLO passthrough connection Zhang Chen
  5 siblings, 0 replies; 25+ messages in thread
From: Zhang Chen @ 2021-06-15 11:37 UTC (permalink / raw)
  To: Jason Wang, qemu-dev, Eric Blake, Dr. David Alan Gilbert,
	Markus Armbruster, Daniel P. Berrangé,
	Gerd Hoffmann, Li Zhijian
  Cc: Zhang Chen, Lukas Straub, Zhang Chen

Add passthrough list for each CompareState.

Signed-off-by: Zhang Chen <chen.zhang@intel.com>
---
 net/colo-compare.c | 28 ++++++++++++++++++++++++++++
 net/colo-compare.h | 12 ++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index dcd24bb113..3fa108069d 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -161,6 +161,7 @@ static int packet_enqueue(CompareState *s, int mode, Connection **con)
     ConnectionKey key;
     Packet *pkt = NULL;
     Connection *conn;
+    COLOPassthroughEntry *pass, *next;
     int ret;
 
     if (mode == PRIMARY_IN) {
@@ -180,6 +181,31 @@ static int packet_enqueue(CompareState *s, int mode, Connection **con)
     }
     fill_connection_key(pkt, &key);
 
+    /* Check COLO passthrough specifications */
+    qemu_mutex_lock(&s->passthroughlist_mutex);
+    if (!QLIST_EMPTY(&s->passthroughlist)) {
+        QLIST_FOREACH_SAFE(pass, &s->passthroughlist, node, next) {
+            if (key.ip_proto == pass->l4_protocol->p_proto) {
+                if (pass->src_port == 0 || pass->src_port == key.dst_port) {
+                    if (pass->src_ip.s_addr == 0 ||
+                        pass->src_ip.s_addr == key.src.s_addr) {
+                        if (pass->dst_port == 0 ||
+                            pass->dst_port == key.src_port) {
+                            if (pass->dst_ip.s_addr == 0 ||
+                                pass->dst_ip.s_addr == key.dst.s_addr) {
+                                packet_destroy(pkt, NULL);
+                                pkt = NULL;
+                                qemu_mutex_unlock(&s->passthroughlist_mutex);
+                                return -1;
+                            }
+                        }
+                    }
+                }
+            }
+        }
+    }
+    qemu_mutex_unlock(&s->passthroughlist_mutex);
+
     conn = connection_get(s->connection_track_table,
                           &key,
                           &s->conn_list);
@@ -1232,6 +1258,7 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
     }
 
     g_queue_init(&s->conn_list);
+    QLIST_INIT(&s->passthroughlist);
 
     s->connection_track_table = g_hash_table_new_full(connection_key_hash,
                                                       connection_key_equal,
@@ -1246,6 +1273,7 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
         qemu_cond_init(&event_complete_cond);
         colo_compare_active = true;
     }
+    qemu_mutex_init(&s->passthroughlist_mutex);
     QTAILQ_INSERT_TAIL(&net_compares, s, next);
     qemu_mutex_unlock(&colo_compare_mutex);
 
diff --git a/net/colo-compare.h b/net/colo-compare.h
index 031b627a2f..519d266de3 100644
--- a/net/colo-compare.h
+++ b/net/colo-compare.h
@@ -23,6 +23,7 @@
 #include "migration/migration.h"
 #include "sysemu/iothread.h"
 #include "colo.h"
+#include <netdb.h>
 
 #define TYPE_COLO_COMPARE "colo-compare"
 typedef struct CompareState CompareState;
@@ -39,6 +40,15 @@ typedef struct COLOSendCo {
     int ret;
 } COLOSendCo;
 
+typedef struct COLOPassthroughEntry {
+    struct protoent *l4_protocol;
+    int src_port;
+    int dst_port;
+    struct in_addr src_ip;
+    struct in_addr dst_ip;
+    QLIST_ENTRY(COLOPassthroughEntry) node;
+} COLOPassthroughEntry;
+
 /*
  *  + CompareState ++
  *  |               |
@@ -95,6 +105,8 @@ struct CompareState {
 
     QEMUBH *event_bh;
     enum colo_event event;
+    QLIST_HEAD(, COLOPassthroughEntry) passthroughlist;
+    QemuMutex passthroughlist_mutex;
 
     QTAILQ_ENTRY(CompareState) next;
 };
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH V8 6/6] net/net.c: Add handler for COLO passthrough connection
  2021-06-15 11:37 [PATCH V8 0/6] Passthrough specific network traffic in COLO Zhang Chen
                   ` (4 preceding siblings ...)
  2021-06-15 11:37 ` [PATCH V8 5/6] net/colo-compare: Add passthrough list to CompareState Zhang Chen
@ 2021-06-15 11:37 ` Zhang Chen
  5 siblings, 0 replies; 25+ messages in thread
From: Zhang Chen @ 2021-06-15 11:37 UTC (permalink / raw)
  To: Jason Wang, qemu-dev, Eric Blake, Dr. David Alan Gilbert,
	Markus Armbruster, Daniel P. Berrangé,
	Gerd Hoffmann, Li Zhijian
  Cc: Zhang Chen, Lukas Straub, Zhang Chen

Use connection protocol,src port,dst port,src ip,dst ip as the key
to bypass certain network traffic in COLO compare.

Signed-off-by: Zhang Chen <chen.zhang@intel.com>
---
 net/net.c | 162 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 160 insertions(+), 2 deletions(-)

diff --git a/net/net.c b/net/net.c
index f913e97983..3a84ce7fa0 100644
--- a/net/net.c
+++ b/net/net.c
@@ -55,6 +55,8 @@
 #include "net/colo-compare.h"
 #include "net/filter.h"
 #include "qapi/string-output-visitor.h"
+#include "net/colo-compare.h"
+#include "qom/object_interfaces.h"
 
 /* Net bridge is currently not supported for W32. */
 #if !defined(_WIN32)
@@ -1195,14 +1197,170 @@ void qmp_netdev_del(const char *id, Error **errp)
     }
 }
 
+static CompareState *colo_passthrough_check(IPFlowSpec *spec, Error **errp)
+{
+    Object *container;
+    Object *obj;
+    CompareState *s;
+
+    if (!spec->object_name) {
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "object-name",
+                   "Need input colo-compare object name");
+        return NULL;
+    }
+
+    container = object_get_objects_root();
+    obj = object_resolve_path_component(container, spec->object_name);
+    if (!obj) {
+        error_setg(errp, "colo-compare '%s' not found", spec->object_name);
+        return NULL;
+    }
+
+    s = COLO_COMPARE(obj);
+
+    if (!getprotobyname(spec->protocol)) {
+        error_setg(errp, "COLO pass through get wrong protocol");
+        return NULL;
+    }
+
+    if ((spec->source->host && !qemu_isdigit(spec->source->host[0])) ||
+        (spec->destination->host &&
+        !qemu_isdigit(spec->destination->host[0]))) {
+        error_setg(errp, "COLO pass through get wrong IP");
+        return NULL;
+    }
+
+    if (atoi(spec->source->port) > 65536 || atoi(spec->source->port) < 0 ||
+        atoi(spec->destination->port) > 65536 ||
+        atoi(spec->destination->port) < 0) {
+        error_setg(errp, "COLO pass through get wrong port");
+        return NULL;
+    }
+
+    return s;
+}
+
+static COLOPassthroughEntry *compare_passthrough_find(CompareState *s,
+                                                      COLOPassthroughEntry *ent)
+{
+    COLOPassthroughEntry *next = NULL, *origin = NULL;
+
+    if (!QLIST_EMPTY(&s->passthroughlist)) {
+        QLIST_FOREACH_SAFE(origin, &s->passthroughlist, node, next) {
+            if ((ent->l4_protocol->p_proto == origin->l4_protocol->p_proto) &&
+                (ent->src_port == origin->src_port) &&
+                (ent->dst_port == origin->dst_port) &&
+                (ent->src_ip.s_addr == origin->src_ip.s_addr) &&
+                (ent->dst_ip.s_addr == origin->dst_ip.s_addr)) {
+                return origin;
+            }
+        }
+    }
+
+    return NULL;
+}
+
+static void compare_passthrough_add(CompareState *s,
+                                    IPFlowSpec *spec,
+                                    Error **errp)
+{
+    COLOPassthroughEntry *pass = NULL;
+
+    pass = g_new0(COLOPassthroughEntry, 1);
+
+    pass->l4_protocol = getprotobyname(spec->protocol);
+    pass->src_port = atoi(spec->source->port);
+    pass->dst_port = atoi(spec->destination->port);
+
+    if (!inet_aton(spec->source->host, &pass->src_ip)) {
+        pass->src_ip.s_addr = 0;
+    }
+
+    if (!inet_aton(spec->destination->host, &pass->dst_ip)) {
+        pass->dst_ip.s_addr = 0;
+    }
+
+    qemu_mutex_lock(&s->passthroughlist_mutex);
+    if (compare_passthrough_find(s, pass)) {
+        error_setg(errp, "The pass through connection already exists");
+        g_free(pass);
+        qemu_mutex_unlock(&s->passthroughlist_mutex);
+        return;
+    }
+
+    QLIST_INSERT_HEAD(&s->passthroughlist, pass, node);
+    qemu_mutex_unlock(&s->passthroughlist_mutex);
+}
+
+static void compare_passthrough_del(CompareState *s,
+                                    IPFlowSpec *spec,
+                                    Error **errp)
+{
+    COLOPassthroughEntry *pass = NULL, *result = NULL;
+
+    pass = g_new0(COLOPassthroughEntry, 1);
+
+    pass->l4_protocol = getprotobyname(spec->protocol);
+    pass->src_port = atoi(spec->source->port);
+    pass->dst_port = atoi(spec->destination->port);
+
+    if (!inet_aton(spec->source->host, &pass->src_ip)) {
+        pass->src_ip.s_addr = 0;
+    }
+
+    if (!inet_aton(spec->destination->host, &pass->dst_ip)) {
+        pass->dst_ip.s_addr = 0;
+    }
+
+    qemu_mutex_lock(&s->passthroughlist_mutex);
+
+    result = compare_passthrough_find(s, pass);
+    if (result) {
+        QLIST_REMOVE(result, node);
+        g_free(result);
+    } else {
+        error_setg(errp, "Can't find the IP flow Spec");
+    }
+
+    g_free(pass);
+    qemu_mutex_unlock(&s->passthroughlist_mutex);
+}
+
+
 void qmp_colo_passthrough_add(IPFlowSpec *spec, Error **errp)
 {
-    /* TODO implement setup passthrough rule */
+    CompareState *s;
+    Error *err = NULL;
+
+    s = colo_passthrough_check(spec, &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+
+    compare_passthrough_add(s, spec, &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
 }
 
 void qmp_colo_passthrough_del(IPFlowSpec *spec, Error **errp)
 {
-    /* TODO implement delete passthrough rule */
+    CompareState *s;
+    Error *err = NULL;
+
+    s = colo_passthrough_check(spec, &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+
+    compare_passthrough_del(s, spec, &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
 }
 
 static void netfilter_print_info(Monitor *mon, NetFilterState *nf)
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [PATCH V8 1/6] qapi/net: Add IPFlowSpec and QMP command for COLO passthrough
  2021-06-15 11:37 ` [PATCH V8 1/6] qapi/net: Add IPFlowSpec and QMP command for COLO passthrough Zhang Chen
@ 2021-06-15 14:43   ` Markus Armbruster
  2021-06-16  2:12     ` Zhang, Chen
  2021-06-21 11:30     ` Dr. David Alan Gilbert
  2021-06-15 15:01   ` Lukas Straub
  2021-06-22  7:05   ` Jason Wang
  2 siblings, 2 replies; 25+ messages in thread
From: Markus Armbruster @ 2021-06-15 14:43 UTC (permalink / raw)
  To: Zhang Chen
  Cc: Lukas Straub, Daniel P. Berrangé,
	Li Zhijian, Jason Wang, qemu-dev, Dr. David Alan Gilbert,
	Gerd Hoffmann, Zhang Chen, Eric Blake

Zhang Chen <chen.zhang@intel.com> writes:

> Since the real user scenario does not need COLO to monitor all traffic.
> Add colo-passthrough-add and colo-passthrough-del to maintain
> a COLO network passthrough list. Add IPFlowSpec struct for all QMP commands.
> All the fields of IPFlowSpec are optional.
>
> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> ---

The QAPI schema looks good to me, but the interface documentation is
still not quite clear enough.  To make progress, I'm going to make
concrete suggestions wherever I can despite being quite clueless about
the subject matter.  Risks me writing something that's clearer, but
wrong.  Keep that in mind, please.

>  net/net.c     | 10 +++++++
>  qapi/net.json | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 84 insertions(+)
>
> diff --git a/net/net.c b/net/net.c
> index 76bbb7c31b..f913e97983 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1195,6 +1195,16 @@ void qmp_netdev_del(const char *id, Error **errp)
>      }
>  }
>  
> +void qmp_colo_passthrough_add(IPFlowSpec *spec, Error **errp)
> +{
> +    /* TODO implement setup passthrough rule */
> +}
> +
> +void qmp_colo_passthrough_del(IPFlowSpec *spec, Error **errp)
> +{
> +    /* TODO implement delete passthrough rule */
> +}
> +
>  static void netfilter_print_info(Monitor *mon, NetFilterState *nf)
>  {
>      char *str;
> diff --git a/qapi/net.json b/qapi/net.json
> index 7fab2e7cd8..91f2e1495a 100644
> --- a/qapi/net.json
> +++ b/qapi/net.json
> @@ -7,6 +7,7 @@
>  ##
>  
>  { 'include': 'common.json' }
> +{ 'include': 'sockets.json' }
>  
>  ##
>  # @set_link:
> @@ -696,3 +697,76 @@
>  ##
>  { 'event': 'FAILOVER_NEGOTIATED',
>    'data': {'device-id': 'str'} }
> +
> +##
> +# @IPFlowSpec:
> +#
> +# IP flow specification.
> +#
> +# @protocol: Transport layer protocol like TCP/UDP, etc. The protocol is the
> +#            string instead of enum, because it can be passed to getprotobyname(3)
> +#            and avoid duplication with /etc/protocols.

The rationale is good, but it doesn't really belong into the interface
documentation.  Suggest:

   # @protocol: Transport layer protocol like TCP/UDP, etc.  This will be
   #            passed to getprotobyname(3).


> +#
> +# @object-name: The @object-name means packet handler in Qemu. Because not
> +#               all the network packet must pass the colo-compare module,
> +#               the net-filters are same situation. There modules attach to
> +#               netdev or chardev to work, VM can run multiple modules
> +#               at the same time. So it needs the object-name to set
> +#               the effective module.

I still don't understand this, and I'm too ignorant of COLO and
networking to suggest improvements.

Jason or David, perhaps?

> +#
> +# @source: Source address and port.
> +#
> +# @destination: Destination address and port.
> +#
> +# Since: 6.1
> +##
> +{ 'struct': 'IPFlowSpec',
> +  'data': { '*protocol': 'str', '*object-name': 'str',
> +    '*source': 'InetSocketAddressBase',
> +    '*destination': 'InetSocketAddressBase' } }
> +
> +##
> +# @colo-passthrough-add:
> +#
> +# Add passthrough entry IPFlowSpec to the COLO-compare instance.
> +# The protocol and source/destination IP/ports are optional. if the user
> +# only inputs part of the information, this will match all traffic.

Actually, all arguments are optional.

Suggest:

   # Add an entry to the COLO network passthrough list.
   # Absent protocol, host addresses and ports match anything.

If there is more than one such list, then "to a COLO network passthrough
list" instead.

Still missing then: meaning of absent @object-name.  Does it select the
COLO network passthrough list, perhaps?

> +#
> +# Returns: Nothing on success
> +#
> +# Since: 6.1
> +#
> +# Example:
> +#
> +# -> { "execute": "colo-passthrough-add",
> +#      "arguments": { "protocol": "tcp", "object-name": "object0",
> +#      "source": {"host": "192.168.1.1", "port": "1234"},
> +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
> +# <- { "return": {} }
> +#
> +##
> +{ 'command': 'colo-passthrough-add', 'boxed': true,
> +     'data': 'IPFlowSpec' }
> +
> +##
> +# @colo-passthrough-del:
> +#
> +# Delete passthrough entry IPFlowSpec to the COLO-compare instance.
> +# The protocol and source/destination IP/ports are optional. if the user
> +# only inputs part of the information, this will match all traffic.

I suspect this command doesn't actually match traffic, it matches
entries added with colo-passthrough-add.

Can it delete more than one such entry?

Suggest:

   # Delete an entry from the COLO network passthrough list.

and then explain how the command arguments select entries.

> +#
> +# Returns: Nothing on success
> +#
> +# Since: 6.1
> +#
> +# Example:
> +#
> +# -> { "execute": "colo-passthrough-del",
> +#      "arguments": { "protocol": "tcp", "object-name": "object0",
> +#      "source": {"host": "192.168.1.1", "port": "1234"},
> +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
> +# <- { "return": {} }
> +#
> +##
> +{ 'command': 'colo-passthrough-del', 'boxed': true,
> +     'data': 'IPFlowSpec' }



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH V8 1/6] qapi/net: Add IPFlowSpec and QMP command for COLO passthrough
  2021-06-15 11:37 ` [PATCH V8 1/6] qapi/net: Add IPFlowSpec and QMP command for COLO passthrough Zhang Chen
  2021-06-15 14:43   ` Markus Armbruster
@ 2021-06-15 15:01   ` Lukas Straub
  2021-06-16  1:20     ` Zhang, Chen
  2021-06-22  7:05   ` Jason Wang
  2 siblings, 1 reply; 25+ messages in thread
From: Lukas Straub @ 2021-06-15 15:01 UTC (permalink / raw)
  To: Zhang Chen
  Cc: Daniel P. Berrangé,
	Li Zhijian, Jason Wang, Markus Armbruster, qemu-dev,
	Gerd Hoffmann, Zhang Chen, Eric Blake, Dr. David Alan Gilbert

[-- Attachment #1: Type: text/plain, Size: 4106 bytes --]

On Tue, 15 Jun 2021 19:37:35 +0800
Zhang Chen <chen.zhang@intel.com> wrote:

> Since the real user scenario does not need COLO to monitor all traffic.
> Add colo-passthrough-add and colo-passthrough-del to maintain
> a COLO network passthrough list. Add IPFlowSpec struct for all QMP commands.
> All the fields of IPFlowSpec are optional.
> 
> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> ---
>  net/net.c     | 10 +++++++
>  qapi/net.json | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 84 insertions(+)
> 
> diff --git a/net/net.c b/net/net.c
> index 76bbb7c31b..f913e97983 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1195,6 +1195,16 @@ void qmp_netdev_del(const char *id, Error **errp)
>      }
>  }
>  
> +void qmp_colo_passthrough_add(IPFlowSpec *spec, Error **errp)
> +{
> +    /* TODO implement setup passthrough rule */
> +}
> +
> +void qmp_colo_passthrough_del(IPFlowSpec *spec, Error **errp)
> +{
> +    /* TODO implement delete passthrough rule */
> +}
> +
>  static void netfilter_print_info(Monitor *mon, NetFilterState *nf)
>  {
>      char *str;
> diff --git a/qapi/net.json b/qapi/net.json
> index 7fab2e7cd8..91f2e1495a 100644
> --- a/qapi/net.json
> +++ b/qapi/net.json
> @@ -7,6 +7,7 @@
>  ##
>  
>  { 'include': 'common.json' }
> +{ 'include': 'sockets.json' }
>  
>  ##
>  # @set_link:
> @@ -696,3 +697,76 @@
>  ##
>  { 'event': 'FAILOVER_NEGOTIATED',
>    'data': {'device-id': 'str'} }
> +
> +##
> +# @IPFlowSpec:
> +#
> +# IP flow specification.
> +#
> +# @protocol: Transport layer protocol like TCP/UDP, etc. The protocol is the
> +#            string instead of enum, because it can be passed to getprotobyname(3)
> +#            and avoid duplication with /etc/protocols.
> +#
> +# @object-name: The @object-name means packet handler in Qemu. Because not
> +#               all the network packet must pass the colo-compare module,
> +#               the net-filters are same situation. There modules attach to
> +#               netdev or chardev to work, VM can run multiple modules
> +#               at the same time. So it needs the object-name to set
> +#               the effective module.

Again: "@object-name: The id of the colo-compare object to add the filter to."

> +# @source: Source address and port.
> +#
> +# @destination: Destination address and port.
> +#
> +# Since: 6.1
> +##
> +{ 'struct': 'IPFlowSpec',
> +  'data': { '*protocol': 'str', '*object-name': 'str',
> +    '*source': 'InetSocketAddressBase',
> +    '*destination': 'InetSocketAddressBase' } }
> +
> +##
> +# @colo-passthrough-add:
> +#
> +# Add passthrough entry IPFlowSpec to the COLO-compare instance.
> +# The protocol and source/destination IP/ports are optional. if the user
> +# only inputs part of the information, this will match all traffic.
> +#
> +# Returns: Nothing on success
> +#
> +# Since: 6.1
> +#
> +# Example:
> +#
> +# -> { "execute": "colo-passthrough-add",
> +#      "arguments": { "protocol": "tcp", "object-name": "object0",
> +#      "source": {"host": "192.168.1.1", "port": "1234"},
> +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
> +# <- { "return": {} }
> +#
> +##
> +{ 'command': 'colo-passthrough-add', 'boxed': true,
> +     'data': 'IPFlowSpec' }
> +
> +##
> +# @colo-passthrough-del:
> +#
> +# Delete passthrough entry IPFlowSpec to the COLO-compare instance.
> +# The protocol and source/destination IP/ports are optional. if the user
> +# only inputs part of the information, this will match all traffic.
> +#
> +# Returns: Nothing on success
> +#
> +# Since: 6.1
> +#
> +# Example:
> +#
> +# -> { "execute": "colo-passthrough-del",
> +#      "arguments": { "protocol": "tcp", "object-name": "object0",
> +#      "source": {"host": "192.168.1.1", "port": "1234"},
> +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
> +# <- { "return": {} }
> +#
> +##
> +{ 'command': 'colo-passthrough-del', 'boxed': true,
> +     'data': 'IPFlowSpec' }



-- 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 25+ messages in thread

* RE: [PATCH V8 1/6] qapi/net: Add IPFlowSpec and QMP command for COLO passthrough
  2021-06-15 15:01   ` Lukas Straub
@ 2021-06-16  1:20     ` Zhang, Chen
  0 siblings, 0 replies; 25+ messages in thread
From: Zhang, Chen @ 2021-06-16  1:20 UTC (permalink / raw)
  To: Lukas Straub
  Cc: Daniel P. Berrangé,
	Li Zhijian, Jason Wang, Markus Armbruster, qemu-dev,
	Gerd Hoffmann, Zhang Chen, Eric Blake, Dr. David Alan Gilbert



> -----Original Message-----
> From: Lukas Straub <lukasstraub2@web.de>
> Sent: Tuesday, June 15, 2021 11:02 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: Jason Wang <jasowang@redhat.com>; qemu-dev <qemu-
> devel@nongnu.org>; Eric Blake <eblake@redhat.com>; Dr. David Alan
> Gilbert <dgilbert@redhat.com>; Markus Armbruster <armbru@redhat.com>;
> Daniel P. Berrangé <berrange@redhat.com>; Gerd Hoffmann
> <kraxel@redhat.com>; Li Zhijian <lizhijian@cn.fujitsu.com>; Zhang Chen
> <zhangckid@gmail.com>
> Subject: Re: [PATCH V8 1/6] qapi/net: Add IPFlowSpec and QMP command
> for COLO passthrough
> 
> On Tue, 15 Jun 2021 19:37:35 +0800
> Zhang Chen <chen.zhang@intel.com> wrote:
> 
> > Since the real user scenario does not need COLO to monitor all traffic.
> > Add colo-passthrough-add and colo-passthrough-del to maintain a COLO
> > network passthrough list. Add IPFlowSpec struct for all QMP commands.
> > All the fields of IPFlowSpec are optional.
> >
> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> > ---
> >  net/net.c     | 10 +++++++
> >  qapi/net.json | 74
> > +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 84 insertions(+)
> >
> > diff --git a/net/net.c b/net/net.c
> > index 76bbb7c31b..f913e97983 100644
> > --- a/net/net.c
> > +++ b/net/net.c
> > @@ -1195,6 +1195,16 @@ void qmp_netdev_del(const char *id, Error
> **errp)
> >      }
> >  }
> >
> > +void qmp_colo_passthrough_add(IPFlowSpec *spec, Error **errp) {
> > +    /* TODO implement setup passthrough rule */ }
> > +
> > +void qmp_colo_passthrough_del(IPFlowSpec *spec, Error **errp) {
> > +    /* TODO implement delete passthrough rule */ }
> > +
> >  static void netfilter_print_info(Monitor *mon, NetFilterState *nf)  {
> >      char *str;
> > diff --git a/qapi/net.json b/qapi/net.json index
> > 7fab2e7cd8..91f2e1495a 100644
> > --- a/qapi/net.json
> > +++ b/qapi/net.json
> > @@ -7,6 +7,7 @@
> >  ##
> >
> >  { 'include': 'common.json' }
> > +{ 'include': 'sockets.json' }
> >
> >  ##
> >  # @set_link:
> > @@ -696,3 +697,76 @@
> >  ##
> >  { 'event': 'FAILOVER_NEGOTIATED',
> >    'data': {'device-id': 'str'} }
> > +
> > +##
> > +# @IPFlowSpec:
> > +#
> > +# IP flow specification.
> > +#
> > +# @protocol: Transport layer protocol like TCP/UDP, etc. The protocol is
> the
> > +#            string instead of enum, because it can be passed to
> getprotobyname(3)
> > +#            and avoid duplication with /etc/protocols.
> > +#
> > +# @object-name: The @object-name means packet handler in Qemu.
> Because not
> > +#               all the network packet must pass the colo-compare module,
> > +#               the net-filters are same situation. There modules attach to
> > +#               netdev or chardev to work, VM can run multiple modules
> > +#               at the same time. So it needs the object-name to set
> > +#               the effective module.
> 
> Again: "@object-name: The id of the colo-compare object to add the filter
> to."

According to the previous discussion,
https://lists.gnu.org/archive/html/qemu-devel/2021-04/msg01088.html

In order to avoid misunderstanding looks use "@object-name: The name of the colo-compare object to add the filter " is better?

Thanks
Chen


> 
> > +# @source: Source address and port.
> > +#
> > +# @destination: Destination address and port.
> > +#
> > +# Since: 6.1
> > +##
> > +{ 'struct': 'IPFlowSpec',
> > +  'data': { '*protocol': 'str', '*object-name': 'str',
> > +    '*source': 'InetSocketAddressBase',
> > +    '*destination': 'InetSocketAddressBase' } }
> > +
> > +##
> > +# @colo-passthrough-add:
> > +#
> > +# Add passthrough entry IPFlowSpec to the COLO-compare instance.
> > +# The protocol and source/destination IP/ports are optional. if the
> > +user # only inputs part of the information, this will match all traffic.
> > +#
> > +# Returns: Nothing on success
> > +#
> > +# Since: 6.1
> > +#
> > +# Example:
> > +#
> > +# -> { "execute": "colo-passthrough-add",
> > +#      "arguments": { "protocol": "tcp", "object-name": "object0",
> > +#      "source": {"host": "192.168.1.1", "port": "1234"},
> > +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
> > +# <- { "return": {} }
> > +#
> > +##
> > +{ 'command': 'colo-passthrough-add', 'boxed': true,
> > +     'data': 'IPFlowSpec' }
> > +
> > +##
> > +# @colo-passthrough-del:
> > +#
> > +# Delete passthrough entry IPFlowSpec to the COLO-compare instance.
> > +# The protocol and source/destination IP/ports are optional. if the
> > +user # only inputs part of the information, this will match all traffic.
> > +#
> > +# Returns: Nothing on success
> > +#
> > +# Since: 6.1
> > +#
> > +# Example:
> > +#
> > +# -> { "execute": "colo-passthrough-del",
> > +#      "arguments": { "protocol": "tcp", "object-name": "object0",
> > +#      "source": {"host": "192.168.1.1", "port": "1234"},
> > +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
> > +# <- { "return": {} }
> > +#
> > +##
> > +{ 'command': 'colo-passthrough-del', 'boxed': true,
> > +     'data': 'IPFlowSpec' }
> 
> 
> 
> --



^ permalink raw reply	[flat|nested] 25+ messages in thread

* RE: [PATCH V8 1/6] qapi/net: Add IPFlowSpec and QMP command for COLO passthrough
  2021-06-15 14:43   ` Markus Armbruster
@ 2021-06-16  2:12     ` Zhang, Chen
  2021-06-16  6:04       ` Markus Armbruster
  2021-06-21 11:30     ` Dr. David Alan Gilbert
  1 sibling, 1 reply; 25+ messages in thread
From: Zhang, Chen @ 2021-06-16  2:12 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Lukas Straub, Daniel P.Berrangé,
	Li Zhijian, Jason Wang, qemu-dev, Dr. David Alan Gilbert,
	Gerd Hoffmann, Zhang Chen, Eric Blake



> -----Original Message-----
> From: Markus Armbruster <armbru@redhat.com>
> Sent: Tuesday, June 15, 2021 10:43 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: Jason Wang <jasowang@redhat.com>; qemu-dev <qemu-
> devel@nongnu.org>; Eric Blake <eblake@redhat.com>; Dr. David Alan
> Gilbert <dgilbert@redhat.com>; Daniel P.Berrangé <berrange@redhat.com>;
> Gerd Hoffmann <kraxel@redhat.com>; Li Zhijian <lizhijian@cn.fujitsu.com>;
> Lukas Straub <lukasstraub2@web.de>; Zhang Chen <zhangckid@gmail.com>
> Subject: Re: [PATCH V8 1/6] qapi/net: Add IPFlowSpec and QMP command
> for COLO passthrough
> 
> Zhang Chen <chen.zhang@intel.com> writes:
> 
> > Since the real user scenario does not need COLO to monitor all traffic.
> > Add colo-passthrough-add and colo-passthrough-del to maintain a COLO
> > network passthrough list. Add IPFlowSpec struct for all QMP commands.
> > All the fields of IPFlowSpec are optional.
> >
> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> > ---
> 
> The QAPI schema looks good to me, but the interface documentation is still
> not quite clear enough.  To make progress, I'm going to make concrete
> suggestions wherever I can despite being quite clueless about the subject
> matter.  Risks me writing something that's clearer, but wrong.  Keep that in
> mind, please.
> 
> >  net/net.c     | 10 +++++++
> >  qapi/net.json | 74
> > +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 84 insertions(+)
> >
> > diff --git a/net/net.c b/net/net.c
> > index 76bbb7c31b..f913e97983 100644
> > --- a/net/net.c
> > +++ b/net/net.c
> > @@ -1195,6 +1195,16 @@ void qmp_netdev_del(const char *id, Error
> **errp)
> >      }
> >  }
> >
> > +void qmp_colo_passthrough_add(IPFlowSpec *spec, Error **errp) {
> > +    /* TODO implement setup passthrough rule */ }
> > +
> > +void qmp_colo_passthrough_del(IPFlowSpec *spec, Error **errp) {
> > +    /* TODO implement delete passthrough rule */ }
> > +
> >  static void netfilter_print_info(Monitor *mon, NetFilterState *nf)  {
> >      char *str;
> > diff --git a/qapi/net.json b/qapi/net.json index
> > 7fab2e7cd8..91f2e1495a 100644
> > --- a/qapi/net.json
> > +++ b/qapi/net.json
> > @@ -7,6 +7,7 @@
> >  ##
> >
> >  { 'include': 'common.json' }
> > +{ 'include': 'sockets.json' }
> >
> >  ##
> >  # @set_link:
> > @@ -696,3 +697,76 @@
> >  ##
> >  { 'event': 'FAILOVER_NEGOTIATED',
> >    'data': {'device-id': 'str'} }
> > +
> > +##
> > +# @IPFlowSpec:
> > +#
> > +# IP flow specification.
> > +#
> > +# @protocol: Transport layer protocol like TCP/UDP, etc. The protocol is
> the
> > +#            string instead of enum, because it can be passed to
> getprotobyname(3)
> > +#            and avoid duplication with /etc/protocols.
> 
> The rationale is good, but it doesn't really belong into the interface
> documentation.  Suggest:
> 
>    # @protocol: Transport layer protocol like TCP/UDP, etc.  This will be
>    #            passed to getprotobyname(3).
> 

OK.

> 
> > +#
> > +# @object-name: The @object-name means packet handler in Qemu.
> Because not
> > +#               all the network packet must pass the colo-compare module,
> > +#               the net-filters are same situation. There modules attach to
> > +#               netdev or chardev to work, VM can run multiple modules
> > +#               at the same time. So it needs the object-name to set
> > +#               the effective module.
> 
> I still don't understand this, and I'm too ignorant of COLO and networking to
> suggest improvements.

Let me use qemu boot parameter to clear it.
For colo-compare, it needs chardev as the source to handle network packet.
-object colo-compare,id=comp0,primary_in=chardev-input0,secondary_in=chardev-input1,outdev=chardev-output0,iothread=iothread0.

For net filters, it needs attached on netdev.
-object filter-redirector,id=red0,netdev=hn0,queue=rx,outdev=chardev-output1
-object filter-mirror,id=mirror0,netdev=hn0,queue=rx,outdev=chardev-output2

And we can use -chardev socket combine the filter and the colo-compare.

Back to the @object-name, One guest maybe have multi colo-compare as the same time, with different object name from different source.
So we need assign the IPFlowSpec to one object as the handler. Same as the net-filters.
Each object instance has its own passthrough list.


> 
> Jason or David, perhaps?
> 
> > +#
> > +# @source: Source address and port.
> > +#
> > +# @destination: Destination address and port.
> > +#
> > +# Since: 6.1
> > +##
> > +{ 'struct': 'IPFlowSpec',
> > +  'data': { '*protocol': 'str', '*object-name': 'str',
> > +    '*source': 'InetSocketAddressBase',
> > +    '*destination': 'InetSocketAddressBase' } }
> > +
> > +##
> > +# @colo-passthrough-add:
> > +#
> > +# Add passthrough entry IPFlowSpec to the COLO-compare instance.
> > +# The protocol and source/destination IP/ports are optional. if the
> > +user # only inputs part of the information, this will match all traffic.
> 
> Actually, all arguments are optional.
> 
> Suggest:
> 
>    # Add an entry to the COLO network passthrough list.
>    # Absent protocol, host addresses and ports match anything.
> 
> If there is more than one such list, then "to a COLO network passthrough list"
> instead.

Yes, more than one list.

> 
> Still missing then: meaning of absent @object-name.  Does it select the COLO
> network passthrough list, perhaps?

Yes, Please see the explanation above. Each object instance has its own passthrough list.

> 
> > +#
> > +# Returns: Nothing on success
> > +#
> > +# Since: 6.1
> > +#
> > +# Example:
> > +#
> > +# -> { "execute": "colo-passthrough-add",
> > +#      "arguments": { "protocol": "tcp", "object-name": "object0",
> > +#      "source": {"host": "192.168.1.1", "port": "1234"},
> > +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
> > +# <- { "return": {} }
> > +#
> > +##
> > +{ 'command': 'colo-passthrough-add', 'boxed': true,
> > +     'data': 'IPFlowSpec' }
> > +
> > +##
> > +# @colo-passthrough-del:
> > +#
> > +# Delete passthrough entry IPFlowSpec to the COLO-compare instance.
> > +# The protocol and source/destination IP/ports are optional. if the
> > +user # only inputs part of the information, this will match all traffic.
> 
> I suspect this command doesn't actually match traffic, it matches entries
> added with colo-passthrough-add.

Yes.

> 
> Can it delete more than one such entry?
> 

Currently no, but it easy to match one more entry to delete.

> Suggest:
> 
>    # Delete an entry from the COLO network passthrough list.
> 
> and then explain how the command arguments select entries.

Search the object's passthrough list, if find same entry,  delete it.

Thanks
Chen

> 
> > +#
> > +# Returns: Nothing on success
> > +#
> > +# Since: 6.1
> > +#
> > +# Example:
> > +#
> > +# -> { "execute": "colo-passthrough-del",
> > +#      "arguments": { "protocol": "tcp", "object-name": "object0",
> > +#      "source": {"host": "192.168.1.1", "port": "1234"},
> > +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
> > +# <- { "return": {} }
> > +#
> > +##
> > +{ 'command': 'colo-passthrough-del', 'boxed': true,
> > +     'data': 'IPFlowSpec' }



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH V8 1/6] qapi/net: Add IPFlowSpec and QMP command for COLO passthrough
  2021-06-16  2:12     ` Zhang, Chen
@ 2021-06-16  6:04       ` Markus Armbruster
  2021-06-16  6:45         ` Zhang, Chen
  0 siblings, 1 reply; 25+ messages in thread
From: Markus Armbruster @ 2021-06-16  6:04 UTC (permalink / raw)
  To: Zhang, Chen
  Cc: Lukas Straub, Daniel P.Berrangé,
	Li Zhijian, Jason Wang, qemu-dev, Dr. David Alan Gilbert,
	Gerd Hoffmann, Zhang Chen, Eric Blake

"Zhang, Chen" <chen.zhang@intel.com> writes:

>> -----Original Message-----
>> From: Markus Armbruster <armbru@redhat.com>
>> Sent: Tuesday, June 15, 2021 10:43 PM
>> To: Zhang, Chen <chen.zhang@intel.com>
>> Cc: Jason Wang <jasowang@redhat.com>; qemu-dev <qemu-
>> devel@nongnu.org>; Eric Blake <eblake@redhat.com>; Dr. David Alan
>> Gilbert <dgilbert@redhat.com>; Daniel P.Berrangé <berrange@redhat.com>;
>> Gerd Hoffmann <kraxel@redhat.com>; Li Zhijian <lizhijian@cn.fujitsu.com>;
>> Lukas Straub <lukasstraub2@web.de>; Zhang Chen <zhangckid@gmail.com>
>> Subject: Re: [PATCH V8 1/6] qapi/net: Add IPFlowSpec and QMP command
>> for COLO passthrough
>> 
>> Zhang Chen <chen.zhang@intel.com> writes:
>> 
>> > Since the real user scenario does not need COLO to monitor all traffic.
>> > Add colo-passthrough-add and colo-passthrough-del to maintain a COLO
>> > network passthrough list. Add IPFlowSpec struct for all QMP commands.
>> > All the fields of IPFlowSpec are optional.
>> >
>> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
>> > ---
>> 
>> The QAPI schema looks good to me, but the interface documentation is still
>> not quite clear enough.  To make progress, I'm going to make concrete
>> suggestions wherever I can despite being quite clueless about the subject
>> matter.  Risks me writing something that's clearer, but wrong.  Keep that in
>> mind, please.
>> 
>> >  net/net.c     | 10 +++++++
>> >  qapi/net.json | 74
>> > +++++++++++++++++++++++++++++++++++++++++++++++++++
>> >  2 files changed, 84 insertions(+)
>> >
>> > diff --git a/net/net.c b/net/net.c
>> > index 76bbb7c31b..f913e97983 100644
>> > --- a/net/net.c
>> > +++ b/net/net.c
>> > @@ -1195,6 +1195,16 @@ void qmp_netdev_del(const char *id, Error **errp)
>> >      }
>> >  }
>> >
>> > +void qmp_colo_passthrough_add(IPFlowSpec *spec, Error **errp) {
>> > +    /* TODO implement setup passthrough rule */ }
>> > +
>> > +void qmp_colo_passthrough_del(IPFlowSpec *spec, Error **errp) {
>> > +    /* TODO implement delete passthrough rule */ }
>> > +
>> >  static void netfilter_print_info(Monitor *mon, NetFilterState *nf)  {
>> >      char *str;
>> > diff --git a/qapi/net.json b/qapi/net.json index
>> > 7fab2e7cd8..91f2e1495a 100644
>> > --- a/qapi/net.json
>> > +++ b/qapi/net.json
>> > @@ -7,6 +7,7 @@
>> >  ##
>> >
>> >  { 'include': 'common.json' }
>> > +{ 'include': 'sockets.json' }
>> >
>> >  ##
>> >  # @set_link:
>> > @@ -696,3 +697,76 @@
>> >  ##
>> >  { 'event': 'FAILOVER_NEGOTIATED',
>> >    'data': {'device-id': 'str'} }
>> > +
>> > +##
>> > +# @IPFlowSpec:
>> > +#
>> > +# IP flow specification.
>> > +#
>> > +# @protocol: Transport layer protocol like TCP/UDP, etc. The protocol is the
>> > +#            string instead of enum, because it can be passed to getprotobyname(3)
>> > +#            and avoid duplication with /etc/protocols.
>> 
>> The rationale is good, but it doesn't really belong into the interface
>> documentation.  Suggest:
>> 
>>    # @protocol: Transport layer protocol like TCP/UDP, etc.  This will be
>>    #            passed to getprotobyname(3).
>> 
>
> OK.
>
>> 
>> > +#
>> > +# @object-name: The @object-name means packet handler in Qemu. Because not
>> > +#               all the network packet must pass the colo-compare module,
>> > +#               the net-filters are same situation. There modules attach to
>> > +#               netdev or chardev to work, VM can run multiple modules
>> > +#               at the same time. So it needs the object-name to set
>> > +#               the effective module.
>> 
>> I still don't understand this, and I'm too ignorant of COLO and networking to
>> suggest improvements.
>
> Let me use qemu boot parameter to clear it.
> For colo-compare, it needs chardev as the source to handle network packet.
> -object colo-compare,id=comp0,primary_in=chardev-input0,secondary_in=chardev-input1,outdev=chardev-output0,iothread=iothread0.
>
> For net filters, it needs attached on netdev.
> -object filter-redirector,id=red0,netdev=hn0,queue=rx,outdev=chardev-output1
> -object filter-mirror,id=mirror0,netdev=hn0,queue=rx,outdev=chardev-output2
>
> And we can use -chardev socket combine the filter and the colo-compare.
>
> Back to the @object-name, One guest maybe have multi colo-compare as the same time, with different object name from different source.
> So we need assign the IPFlowSpec to one object as the handler. Same as the net-filters.
> Each object instance has its own passthrough list.

So the @object-name here references one of the "packet handler objects"
(colo-compare, filter-redirector, filter-mirror) by @id.  Correct?

In other words, @object-name is the ID of a QOM object, and the QOM
object must be of a certain kind (i.e. provide certain functionality).
Correct?

What exactly makes a QOM object a "packet handler object?"

Right now, the packet handler object types are colo-compare,
filter-redirector, filter-mirror, and that's all.  Correct?

Another question the doc comment needs to answer: what happens when
@object-name is absent?

>> Jason or David, perhaps?
>> 
>> > +#
>> > +# @source: Source address and port.
>> > +#
>> > +# @destination: Destination address and port.
>> > +#
>> > +# Since: 6.1
>> > +##
>> > +{ 'struct': 'IPFlowSpec',
>> > +  'data': { '*protocol': 'str', '*object-name': 'str',
>> > +    '*source': 'InetSocketAddressBase',
>> > +    '*destination': 'InetSocketAddressBase' } }
>> > +
>> > +##
>> > +# @colo-passthrough-add:
>> > +#
>> > +# Add passthrough entry IPFlowSpec to the COLO-compare instance.
>> > +# The protocol and source/destination IP/ports are optional. if the
>> > +user # only inputs part of the information, this will match all traffic.
>> 
>> Actually, all arguments are optional.
>> 
>> Suggest:
>> 
>>    # Add an entry to the COLO network passthrough list.
>>    # Absent protocol, host addresses and ports match anything.
>> 
>> If there is more than one such list, then "to a COLO network passthrough list"
>> instead.
>
> Yes, more than one list.
>
>> 
>> Still missing then: meaning of absent @object-name.  Does it select the COLO
>> network passthrough list, perhaps?
>
> Yes, Please see the explanation above. Each object instance has its own passthrough list.

Got it now.

>> > +#
>> > +# Returns: Nothing on success
>> > +#
>> > +# Since: 6.1
>> > +#
>> > +# Example:
>> > +#
>> > +# -> { "execute": "colo-passthrough-add",
>> > +#      "arguments": { "protocol": "tcp", "object-name": "object0",
>> > +#      "source": {"host": "192.168.1.1", "port": "1234"},
>> > +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
>> > +# <- { "return": {} }
>> > +#
>> > +##
>> > +{ 'command': 'colo-passthrough-add', 'boxed': true,
>> > +     'data': 'IPFlowSpec' }
>> > +
>> > +##
>> > +# @colo-passthrough-del:
>> > +#
>> > +# Delete passthrough entry IPFlowSpec to the COLO-compare instance.
>> > +# The protocol and source/destination IP/ports are optional. if the
>> > +user # only inputs part of the information, this will match all traffic.
>> 
>> I suspect this command doesn't actually match traffic, it matches entries
>> added with colo-passthrough-add.
>
> Yes.
>
>> 
>> Can it delete more than one such entry?
>> 
>
> Currently no, but it easy to match one more entry to delete.

If the passthrough list entries had some unique ID, we could refer to
one entry by its ID.  It's how things commonly work.

Without an ID, we need to match by value, like you do.  I can see three
possible behaviors:

1. Select first entry that matches.

2. Select all entries that match.

3. If exactly one entry matches, select it.

The second design choice is behavior when nothing gets selected:

a. Silently do nothing

b. Error

Which one did you implement?  My guess based on your answers is 1a.

>> Suggest:
>> 
>>    # Delete an entry from the COLO network passthrough list.
>> 
>> and then explain how the command arguments select entries.
>
> Search the object's passthrough list, if find same entry,  delete it.
>
> Thanks
> Chen
>
>> 
>> > +#
>> > +# Returns: Nothing on success
>> > +#
>> > +# Since: 6.1
>> > +#
>> > +# Example:
>> > +#
>> > +# -> { "execute": "colo-passthrough-del",
>> > +#      "arguments": { "protocol": "tcp", "object-name": "object0",
>> > +#      "source": {"host": "192.168.1.1", "port": "1234"},
>> > +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
>> > +# <- { "return": {} }
>> > +#
>> > +##
>> > +{ 'command': 'colo-passthrough-del', 'boxed': true,
>> > +     'data': 'IPFlowSpec' }



^ permalink raw reply	[flat|nested] 25+ messages in thread

* RE: [PATCH V8 1/6] qapi/net: Add IPFlowSpec and QMP command for COLO passthrough
  2021-06-16  6:04       ` Markus Armbruster
@ 2021-06-16  6:45         ` Zhang, Chen
  2021-06-16 13:26           ` Markus Armbruster
  0 siblings, 1 reply; 25+ messages in thread
From: Zhang, Chen @ 2021-06-16  6:45 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Lukas Straub, Daniel P.Berrangé,
	Li Zhijian, Jason Wang, qemu-dev, Dr. David Alan Gilbert,
	Gerd Hoffmann, Zhang Chen, Eric Blake



> -----Original Message-----
> From: Markus Armbruster <armbru@redhat.com>
> Sent: Wednesday, June 16, 2021 2:04 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: Lukas Straub <lukasstraub2@web.de>; Daniel P.Berrangé
> <berrange@redhat.com>; Li Zhijian <lizhijian@cn.fujitsu.com>; Jason Wang
> <jasowang@redhat.com>; qemu-dev <qemu-devel@nongnu.org>; Dr. David
> Alan Gilbert <dgilbert@redhat.com>; Gerd Hoffmann <kraxel@redhat.com>;
> Zhang Chen <zhangckid@gmail.com>; Eric Blake <eblake@redhat.com>
> Subject: Re: [PATCH V8 1/6] qapi/net: Add IPFlowSpec and QMP command
> for COLO passthrough
> 
> "Zhang, Chen" <chen.zhang@intel.com> writes:
> 
> >> -----Original Message-----
> >> From: Markus Armbruster <armbru@redhat.com>
> >> Sent: Tuesday, June 15, 2021 10:43 PM
> >> To: Zhang, Chen <chen.zhang@intel.com>
> >> Cc: Jason Wang <jasowang@redhat.com>; qemu-dev <qemu-
> >> devel@nongnu.org>; Eric Blake <eblake@redhat.com>; Dr. David Alan
> >> Gilbert <dgilbert@redhat.com>; Daniel P.Berrangé
> >> <berrange@redhat.com>; Gerd Hoffmann <kraxel@redhat.com>; Li
> Zhijian
> >> <lizhijian@cn.fujitsu.com>; Lukas Straub <lukasstraub2@web.de>; Zhang
> >> Chen <zhangckid@gmail.com>
> >> Subject: Re: [PATCH V8 1/6] qapi/net: Add IPFlowSpec and QMP
> command
> >> for COLO passthrough
> >>
> >> Zhang Chen <chen.zhang@intel.com> writes:
> >>
> >> > Since the real user scenario does not need COLO to monitor all traffic.
> >> > Add colo-passthrough-add and colo-passthrough-del to maintain a
> >> > COLO network passthrough list. Add IPFlowSpec struct for all QMP
> commands.
> >> > All the fields of IPFlowSpec are optional.
> >> >
> >> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> >> > ---
> >>
> >> The QAPI schema looks good to me, but the interface documentation is
> >> still not quite clear enough.  To make progress, I'm going to make
> >> concrete suggestions wherever I can despite being quite clueless
> >> about the subject matter.  Risks me writing something that's clearer,
> >> but wrong.  Keep that in mind, please.
> >>
> >> >  net/net.c     | 10 +++++++
> >> >  qapi/net.json | 74
> >> > +++++++++++++++++++++++++++++++++++++++++++++++++++
> >> >  2 files changed, 84 insertions(+)
> >> >
> >> > diff --git a/net/net.c b/net/net.c
> >> > index 76bbb7c31b..f913e97983 100644
> >> > --- a/net/net.c
> >> > +++ b/net/net.c
> >> > @@ -1195,6 +1195,16 @@ void qmp_netdev_del(const char *id, Error
> **errp)
> >> >      }
> >> >  }
> >> >
> >> > +void qmp_colo_passthrough_add(IPFlowSpec *spec, Error **errp) {
> >> > +    /* TODO implement setup passthrough rule */ }
> >> > +
> >> > +void qmp_colo_passthrough_del(IPFlowSpec *spec, Error **errp) {
> >> > +    /* TODO implement delete passthrough rule */ }
> >> > +
> >> >  static void netfilter_print_info(Monitor *mon, NetFilterState *nf)  {
> >> >      char *str;
> >> > diff --git a/qapi/net.json b/qapi/net.json index
> >> > 7fab2e7cd8..91f2e1495a 100644
> >> > --- a/qapi/net.json
> >> > +++ b/qapi/net.json
> >> > @@ -7,6 +7,7 @@
> >> >  ##
> >> >
> >> >  { 'include': 'common.json' }
> >> > +{ 'include': 'sockets.json' }
> >> >
> >> >  ##
> >> >  # @set_link:
> >> > @@ -696,3 +697,76 @@
> >> >  ##
> >> >  { 'event': 'FAILOVER_NEGOTIATED',
> >> >    'data': {'device-id': 'str'} }
> >> > +
> >> > +##
> >> > +# @IPFlowSpec:
> >> > +#
> >> > +# IP flow specification.
> >> > +#
> >> > +# @protocol: Transport layer protocol like TCP/UDP, etc. The protocol
> is the
> >> > +#            string instead of enum, because it can be passed to
> getprotobyname(3)
> >> > +#            and avoid duplication with /etc/protocols.
> >>
> >> The rationale is good, but it doesn't really belong into the
> >> interface documentation.  Suggest:
> >>
> >>    # @protocol: Transport layer protocol like TCP/UDP, etc.  This will be
> >>    #            passed to getprotobyname(3).
> >>
> >
> > OK.
> >
> >>
> >> > +#
> >> > +# @object-name: The @object-name means packet handler in Qemu.
> Because not
> >> > +#               all the network packet must pass the colo-compare module,
> >> > +#               the net-filters are same situation. There modules attach to
> >> > +#               netdev or chardev to work, VM can run multiple modules
> >> > +#               at the same time. So it needs the object-name to set
> >> > +#               the effective module.
> >>
> >> I still don't understand this, and I'm too ignorant of COLO and
> >> networking to suggest improvements.
> >
> > Let me use qemu boot parameter to clear it.
> > For colo-compare, it needs chardev as the source to handle network packet.
> > -object colo-compare,id=comp0,primary_in=chardev-
> input0,secondary_in=chardev-input1,outdev=chardev-
> output0,iothread=iothread0.
> >
> > For net filters, it needs attached on netdev.
> > -object
> > filter-redirector,id=red0,netdev=hn0,queue=rx,outdev=chardev-output1
> > -object
> > filter-mirror,id=mirror0,netdev=hn0,queue=rx,outdev=chardev-output2
> >
> > And we can use -chardev socket combine the filter and the colo-compare.
> >
> > Back to the @object-name, One guest maybe have multi colo-compare as
> the same time, with different object name from different source.
> > So we need assign the IPFlowSpec to one object as the handler. Same as
> the net-filters.
> > Each object instance has its own passthrough list.
> 
> So the @object-name here references one of the "packet handler objects"
> (colo-compare, filter-redirector, filter-mirror) by @id.  Correct?

Yes.

> 
> In other words, @object-name is the ID of a QOM object, and the QOM
> object must be of a certain kind (i.e. provide certain functionality).
> Correct?

Yes.

> 
> What exactly makes a QOM object a "packet handler object?"
> 

Firstly, the original object need have basic network packet input/output capability.
It's a good question, maybe we need add a flag in general object structure to show the capability.

> Right now, the packet handler object types are colo-compare, filter-
> redirector, filter-mirror, and that's all.  Correct?

No, this series just make colo-compare become a packet handler, This is a beginning, I plan to make other filters support it.


> 
> Another question the doc comment needs to answer: what happens when
> @object-name is absent?

Please see the explanation below.

> 
> >> Jason or David, perhaps?
> >>
> >> > +#
> >> > +# @source: Source address and port.
> >> > +#
> >> > +# @destination: Destination address and port.
> >> > +#
> >> > +# Since: 6.1
> >> > +##
> >> > +{ 'struct': 'IPFlowSpec',
> >> > +  'data': { '*protocol': 'str', '*object-name': 'str',
> >> > +    '*source': 'InetSocketAddressBase',
> >> > +    '*destination': 'InetSocketAddressBase' } }
> >> > +
> >> > +##
> >> > +# @colo-passthrough-add:
> >> > +#
> >> > +# Add passthrough entry IPFlowSpec to the COLO-compare instance.
> >> > +# The protocol and source/destination IP/ports are optional. if
> >> > +the user # only inputs part of the information, this will match all traffic.
> >>
> >> Actually, all arguments are optional.
> >>
> >> Suggest:
> >>
> >>    # Add an entry to the COLO network passthrough list.
> >>    # Absent protocol, host addresses and ports match anything.
> >>
> >> If there is more than one such list, then "to a COLO network passthrough
> list"
> >> instead.
> >
> > Yes, more than one list.
> >
> >>
> >> Still missing then: meaning of absent @object-name.  Does it select
> >> the COLO network passthrough list, perhaps?
> >
> > Yes, Please see the explanation above. Each object instance has its own
> passthrough list.
> 
> Got it now.
> 
> >> > +#
> >> > +# Returns: Nothing on success
> >> > +#
> >> > +# Since: 6.1
> >> > +#
> >> > +# Example:
> >> > +#
> >> > +# -> { "execute": "colo-passthrough-add",
> >> > +#      "arguments": { "protocol": "tcp", "object-name": "object0",
> >> > +#      "source": {"host": "192.168.1.1", "port": "1234"},
> >> > +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
> >> > +# <- { "return": {} }
> >> > +#
> >> > +##
> >> > +{ 'command': 'colo-passthrough-add', 'boxed': true,
> >> > +     'data': 'IPFlowSpec' }
> >> > +
> >> > +##
> >> > +# @colo-passthrough-del:
> >> > +#
> >> > +# Delete passthrough entry IPFlowSpec to the COLO-compare instance.
> >> > +# The protocol and source/destination IP/ports are optional. if
> >> > +the user # only inputs part of the information, this will match all traffic.
> >>
> >> I suspect this command doesn't actually match traffic, it matches
> >> entries added with colo-passthrough-add.
> >
> > Yes.
> >
> >>
> >> Can it delete more than one such entry?
> >>
> >
> > Currently no, but it easy to match one more entry to delete.
> 
> If the passthrough list entries had some unique ID, we could refer to one
> entry by its ID.  It's how things commonly work.
> 
> Without an ID, we need to match by value, like you do.  I can see three
> possible behaviors:
> 
> 1. Select first entry that matches.
> 
> 2. Select all entries that match.
> 
> 3. If exactly one entry matches, select it.
> 
> The second design choice is behavior when nothing gets selected:
> 
> a. Silently do nothing
> 
> b. Error
> 
> Which one did you implement?  My guess based on your answers is 1a.

Re-think about it,  If we want to match by value, we need know which object have the capability and search in each object passthrough list.
Obviously, we haven't such flag in object structure. So It more reasonable to make @object-name as a must at the beginning.
Because the passthrough list always in the network handler object. Maybe we need a global passthrough list for each guest to handle it in the future.
It will have two-level passthrough list to control network.

Thanks
Chen


> 
> >> Suggest:
> >>
> >>    # Delete an entry from the COLO network passthrough list.
> >>
> >> and then explain how the command arguments select entries.
> >
> > Search the object's passthrough list, if find same entry,  delete it.
> >
> > Thanks
> > Chen
> >
> >>
> >> > +#
> >> > +# Returns: Nothing on success
> >> > +#
> >> > +# Since: 6.1
> >> > +#
> >> > +# Example:
> >> > +#
> >> > +# -> { "execute": "colo-passthrough-del",
> >> > +#      "arguments": { "protocol": "tcp", "object-name": "object0",
> >> > +#      "source": {"host": "192.168.1.1", "port": "1234"},
> >> > +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
> >> > +# <- { "return": {} }
> >> > +#
> >> > +##
> >> > +{ 'command': 'colo-passthrough-del', 'boxed': true,
> >> > +     'data': 'IPFlowSpec' }


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH V8 1/6] qapi/net: Add IPFlowSpec and QMP command for COLO passthrough
  2021-06-16  6:45         ` Zhang, Chen
@ 2021-06-16 13:26           ` Markus Armbruster
  2021-06-17  3:27             ` Zhang, Chen
  0 siblings, 1 reply; 25+ messages in thread
From: Markus Armbruster @ 2021-06-16 13:26 UTC (permalink / raw)
  To: Zhang, Chen
  Cc: Lukas Straub, Daniel P.Berrangé,
	Li Zhijian, Jason Wang, qemu-dev, Dr. David Alan Gilbert,
	Gerd Hoffmann, Zhang Chen, Eric Blake

"Zhang, Chen" <chen.zhang@intel.com> writes:

>> -----Original Message-----
>> From: Markus Armbruster <armbru@redhat.com>
>> Sent: Wednesday, June 16, 2021 2:04 PM
>> To: Zhang, Chen <chen.zhang@intel.com>
>> Cc: Lukas Straub <lukasstraub2@web.de>; Daniel P.Berrangé
>> <berrange@redhat.com>; Li Zhijian <lizhijian@cn.fujitsu.com>; Jason Wang
>> <jasowang@redhat.com>; qemu-dev <qemu-devel@nongnu.org>; Dr. David
>> Alan Gilbert <dgilbert@redhat.com>; Gerd Hoffmann <kraxel@redhat.com>;
>> Zhang Chen <zhangckid@gmail.com>; Eric Blake <eblake@redhat.com>
>> Subject: Re: [PATCH V8 1/6] qapi/net: Add IPFlowSpec and QMP command
>> for COLO passthrough
>> 
>> "Zhang, Chen" <chen.zhang@intel.com> writes:
>> 
>> >> -----Original Message-----
>> >> From: Markus Armbruster <armbru@redhat.com>
>> >> Sent: Tuesday, June 15, 2021 10:43 PM
>> >> To: Zhang, Chen <chen.zhang@intel.com>
>> >> Cc: Jason Wang <jasowang@redhat.com>; qemu-dev <qemu-
>> >> devel@nongnu.org>; Eric Blake <eblake@redhat.com>; Dr. David Alan
>> >> Gilbert <dgilbert@redhat.com>; Daniel P.Berrangé
>> >> <berrange@redhat.com>; Gerd Hoffmann <kraxel@redhat.com>; Li
>> Zhijian
>> >> <lizhijian@cn.fujitsu.com>; Lukas Straub <lukasstraub2@web.de>; Zhang
>> >> Chen <zhangckid@gmail.com>
>> >> Subject: Re: [PATCH V8 1/6] qapi/net: Add IPFlowSpec and QMP
>> command
>> >> for COLO passthrough
>> >>
>> >> Zhang Chen <chen.zhang@intel.com> writes:
>> >>
>> >> > Since the real user scenario does not need COLO to monitor all traffic.
>> >> > Add colo-passthrough-add and colo-passthrough-del to maintain a
>> >> > COLO network passthrough list. Add IPFlowSpec struct for all QMP commands.
>> >> > All the fields of IPFlowSpec are optional.
>> >> >
>> >> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
>> >> > ---
>> >>
>> >> The QAPI schema looks good to me, but the interface documentation is
>> >> still not quite clear enough.  To make progress, I'm going to make
>> >> concrete suggestions wherever I can despite being quite clueless
>> >> about the subject matter.  Risks me writing something that's clearer,
>> >> but wrong.  Keep that in mind, please.
>> >>
>> >> >  net/net.c     | 10 +++++++
>> >> >  qapi/net.json | 74
>> >> > +++++++++++++++++++++++++++++++++++++++++++++++++++
>> >> >  2 files changed, 84 insertions(+)
>> >> >
>> >> > diff --git a/net/net.c b/net/net.c
>> >> > index 76bbb7c31b..f913e97983 100644
>> >> > --- a/net/net.c
>> >> > +++ b/net/net.c
>> >> > @@ -1195,6 +1195,16 @@ void qmp_netdev_del(const char *id, Error **errp)
>> >> >      }
>> >> >  }
>> >> >
>> >> > +void qmp_colo_passthrough_add(IPFlowSpec *spec, Error **errp) {
>> >> > +    /* TODO implement setup passthrough rule */ }
>> >> > +
>> >> > +void qmp_colo_passthrough_del(IPFlowSpec *spec, Error **errp) {
>> >> > +    /* TODO implement delete passthrough rule */ }
>> >> > +
>> >> >  static void netfilter_print_info(Monitor *mon, NetFilterState *nf)  {
>> >> >      char *str;
>> >> > diff --git a/qapi/net.json b/qapi/net.json index
>> >> > 7fab2e7cd8..91f2e1495a 100644
>> >> > --- a/qapi/net.json
>> >> > +++ b/qapi/net.json
>> >> > @@ -7,6 +7,7 @@
>> >> >  ##
>> >> >
>> >> >  { 'include': 'common.json' }
>> >> > +{ 'include': 'sockets.json' }
>> >> >
>> >> >  ##
>> >> >  # @set_link:
>> >> > @@ -696,3 +697,76 @@
>> >> >  ##
>> >> >  { 'event': 'FAILOVER_NEGOTIATED',
>> >> >    'data': {'device-id': 'str'} }
>> >> > +
>> >> > +##
>> >> > +# @IPFlowSpec:
>> >> > +#
>> >> > +# IP flow specification.
>> >> > +#
>> >> > +# @protocol: Transport layer protocol like TCP/UDP, etc. The protocol is the
>> >> > +#            string instead of enum, because it can be passed to getprotobyname(3)
>> >> > +#            and avoid duplication with /etc/protocols.
>> >>
>> >> The rationale is good, but it doesn't really belong into the
>> >> interface documentation.  Suggest:
>> >>
>> >>    # @protocol: Transport layer protocol like TCP/UDP, etc.  This will be
>> >>    #            passed to getprotobyname(3).
>> >>
>> >
>> > OK.
>> >
>> >>
>> >> > +#
>> >> > +# @object-name: The @object-name means packet handler in Qemu. Because not
>> >> > +#               all the network packet must pass the colo-compare module,
>> >> > +#               the net-filters are same situation. There modules attach to
>> >> > +#               netdev or chardev to work, VM can run multiple modules
>> >> > +#               at the same time. So it needs the object-name to set
>> >> > +#               the effective module.
>> >>
>> >> I still don't understand this, and I'm too ignorant of COLO and
>> >> networking to suggest improvements.
>> >
>> > Let me use qemu boot parameter to clear it.
>> > For colo-compare, it needs chardev as the source to handle network packet.
>> > -object colo-compare,id=comp0,primary_in=chardev-input0,secondary_in=chardev-input1,outdev=chardev-output0,iothread=iothread0.
>> >
>> > For net filters, it needs attached on netdev.
>> > -object filter-redirector,id=red0,netdev=hn0,queue=rx,outdev=chardev-output1
>> > -object filter-mirror,id=mirror0,netdev=hn0,queue=rx,outdev=chardev-output2
>> >
>> > And we can use -chardev socket combine the filter and the colo-compare.
>> >
>> > Back to the @object-name, One guest maybe have multi colo-compare as the same time, with different object name from different source.
>> > So we need assign the IPFlowSpec to one object as the handler. Same as the net-filters.
>> > Each object instance has its own passthrough list.
>> 
>> So the @object-name here references one of the "packet handler objects"
>> (colo-compare, filter-redirector, filter-mirror) by @id.  Correct?
>
> Yes.
>
>> 
>> In other words, @object-name is the ID of a QOM object, and the QOM
>> object must be of a certain kind (i.e. provide certain functionality).
>> Correct?
>
> Yes.

Got it.

>> What exactly makes a QOM object a "packet handler object?"
>> 
>
> Firstly, the original object need have basic network packet input/output capability.
> It's a good question, maybe we need add a flag in general object structure to show the capability.

A QOM interface might fit the bill: a QOM type is a packet handler if
and only if it implements the packet handler interface.
 
>> Right now, the packet handler object types are colo-compare, filter-
>> redirector, filter-mirror, and that's all.  Correct?
>
> No, this series just make colo-compare become a packet handler, This is a beginning, I plan to make other filters support it.

Okay.

Are these other filters similarly related to COLO?  I'm asking because
the commands are called colo-passthrough-FOO.  If this goes beyond COLO,
we may want to name them differently.

>> Another question the doc comment needs to answer: what happens when
>> @object-name is absent?
>
> Please see the explanation below.

You seem to consider making it mandatory there.  My question would be
moot then.

>> >> Jason or David, perhaps?
>> >>
>> >> > +#
>> >> > +# @source: Source address and port.
>> >> > +#
>> >> > +# @destination: Destination address and port.
>> >> > +#
>> >> > +# Since: 6.1
>> >> > +##
>> >> > +{ 'struct': 'IPFlowSpec',
>> >> > +  'data': { '*protocol': 'str', '*object-name': 'str',
>> >> > +    '*source': 'InetSocketAddressBase',
>> >> > +    '*destination': 'InetSocketAddressBase' } }
>> >> > +
>> >> > +##
>> >> > +# @colo-passthrough-add:
>> >> > +#
>> >> > +# Add passthrough entry IPFlowSpec to the COLO-compare instance.
>> >> > +# The protocol and source/destination IP/ports are optional. if
>> >> > +the user # only inputs part of the information, this will match all traffic.
>> >>
>> >> Actually, all arguments are optional.
>> >>
>> >> Suggest:
>> >>
>> >>    # Add an entry to the COLO network passthrough list.
>> >>    # Absent protocol, host addresses and ports match anything.
>> >>
>> >> If there is more than one such list, then "to a COLO network passthrough list"
>> >> instead.
>> >
>> > Yes, more than one list.
>> >
>> >>
>> >> Still missing then: meaning of absent @object-name.  Does it select
>> >> the COLO network passthrough list, perhaps?
>> >
>> > Yes, Please see the explanation above. Each object instance has its own passthrough list.
>> 
>> Got it now.
>> 
>> >> > +#
>> >> > +# Returns: Nothing on success
>> >> > +#
>> >> > +# Since: 6.1
>> >> > +#
>> >> > +# Example:
>> >> > +#
>> >> > +# -> { "execute": "colo-passthrough-add",
>> >> > +#      "arguments": { "protocol": "tcp", "object-name": "object0",
>> >> > +#      "source": {"host": "192.168.1.1", "port": "1234"},
>> >> > +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
>> >> > +# <- { "return": {} }
>> >> > +#
>> >> > +##
>> >> > +{ 'command': 'colo-passthrough-add', 'boxed': true,
>> >> > +     'data': 'IPFlowSpec' }
>> >> > +
>> >> > +##
>> >> > +# @colo-passthrough-del:
>> >> > +#
>> >> > +# Delete passthrough entry IPFlowSpec to the COLO-compare instance.
>> >> > +# The protocol and source/destination IP/ports are optional. if
>> >> > +the user # only inputs part of the information, this will match all traffic.
>> >>
>> >> I suspect this command doesn't actually match traffic, it matches
>> >> entries added with colo-passthrough-add.
>> >
>> > Yes.
>> >
>> >>
>> >> Can it delete more than one such entry?
>> >>
>> >
>> > Currently no, but it easy to match one more entry to delete.
>> 
>> If the passthrough list entries had some unique ID, we could refer to one
>> entry by its ID.  It's how things commonly work.
>> 
>> Without an ID, we need to match by value, like you do.  I can see three
>> possible behaviors:
>> 
>> 1. Select first entry that matches.
>> 
>> 2. Select all entries that match.
>> 
>> 3. If exactly one entry matches, select it.
>> 
>> The second design choice is behavior when nothing gets selected:
>> 
>> a. Silently do nothing
>> 
>> b. Error
>> 
>> Which one did you implement?  My guess based on your answers is 1a.
>
> Re-think about it,  If we want to match by value, we need know which object have the capability and search in each object passthrough list.
> Obviously, we haven't such flag in object structure. So It more reasonable to make @object-name as a must at the beginning.
> Because the passthrough list always in the network handler object. Maybe we need a global passthrough list for each guest to handle it in the future.
> It will have two-level passthrough list to control network.

I'm not sure I understand.

If you make @object-name mandatory both for colo-passthrough-add and
colo-passthrough-del, then we can simply use @object-name to find the
object, check it implements the packet handler interface, use the packet
handler interface to get its passthrough list, then add to / delete from
that list.

If we find a use for making @object-name optional later, we can do so
without breaking compatibility.



^ permalink raw reply	[flat|nested] 25+ messages in thread

* RE: [PATCH V8 1/6] qapi/net: Add IPFlowSpec and QMP command for COLO passthrough
  2021-06-16 13:26           ` Markus Armbruster
@ 2021-06-17  3:27             ` Zhang, Chen
  2021-06-17 11:03               ` Markus Armbruster
  0 siblings, 1 reply; 25+ messages in thread
From: Zhang, Chen @ 2021-06-17  3:27 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Lukas Straub, Daniel P.Berrangé,
	Li Zhijian, Jason Wang, qemu-dev, Dr. David Alan Gilbert,
	Gerd Hoffmann, Zhang Chen, Eric Blake



> -----Original Message-----
> From: Markus Armbruster <armbru@redhat.com>
> Sent: Wednesday, June 16, 2021 9:26 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: Lukas Straub <lukasstraub2@web.de>; Daniel P.Berrangé
> <berrange@redhat.com>; Li Zhijian <lizhijian@cn.fujitsu.com>; Jason Wang
> <jasowang@redhat.com>; qemu-dev <qemu-devel@nongnu.org>; Dr. David
> Alan Gilbert <dgilbert@redhat.com>; Gerd Hoffmann <kraxel@redhat.com>;
> Zhang Chen <zhangckid@gmail.com>; Eric Blake <eblake@redhat.com>
> Subject: Re: [PATCH V8 1/6] qapi/net: Add IPFlowSpec and QMP command
> for COLO passthrough
> 
> "Zhang, Chen" <chen.zhang@intel.com> writes:
> 
> >> -----Original Message-----
> >> From: Markus Armbruster <armbru@redhat.com>
> >> Sent: Wednesday, June 16, 2021 2:04 PM
> >> To: Zhang, Chen <chen.zhang@intel.com>
> >> Cc: Lukas Straub <lukasstraub2@web.de>; Daniel P.Berrangé
> >> <berrange@redhat.com>; Li Zhijian <lizhijian@cn.fujitsu.com>; Jason
> >> Wang <jasowang@redhat.com>; qemu-dev <qemu-devel@nongnu.org>;
> Dr.
> >> David Alan Gilbert <dgilbert@redhat.com>; Gerd Hoffmann
> >> <kraxel@redhat.com>; Zhang Chen <zhangckid@gmail.com>; Eric Blake
> >> <eblake@redhat.com>
> >> Subject: Re: [PATCH V8 1/6] qapi/net: Add IPFlowSpec and QMP
> command
> >> for COLO passthrough
> >>
> >> "Zhang, Chen" <chen.zhang@intel.com> writes:
> >>
> >> >> -----Original Message-----
> >> >> From: Markus Armbruster <armbru@redhat.com>
> >> >> Sent: Tuesday, June 15, 2021 10:43 PM
> >> >> To: Zhang, Chen <chen.zhang@intel.com>
> >> >> Cc: Jason Wang <jasowang@redhat.com>; qemu-dev <qemu-
> >> >> devel@nongnu.org>; Eric Blake <eblake@redhat.com>; Dr. David Alan
> >> >> Gilbert <dgilbert@redhat.com>; Daniel P.Berrangé
> >> >> <berrange@redhat.com>; Gerd Hoffmann <kraxel@redhat.com>; Li
> >> Zhijian
> >> >> <lizhijian@cn.fujitsu.com>; Lukas Straub <lukasstraub2@web.de>;
> >> >> Zhang Chen <zhangckid@gmail.com>
> >> >> Subject: Re: [PATCH V8 1/6] qapi/net: Add IPFlowSpec and QMP
> >> command
> >> >> for COLO passthrough
> >> >>
> >> >> Zhang Chen <chen.zhang@intel.com> writes:
> >> >>
> >> >> > Since the real user scenario does not need COLO to monitor all traffic.
> >> >> > Add colo-passthrough-add and colo-passthrough-del to maintain a
> >> >> > COLO network passthrough list. Add IPFlowSpec struct for all QMP
> commands.
> >> >> > All the fields of IPFlowSpec are optional.
> >> >> >
> >> >> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> >> >> > ---
> >> >>
> >> >> The QAPI schema looks good to me, but the interface documentation
> >> >> is still not quite clear enough.  To make progress, I'm going to
> >> >> make concrete suggestions wherever I can despite being quite
> >> >> clueless about the subject matter.  Risks me writing something
> >> >> that's clearer, but wrong.  Keep that in mind, please.
> >> >>
> >> >> >  net/net.c     | 10 +++++++
> >> >> >  qapi/net.json | 74
> >> >> > +++++++++++++++++++++++++++++++++++++++++++++++++++
> >> >> >  2 files changed, 84 insertions(+)
> >> >> >
> >> >> > diff --git a/net/net.c b/net/net.c index 76bbb7c31b..f913e97983
> >> >> > 100644
> >> >> > --- a/net/net.c
> >> >> > +++ b/net/net.c
> >> >> > @@ -1195,6 +1195,16 @@ void qmp_netdev_del(const char *id,
> Error **errp)
> >> >> >      }
> >> >> >  }
> >> >> >
> >> >> > +void qmp_colo_passthrough_add(IPFlowSpec *spec, Error **errp)
> {
> >> >> > +    /* TODO implement setup passthrough rule */ }
> >> >> > +
> >> >> > +void qmp_colo_passthrough_del(IPFlowSpec *spec, Error **errp) {
> >> >> > +    /* TODO implement delete passthrough rule */ }
> >> >> > +
> >> >> >  static void netfilter_print_info(Monitor *mon, NetFilterState *nf)  {
> >> >> >      char *str;
> >> >> > diff --git a/qapi/net.json b/qapi/net.json index
> >> >> > 7fab2e7cd8..91f2e1495a 100644
> >> >> > --- a/qapi/net.json
> >> >> > +++ b/qapi/net.json
> >> >> > @@ -7,6 +7,7 @@
> >> >> >  ##
> >> >> >
> >> >> >  { 'include': 'common.json' }
> >> >> > +{ 'include': 'sockets.json' }
> >> >> >
> >> >> >  ##
> >> >> >  # @set_link:
> >> >> > @@ -696,3 +697,76 @@
> >> >> >  ##
> >> >> >  { 'event': 'FAILOVER_NEGOTIATED',
> >> >> >    'data': {'device-id': 'str'} }
> >> >> > +
> >> >> > +##
> >> >> > +# @IPFlowSpec:
> >> >> > +#
> >> >> > +# IP flow specification.
> >> >> > +#
> >> >> > +# @protocol: Transport layer protocol like TCP/UDP, etc. The
> protocol is the
> >> >> > +#            string instead of enum, because it can be passed to
> getprotobyname(3)
> >> >> > +#            and avoid duplication with /etc/protocols.
> >> >>
> >> >> The rationale is good, but it doesn't really belong into the
> >> >> interface documentation.  Suggest:
> >> >>
> >> >>    # @protocol: Transport layer protocol like TCP/UDP, etc.  This will be
> >> >>    #            passed to getprotobyname(3).
> >> >>
> >> >
> >> > OK.
> >> >
> >> >>
> >> >> > +#
> >> >> > +# @object-name: The @object-name means packet handler in
> Qemu. Because not
> >> >> > +#               all the network packet must pass the colo-compare
> module,
> >> >> > +#               the net-filters are same situation. There modules attach to
> >> >> > +#               netdev or chardev to work, VM can run multiple modules
> >> >> > +#               at the same time. So it needs the object-name to set
> >> >> > +#               the effective module.
> >> >>
> >> >> I still don't understand this, and I'm too ignorant of COLO and
> >> >> networking to suggest improvements.
> >> >
> >> > Let me use qemu boot parameter to clear it.
> >> > For colo-compare, it needs chardev as the source to handle network
> packet.
> >> > -object colo-compare,id=comp0,primary_in=chardev-
> input0,secondary_in=chardev-input1,outdev=chardev-
> output0,iothread=iothread0.
> >> >
> >> > For net filters, it needs attached on netdev.
> >> > -object
> >> > filter-redirector,id=red0,netdev=hn0,queue=rx,outdev=chardev-
> output
> >> > 1 -object
> >> > filter-mirror,id=mirror0,netdev=hn0,queue=rx,outdev=chardev-
> output2
> >> >
> >> > And we can use -chardev socket combine the filter and the colo-
> compare.
> >> >
> >> > Back to the @object-name, One guest maybe have multi colo-compare
> as the same time, with different object name from different source.
> >> > So we need assign the IPFlowSpec to one object as the handler. Same
> as the net-filters.
> >> > Each object instance has its own passthrough list.
> >>
> >> So the @object-name here references one of the "packet handler
> objects"
> >> (colo-compare, filter-redirector, filter-mirror) by @id.  Correct?
> >
> > Yes.
> >
> >>
> >> In other words, @object-name is the ID of a QOM object, and the QOM
> >> object must be of a certain kind (i.e. provide certain functionality).
> >> Correct?
> >
> > Yes.
> 
> Got it.
> 
> >> What exactly makes a QOM object a "packet handler object?"
> >>
> >
> > Firstly, the original object need have basic network packet input/output
> capability.
> > It's a good question, maybe we need add a flag in general object structure
> to show the capability.
> 
> A QOM interface might fit the bill: a QOM type is a packet handler if and only
> if it implements the packet handler interface.
> 
> >> Right now, the packet handler object types are colo-compare, filter-
> >> redirector, filter-mirror, and that's all.  Correct?
> >
> > No, this series just make colo-compare become a packet handler, This is a
> beginning, I plan to make other filters support it.
> 
> Okay.
> 
> Are these other filters similarly related to COLO?  I'm asking because the
> commands are called colo-passthrough-FOO.  If this goes beyond COLO, we
> may want to name them differently.

No, net-filter is an independent module, although colo must use net-filter to build colo-proxy.
I think we can change the name when enable net-filter support passthrough list.

> 
> >> Another question the doc comment needs to answer: what happens
> when
> >> @object-name is absent?
> >
> > Please see the explanation below.
> 
> You seem to consider making it mandatory there.  My question would be
> moot then.
> 
> >> >> Jason or David, perhaps?
> >> >>
> >> >> > +#
> >> >> > +# @source: Source address and port.
> >> >> > +#
> >> >> > +# @destination: Destination address and port.
> >> >> > +#
> >> >> > +# Since: 6.1
> >> >> > +##
> >> >> > +{ 'struct': 'IPFlowSpec',
> >> >> > +  'data': { '*protocol': 'str', '*object-name': 'str',
> >> >> > +    '*source': 'InetSocketAddressBase',
> >> >> > +    '*destination': 'InetSocketAddressBase' } }
> >> >> > +
> >> >> > +##
> >> >> > +# @colo-passthrough-add:
> >> >> > +#
> >> >> > +# Add passthrough entry IPFlowSpec to the COLO-compare
> instance.
> >> >> > +# The protocol and source/destination IP/ports are optional. if
> >> >> > +the user # only inputs part of the information, this will match all
> traffic.
> >> >>
> >> >> Actually, all arguments are optional.
> >> >>
> >> >> Suggest:
> >> >>
> >> >>    # Add an entry to the COLO network passthrough list.
> >> >>    # Absent protocol, host addresses and ports match anything.
> >> >>
> >> >> If there is more than one such list, then "to a COLO network
> passthrough list"
> >> >> instead.
> >> >
> >> > Yes, more than one list.
> >> >
> >> >>
> >> >> Still missing then: meaning of absent @object-name.  Does it
> >> >> select the COLO network passthrough list, perhaps?
> >> >
> >> > Yes, Please see the explanation above. Each object instance has its own
> passthrough list.
> >>
> >> Got it now.
> >>
> >> >> > +#
> >> >> > +# Returns: Nothing on success
> >> >> > +#
> >> >> > +# Since: 6.1
> >> >> > +#
> >> >> > +# Example:
> >> >> > +#
> >> >> > +# -> { "execute": "colo-passthrough-add",
> >> >> > +#      "arguments": { "protocol": "tcp", "object-name": "object0",
> >> >> > +#      "source": {"host": "192.168.1.1", "port": "1234"},
> >> >> > +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
> >> >> > +# <- { "return": {} }
> >> >> > +#
> >> >> > +##
> >> >> > +{ 'command': 'colo-passthrough-add', 'boxed': true,
> >> >> > +     'data': 'IPFlowSpec' }
> >> >> > +
> >> >> > +##
> >> >> > +# @colo-passthrough-del:
> >> >> > +#
> >> >> > +# Delete passthrough entry IPFlowSpec to the COLO-compare
> instance.
> >> >> > +# The protocol and source/destination IP/ports are optional. if
> >> >> > +the user # only inputs part of the information, this will match all
> traffic.
> >> >>
> >> >> I suspect this command doesn't actually match traffic, it matches
> >> >> entries added with colo-passthrough-add.
> >> >
> >> > Yes.
> >> >
> >> >>
> >> >> Can it delete more than one such entry?
> >> >>
> >> >
> >> > Currently no, but it easy to match one more entry to delete.
> >>
> >> If the passthrough list entries had some unique ID, we could refer to
> >> one entry by its ID.  It's how things commonly work.
> >>
> >> Without an ID, we need to match by value, like you do.  I can see
> >> three possible behaviors:
> >>
> >> 1. Select first entry that matches.
> >>
> >> 2. Select all entries that match.
> >>
> >> 3. If exactly one entry matches, select it.
> >>
> >> The second design choice is behavior when nothing gets selected:
> >>
> >> a. Silently do nothing
> >>
> >> b. Error
> >>
> >> Which one did you implement?  My guess based on your answers is 1a.
> >
> > Re-think about it,  If we want to match by value, we need know which
> object have the capability and search in each object passthrough list.
> > Obviously, we haven't such flag in object structure. So It more reasonable
> to make @object-name as a must at the beginning.
> > Because the passthrough list always in the network handler object. Maybe
> we need a global passthrough list for each guest to handle it in the future.
> > It will have two-level passthrough list to control network.
> 
> I'm not sure I understand.
> 
> If you make @object-name mandatory both for colo-passthrough-add and
> colo-passthrough-del, then we can simply use @object-name to find the
> object, check it implements the packet handler interface, use the packet
> handler interface to get its passthrough list, then add to / delete from that
> list.

Yes, I think so.

> 
> If we find a use for making @object-name optional later, we can do so
> without breaking compatibility.

Yes, It still need current code as the base. Just need add a global passthrough list.


Thanks
Chen














^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH V8 1/6] qapi/net: Add IPFlowSpec and QMP command for COLO passthrough
  2021-06-17  3:27             ` Zhang, Chen
@ 2021-06-17 11:03               ` Markus Armbruster
  2021-06-22  5:58                 ` chen.zhang
  0 siblings, 1 reply; 25+ messages in thread
From: Markus Armbruster @ 2021-06-17 11:03 UTC (permalink / raw)
  To: Zhang, Chen
  Cc: Lukas Straub, Daniel P.Berrangé,
	Li Zhijian, Jason Wang, qemu-dev, Dr. David Alan Gilbert,
	Gerd Hoffmann, Zhang Chen, Eric Blake

You recently started using

    Content-Type: text/plain; charset="utf-8"
    Content-Transfer-Encoding: base64

Please consider anorhter Content-Transfer-Encoding instead.
quoted-printable should do.

"Zhang, Chen" <chen.zhang@intel.com> writes:

[...]

> No, net-filter is an independent module, although colo must use net-filter to build colo-proxy.
> I think we can change the name when enable net-filter support passthrough list.

Changing names of stable interfaces is always awkward.  So, either make
this an unstable interface, or use names that are likely to work for the
foreseeable evolution of the interface.

What about passthrough-filter-add and -del?  Jason?

[...]



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH V8 1/6] qapi/net: Add IPFlowSpec and QMP command for COLO passthrough
  2021-06-15 14:43   ` Markus Armbruster
  2021-06-16  2:12     ` Zhang, Chen
@ 2021-06-21 11:30     ` Dr. David Alan Gilbert
  2021-06-22  6:01       ` chen.zhang
  1 sibling, 1 reply; 25+ messages in thread
From: Dr. David Alan Gilbert @ 2021-06-21 11:30 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Lukas Straub, Daniel P. Berrangé,
	Li Zhijian, Jason Wang, qemu-dev, Zhang Chen, Gerd Hoffmann,
	Zhang Chen, Eric Blake

* Markus Armbruster (armbru@redhat.com) wrote:
> Zhang Chen <chen.zhang@intel.com> writes:
> 
> > Since the real user scenario does not need COLO to monitor all traffic.
> > Add colo-passthrough-add and colo-passthrough-del to maintain
> > a COLO network passthrough list. Add IPFlowSpec struct for all QMP commands.
> > All the fields of IPFlowSpec are optional.
> >
> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> > ---
> 
> The QAPI schema looks good to me, but the interface documentation is
> still not quite clear enough.  To make progress, I'm going to make
> concrete suggestions wherever I can despite being quite clueless about
> the subject matter.  Risks me writing something that's clearer, but
> wrong.  Keep that in mind, please.
> 
> >  net/net.c     | 10 +++++++
> >  qapi/net.json | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 84 insertions(+)
> >
> > diff --git a/net/net.c b/net/net.c
> > index 76bbb7c31b..f913e97983 100644
> > --- a/net/net.c
> > +++ b/net/net.c
> > @@ -1195,6 +1195,16 @@ void qmp_netdev_del(const char *id, Error **errp)
> >      }
> >  }
> >  
> > +void qmp_colo_passthrough_add(IPFlowSpec *spec, Error **errp)
> > +{
> > +    /* TODO implement setup passthrough rule */
> > +}
> > +
> > +void qmp_colo_passthrough_del(IPFlowSpec *spec, Error **errp)
> > +{
> > +    /* TODO implement delete passthrough rule */
> > +}
> > +
> >  static void netfilter_print_info(Monitor *mon, NetFilterState *nf)
> >  {
> >      char *str;
> > diff --git a/qapi/net.json b/qapi/net.json
> > index 7fab2e7cd8..91f2e1495a 100644
> > --- a/qapi/net.json
> > +++ b/qapi/net.json
> > @@ -7,6 +7,7 @@
> >  ##
> >  
> >  { 'include': 'common.json' }
> > +{ 'include': 'sockets.json' }
> >  
> >  ##
> >  # @set_link:
> > @@ -696,3 +697,76 @@
> >  ##
> >  { 'event': 'FAILOVER_NEGOTIATED',
> >    'data': {'device-id': 'str'} }
> > +
> > +##
> > +# @IPFlowSpec:
> > +#
> > +# IP flow specification.
> > +#
> > +# @protocol: Transport layer protocol like TCP/UDP, etc. The protocol is the
> > +#            string instead of enum, because it can be passed to getprotobyname(3)
> > +#            and avoid duplication with /etc/protocols.
> 
> The rationale is good, but it doesn't really belong into the interface
> documentation.  Suggest:
> 
>    # @protocol: Transport layer protocol like TCP/UDP, etc.  This will be
>    #            passed to getprotobyname(3).
> 
> 
> > +#
> > +# @object-name: The @object-name means packet handler in Qemu. Because not
> > +#               all the network packet must pass the colo-compare module,
> > +#               the net-filters are same situation. There modules attach to
> > +#               netdev or chardev to work, VM can run multiple modules
> > +#               at the same time. So it needs the object-name to set
> > +#               the effective module.
> 
> I still don't understand this, and I'm too ignorant of COLO and
> networking to suggest improvements.
> 
> Jason or David, perhaps?

I'll leave Jason to check on the object behaviour (and I see the rest of
the thread); but at a high level, COLO is deciding whether to do VM
synchronisation on whether the network behaviour of the two VMs get out
of sync (e.g. due to randomness in the flow of requests); if you don't
sync then when you fail-over, you'll get TCP errors/inconsistencies in
the stream view from the secondary; but this patch series is saying
you don't care if some TCP connections fail like that, you only care
about maybe the main sockets the application is providing.

Dave

> > +#
> > +# @source: Source address and port.
> > +#
> > +# @destination: Destination address and port.
> > +#
> > +# Since: 6.1
> > +##
> > +{ 'struct': 'IPFlowSpec',
> > +  'data': { '*protocol': 'str', '*object-name': 'str',
> > +    '*source': 'InetSocketAddressBase',
> > +    '*destination': 'InetSocketAddressBase' } }
> > +
> > +##
> > +# @colo-passthrough-add:
> > +#
> > +# Add passthrough entry IPFlowSpec to the COLO-compare instance.
> > +# The protocol and source/destination IP/ports are optional. if the user
> > +# only inputs part of the information, this will match all traffic.
> 
> Actually, all arguments are optional.
> 
> Suggest:
> 
>    # Add an entry to the COLO network passthrough list.
>    # Absent protocol, host addresses and ports match anything.
> 
> If there is more than one such list, then "to a COLO network passthrough
> list" instead.
> 
> Still missing then: meaning of absent @object-name.  Does it select the
> COLO network passthrough list, perhaps?
> 
> > +#
> > +# Returns: Nothing on success
> > +#
> > +# Since: 6.1
> > +#
> > +# Example:
> > +#
> > +# -> { "execute": "colo-passthrough-add",
> > +#      "arguments": { "protocol": "tcp", "object-name": "object0",
> > +#      "source": {"host": "192.168.1.1", "port": "1234"},
> > +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
> > +# <- { "return": {} }
> > +#
> > +##
> > +{ 'command': 'colo-passthrough-add', 'boxed': true,
> > +     'data': 'IPFlowSpec' }
> > +
> > +##
> > +# @colo-passthrough-del:
> > +#
> > +# Delete passthrough entry IPFlowSpec to the COLO-compare instance.
> > +# The protocol and source/destination IP/ports are optional. if the user
> > +# only inputs part of the information, this will match all traffic.
> 
> I suspect this command doesn't actually match traffic, it matches
> entries added with colo-passthrough-add.
> 
> Can it delete more than one such entry?
> 
> Suggest:
> 
>    # Delete an entry from the COLO network passthrough list.
> 
> and then explain how the command arguments select entries.
> 
> > +#
> > +# Returns: Nothing on success
> > +#
> > +# Since: 6.1
> > +#
> > +# Example:
> > +#
> > +# -> { "execute": "colo-passthrough-del",
> > +#      "arguments": { "protocol": "tcp", "object-name": "object0",
> > +#      "source": {"host": "192.168.1.1", "port": "1234"},
> > +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
> > +# <- { "return": {} }
> > +#
> > +##
> > +{ 'command': 'colo-passthrough-del', 'boxed': true,
> > +     'data': 'IPFlowSpec' }
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH V8 1/6] qapi/net: Add IPFlowSpec and QMP command for COLO passthrough
  2021-06-17 11:03               ` Markus Armbruster
@ 2021-06-22  5:58                 ` chen.zhang
  0 siblings, 0 replies; 25+ messages in thread
From: chen.zhang @ 2021-06-22  5:58 UTC (permalink / raw)
  To: Markus Armbruster, Dr. David Alan Gilbert
  Cc: Lukas Straub, Daniel P.Berrangé,
	Li Zhijian, Jason Wang, qemu-dev, Gerd Hoffmann, Zhang Chen,
	Eric Blake


On 6/17/21 7:03 PM, Markus Armbruster wrote:
> You recently started using
>
>      Content-Type: text/plain; charset="utf-8"
>      Content-Transfer-Encoding: base64
>
> Please consider anorhter Content-Transfer-Encoding instead.
> quoted-printable should do.
>
> "Zhang, Chen" <chen.zhang@intel.com> writes:
>
> [...]
>
>> No, net-filter is an independent module, although colo must use net-filter to build colo-proxy.
>> I think we can change the name when enable net-filter support passthrough list.
> Changing names of stable interfaces is always awkward.  So, either make
> this an unstable interface, or use names that are likely to work for the
> foreseeable evolution of the interface.
>
> What about passthrough-filter-add and -del?  Jason?


Hi Markus and Dave,

It's OK for me, and I synced with Jason, he don't have any objection.

I will change to "passthrough-filter-add/del" and try to merge this 
series if you and Dave don't have objection too.


Thanks

Chen


>
> [...]
>


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH V8 1/6] qapi/net: Add IPFlowSpec and QMP command for COLO passthrough
  2021-06-21 11:30     ` Dr. David Alan Gilbert
@ 2021-06-22  6:01       ` chen.zhang
  2021-06-22  7:04         ` Jason Wang
  0 siblings, 1 reply; 25+ messages in thread
From: chen.zhang @ 2021-06-22  6:01 UTC (permalink / raw)
  To: Dr. David Alan Gilbert, Markus Armbruster
  Cc: Lukas Straub, Daniel P. Berrangé,
	Li Zhijian, Jason Wang, qemu-dev, Gerd Hoffmann, Zhang Chen,
	Eric Blake


On 6/21/21 7:30 PM, Dr. David Alan Gilbert wrote:
> * Markus Armbruster (armbru@redhat.com) wrote:
>> Zhang Chen <chen.zhang@intel.com> writes:
>>
>>> Since the real user scenario does not need COLO to monitor all traffic.
>>> Add colo-passthrough-add and colo-passthrough-del to maintain
>>> a COLO network passthrough list. Add IPFlowSpec struct for all QMP commands.
>>> All the fields of IPFlowSpec are optional.
>>>
>>> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
>>> ---
>> The QAPI schema looks good to me, but the interface documentation is
>> still not quite clear enough.  To make progress, I'm going to make
>> concrete suggestions wherever I can despite being quite clueless about
>> the subject matter.  Risks me writing something that's clearer, but
>> wrong.  Keep that in mind, please.
>>
>>>   net/net.c     | 10 +++++++
>>>   qapi/net.json | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   2 files changed, 84 insertions(+)
>>>
>>> diff --git a/net/net.c b/net/net.c
>>> index 76bbb7c31b..f913e97983 100644
>>> --- a/net/net.c
>>> +++ b/net/net.c
>>> @@ -1195,6 +1195,16 @@ void qmp_netdev_del(const char *id, Error **errp)
>>>       }
>>>   }
>>>   
>>> +void qmp_colo_passthrough_add(IPFlowSpec *spec, Error **errp)
>>> +{
>>> +    /* TODO implement setup passthrough rule */
>>> +}
>>> +
>>> +void qmp_colo_passthrough_del(IPFlowSpec *spec, Error **errp)
>>> +{
>>> +    /* TODO implement delete passthrough rule */
>>> +}
>>> +
>>>   static void netfilter_print_info(Monitor *mon, NetFilterState *nf)
>>>   {
>>>       char *str;
>>> diff --git a/qapi/net.json b/qapi/net.json
>>> index 7fab2e7cd8..91f2e1495a 100644
>>> --- a/qapi/net.json
>>> +++ b/qapi/net.json
>>> @@ -7,6 +7,7 @@
>>>   ##
>>>   
>>>   { 'include': 'common.json' }
>>> +{ 'include': 'sockets.json' }
>>>   
>>>   ##
>>>   # @set_link:
>>> @@ -696,3 +697,76 @@
>>>   ##
>>>   { 'event': 'FAILOVER_NEGOTIATED',
>>>     'data': {'device-id': 'str'} }
>>> +
>>> +##
>>> +# @IPFlowSpec:
>>> +#
>>> +# IP flow specification.
>>> +#
>>> +# @protocol: Transport layer protocol like TCP/UDP, etc. The protocol is the
>>> +#            string instead of enum, because it can be passed to getprotobyname(3)
>>> +#            and avoid duplication with /etc/protocols.
>> The rationale is good, but it doesn't really belong into the interface
>> documentation.  Suggest:
>>
>>     # @protocol: Transport layer protocol like TCP/UDP, etc.  This will be
>>     #            passed to getprotobyname(3).
>>
>>
>>> +#
>>> +# @object-name: The @object-name means packet handler in Qemu. Because not
>>> +#               all the network packet must pass the colo-compare module,
>>> +#               the net-filters are same situation. There modules attach to
>>> +#               netdev or chardev to work, VM can run multiple modules
>>> +#               at the same time. So it needs the object-name to set
>>> +#               the effective module.
>> I still don't understand this, and I'm too ignorant of COLO and
>> networking to suggest improvements.
>>
>> Jason or David, perhaps?
> I'll leave Jason to check on the object behaviour (and I see the rest of
> the thread); but at a high level, COLO is deciding whether to do VM
> synchronisation on whether the network behaviour of the two VMs get out
> of sync (e.g. due to randomness in the flow of requests); if you don't
> sync then when you fail-over, you'll get TCP errors/inconsistencies in
> the stream view from the secondary; but this patch series is saying
> you don't care if some TCP connections fail like that, you only care
> about maybe the main sockets the application is providing.

Yes, you are right.


Thanks

Chen


> Dave
>
>>> +#
>>> +# @source: Source address and port.
>>> +#
>>> +# @destination: Destination address and port.
>>> +#
>>> +# Since: 6.1
>>> +##
>>> +{ 'struct': 'IPFlowSpec',
>>> +  'data': { '*protocol': 'str', '*object-name': 'str',
>>> +    '*source': 'InetSocketAddressBase',
>>> +    '*destination': 'InetSocketAddressBase' } }
>>> +
>>> +##
>>> +# @colo-passthrough-add:
>>> +#
>>> +# Add passthrough entry IPFlowSpec to the COLO-compare instance.
>>> +# The protocol and source/destination IP/ports are optional. if the user
>>> +# only inputs part of the information, this will match all traffic.
>> Actually, all arguments are optional.
>>
>> Suggest:
>>
>>     # Add an entry to the COLO network passthrough list.
>>     # Absent protocol, host addresses and ports match anything.
>>
>> If there is more than one such list, then "to a COLO network passthrough
>> list" instead.
>>
>> Still missing then: meaning of absent @object-name.  Does it select the
>> COLO network passthrough list, perhaps?
>>
>>> +#
>>> +# Returns: Nothing on success
>>> +#
>>> +# Since: 6.1
>>> +#
>>> +# Example:
>>> +#
>>> +# -> { "execute": "colo-passthrough-add",
>>> +#      "arguments": { "protocol": "tcp", "object-name": "object0",
>>> +#      "source": {"host": "192.168.1.1", "port": "1234"},
>>> +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
>>> +# <- { "return": {} }
>>> +#
>>> +##
>>> +{ 'command': 'colo-passthrough-add', 'boxed': true,
>>> +     'data': 'IPFlowSpec' }
>>> +
>>> +##
>>> +# @colo-passthrough-del:
>>> +#
>>> +# Delete passthrough entry IPFlowSpec to the COLO-compare instance.
>>> +# The protocol and source/destination IP/ports are optional. if the user
>>> +# only inputs part of the information, this will match all traffic.
>> I suspect this command doesn't actually match traffic, it matches
>> entries added with colo-passthrough-add.
>>
>> Can it delete more than one such entry?
>>
>> Suggest:
>>
>>     # Delete an entry from the COLO network passthrough list.
>>
>> and then explain how the command arguments select entries.
>>
>>> +#
>>> +# Returns: Nothing on success
>>> +#
>>> +# Since: 6.1
>>> +#
>>> +# Example:
>>> +#
>>> +# -> { "execute": "colo-passthrough-del",
>>> +#      "arguments": { "protocol": "tcp", "object-name": "object0",
>>> +#      "source": {"host": "192.168.1.1", "port": "1234"},
>>> +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
>>> +# <- { "return": {} }
>>> +#
>>> +##
>>> +{ 'command': 'colo-passthrough-del', 'boxed': true,
>>> +     'data': 'IPFlowSpec' }


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH V8 1/6] qapi/net: Add IPFlowSpec and QMP command for COLO passthrough
  2021-06-22  6:01       ` chen.zhang
@ 2021-06-22  7:04         ` Jason Wang
  2021-06-22  7:38           ` chen.zhang
  0 siblings, 1 reply; 25+ messages in thread
From: Jason Wang @ 2021-06-22  7:04 UTC (permalink / raw)
  To: chen.zhang, Dr. David Alan Gilbert, Markus Armbruster
  Cc: Lukas Straub, Daniel P. Berrangé,
	Li Zhijian, qemu-dev, Gerd Hoffmann, Zhang Chen, Eric Blake


在 2021/6/22 下午2:01, chen.zhang@intel.com 写道:
>
> On 6/21/21 7:30 PM, Dr. David Alan Gilbert wrote:
>> * Markus Armbruster (armbru@redhat.com) wrote:
>>> Zhang Chen <chen.zhang@intel.com> writes:
>>>
>>>> Since the real user scenario does not need COLO to monitor all 
>>>> traffic.
>>>> Add colo-passthrough-add and colo-passthrough-del to maintain
>>>> a COLO network passthrough list. Add IPFlowSpec struct for all QMP 
>>>> commands.
>>>> All the fields of IPFlowSpec are optional.
>>>>
>>>> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
>>>> ---
>>> The QAPI schema looks good to me, but the interface documentation is
>>> still not quite clear enough.  To make progress, I'm going to make
>>> concrete suggestions wherever I can despite being quite clueless about
>>> the subject matter.  Risks me writing something that's clearer, but
>>> wrong.  Keep that in mind, please.
>>>
>>>>   net/net.c     | 10 +++++++
>>>>   qapi/net.json | 74 
>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>   2 files changed, 84 insertions(+)
>>>>
>>>> diff --git a/net/net.c b/net/net.c
>>>> index 76bbb7c31b..f913e97983 100644
>>>> --- a/net/net.c
>>>> +++ b/net/net.c
>>>> @@ -1195,6 +1195,16 @@ void qmp_netdev_del(const char *id, Error 
>>>> **errp)
>>>>       }
>>>>   }
>>>>   +void qmp_colo_passthrough_add(IPFlowSpec *spec, Error **errp)
>>>> +{
>>>> +    /* TODO implement setup passthrough rule */
>>>> +}
>>>> +
>>>> +void qmp_colo_passthrough_del(IPFlowSpec *spec, Error **errp)
>>>> +{
>>>> +    /* TODO implement delete passthrough rule */
>>>> +}
>>>> +
>>>>   static void netfilter_print_info(Monitor *mon, NetFilterState *nf)
>>>>   {
>>>>       char *str;
>>>> diff --git a/qapi/net.json b/qapi/net.json
>>>> index 7fab2e7cd8..91f2e1495a 100644
>>>> --- a/qapi/net.json
>>>> +++ b/qapi/net.json
>>>> @@ -7,6 +7,7 @@
>>>>   ##
>>>>     { 'include': 'common.json' }
>>>> +{ 'include': 'sockets.json' }
>>>>     ##
>>>>   # @set_link:
>>>> @@ -696,3 +697,76 @@
>>>>   ##
>>>>   { 'event': 'FAILOVER_NEGOTIATED',
>>>>     'data': {'device-id': 'str'} }
>>>> +
>>>> +##
>>>> +# @IPFlowSpec:
>>>> +#
>>>> +# IP flow specification.
>>>> +#
>>>> +# @protocol: Transport layer protocol like TCP/UDP, etc. The 
>>>> protocol is the
>>>> +#            string instead of enum, because it can be passed to 
>>>> getprotobyname(3)
>>>> +#            and avoid duplication with /etc/protocols.
>>> The rationale is good, but it doesn't really belong into the interface
>>> documentation.  Suggest:
>>>
>>>     # @protocol: Transport layer protocol like TCP/UDP, etc. This 
>>> will be
>>>     #            passed to getprotobyname(3).
>>>
>>>
>>>> +#
>>>> +# @object-name: The @object-name means packet handler in Qemu.


I think we need clarify "packet handler" here. It does not look like a 
Qemu terminology.


>>>> Because not
>>>> +#               all the network packet must pass the colo-compare 
>>>> module,
>>>> +#               the net-filters are same situation. There modules 
>>>> attach to
>>>> +#               netdev or chardev to work, VM can run multiple 
>>>> modules
>>>> +#               at the same time. 


This sentence needs some tweak since I fail to understand it's meaning.


>>>> So it needs the object-name to set
>>>> +#               the effective module.
>>> I still don't understand this, and I'm too ignorant of COLO and
>>> networking to suggest improvements.
>>>
>>> Jason or David, perhaps?
>> I'll leave Jason to check on the object behaviour (and I see the rest of
>> the thread); but at a high level, COLO is deciding whether to do VM
>> synchronisation on whether the network behaviour of the two VMs get out
>> of sync (e.g. due to randomness in the flow of requests); if you don't
>> sync then when you fail-over, you'll get TCP errors/inconsistencies in
>> the stream view from the secondary; but this patch series is saying
>> you don't care if some TCP connections fail like that, you only care
>> about maybe the main sockets the application is providing.
>
> Yes, you are right.


I wonder if it's the best to introduce colo specific command here. 
Instead, can we introduce commands to set and get netfilter properties?

Thanks


>
>
> Thanks
>
> Chen
>
>
>> Dave
>>
>>>> +#
>>>> +# @source: Source address and port.
>>>> +#
>>>> +# @destination: Destination address and port.
>>>> +#
>>>> +# Since: 6.1
>>>> +##
>>>> +{ 'struct': 'IPFlowSpec',
>>>> +  'data': { '*protocol': 'str', '*object-name': 'str',
>>>> +    '*source': 'InetSocketAddressBase',
>>>> +    '*destination': 'InetSocketAddressBase' } }
>>>> +
>>>> +##
>>>> +# @colo-passthrough-add:
>>>> +#
>>>> +# Add passthrough entry IPFlowSpec to the COLO-compare instance.
>>>> +# The protocol and source/destination IP/ports are optional. if 
>>>> the user
>>>> +# only inputs part of the information, this will match all traffic.
>>> Actually, all arguments are optional.
>>>
>>> Suggest:
>>>
>>>     # Add an entry to the COLO network passthrough list.
>>>     # Absent protocol, host addresses and ports match anything.
>>>
>>> If there is more than one such list, then "to a COLO network 
>>> passthrough
>>> list" instead.
>>>
>>> Still missing then: meaning of absent @object-name.  Does it select the
>>> COLO network passthrough list, perhaps?
>>>
>>>> +#
>>>> +# Returns: Nothing on success
>>>> +#
>>>> +# Since: 6.1
>>>> +#
>>>> +# Example:
>>>> +#
>>>> +# -> { "execute": "colo-passthrough-add",
>>>> +#      "arguments": { "protocol": "tcp", "object-name": "object0",
>>>> +#      "source": {"host": "192.168.1.1", "port": "1234"},
>>>> +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
>>>> +# <- { "return": {} }
>>>> +#
>>>> +##
>>>> +{ 'command': 'colo-passthrough-add', 'boxed': true,
>>>> +     'data': 'IPFlowSpec' }
>>>> +
>>>> +##
>>>> +# @colo-passthrough-del:
>>>> +#
>>>> +# Delete passthrough entry IPFlowSpec to the COLO-compare instance.
>>>> +# The protocol and source/destination IP/ports are optional. if 
>>>> the user
>>>> +# only inputs part of the information, this will match all traffic.
>>> I suspect this command doesn't actually match traffic, it matches
>>> entries added with colo-passthrough-add.
>>>
>>> Can it delete more than one such entry?
>>>
>>> Suggest:
>>>
>>>     # Delete an entry from the COLO network passthrough list.
>>>
>>> and then explain how the command arguments select entries.
>>>
>>>> +#
>>>> +# Returns: Nothing on success
>>>> +#
>>>> +# Since: 6.1
>>>> +#
>>>> +# Example:
>>>> +#
>>>> +# -> { "execute": "colo-passthrough-del",
>>>> +#      "arguments": { "protocol": "tcp", "object-name": "object0",
>>>> +#      "source": {"host": "192.168.1.1", "port": "1234"},
>>>> +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
>>>> +# <- { "return": {} }
>>>> +#
>>>> +##
>>>> +{ 'command': 'colo-passthrough-del', 'boxed': true,
>>>> +     'data': 'IPFlowSpec' }
>



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH V8 1/6] qapi/net: Add IPFlowSpec and QMP command for COLO passthrough
  2021-06-15 11:37 ` [PATCH V8 1/6] qapi/net: Add IPFlowSpec and QMP command for COLO passthrough Zhang Chen
  2021-06-15 14:43   ` Markus Armbruster
  2021-06-15 15:01   ` Lukas Straub
@ 2021-06-22  7:05   ` Jason Wang
  2021-06-22  7:41     ` chen.zhang
  2 siblings, 1 reply; 25+ messages in thread
From: Jason Wang @ 2021-06-22  7:05 UTC (permalink / raw)
  To: Zhang Chen, qemu-dev, Eric Blake, Dr. David Alan Gilbert,
	Markus Armbruster, Daniel P. Berrangé,
	Gerd Hoffmann, Li Zhijian
  Cc: Lukas Straub, Zhang Chen


在 2021/6/15 下午7:37, Zhang Chen 写道:
> Since the real user scenario does not need COLO to monitor all traffic.
> Add colo-passthrough-add and colo-passthrough-del to maintain
> a COLO network passthrough list. Add IPFlowSpec struct for all QMP commands.
> All the fields of IPFlowSpec are optional.
>
> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> ---
>   net/net.c     | 10 +++++++


Let's avoid to have colo stuffs in the general net codes.

Thanks


>   qapi/net.json | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 84 insertions(+)
>
> diff --git a/net/net.c b/net/net.c
> index 76bbb7c31b..f913e97983 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1195,6 +1195,16 @@ void qmp_netdev_del(const char *id, Error **errp)
>       }
>   }
>   
> +void qmp_colo_passthrough_add(IPFlowSpec *spec, Error **errp)
> +{
> +    /* TODO implement setup passthrough rule */
> +}
> +
> +void qmp_colo_passthrough_del(IPFlowSpec *spec, Error **errp)
> +{
> +    /* TODO implement delete passthrough rule */
> +}
> +
>   static void netfilter_print_info(Monitor *mon, NetFilterState *nf)
>   {
>       char *str;
> diff --git a/qapi/net.json b/qapi/net.json
> index 7fab2e7cd8..91f2e1495a 100644
> --- a/qapi/net.json
> +++ b/qapi/net.json
> @@ -7,6 +7,7 @@
>   ##
>   
>   { 'include': 'common.json' }
> +{ 'include': 'sockets.json' }
>   
>   ##
>   # @set_link:
> @@ -696,3 +697,76 @@
>   ##
>   { 'event': 'FAILOVER_NEGOTIATED',
>     'data': {'device-id': 'str'} }
> +
> +##
> +# @IPFlowSpec:
> +#
> +# IP flow specification.
> +#
> +# @protocol: Transport layer protocol like TCP/UDP, etc. The protocol is the
> +#            string instead of enum, because it can be passed to getprotobyname(3)
> +#            and avoid duplication with /etc/protocols.
> +#
> +# @object-name: The @object-name means packet handler in Qemu. Because not
> +#               all the network packet must pass the colo-compare module,
> +#               the net-filters are same situation. There modules attach to
> +#               netdev or chardev to work, VM can run multiple modules
> +#               at the same time. So it needs the object-name to set
> +#               the effective module.
> +#
> +# @source: Source address and port.
> +#
> +# @destination: Destination address and port.
> +#
> +# Since: 6.1
> +##
> +{ 'struct': 'IPFlowSpec',
> +  'data': { '*protocol': 'str', '*object-name': 'str',
> +    '*source': 'InetSocketAddressBase',
> +    '*destination': 'InetSocketAddressBase' } }
> +
> +##
> +# @colo-passthrough-add:
> +#
> +# Add passthrough entry IPFlowSpec to the COLO-compare instance.
> +# The protocol and source/destination IP/ports are optional. if the user
> +# only inputs part of the information, this will match all traffic.
> +#
> +# Returns: Nothing on success
> +#
> +# Since: 6.1
> +#
> +# Example:
> +#
> +# -> { "execute": "colo-passthrough-add",
> +#      "arguments": { "protocol": "tcp", "object-name": "object0",
> +#      "source": {"host": "192.168.1.1", "port": "1234"},
> +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
> +# <- { "return": {} }
> +#
> +##
> +{ 'command': 'colo-passthrough-add', 'boxed': true,
> +     'data': 'IPFlowSpec' }
> +
> +##
> +# @colo-passthrough-del:
> +#
> +# Delete passthrough entry IPFlowSpec to the COLO-compare instance.
> +# The protocol and source/destination IP/ports are optional. if the user
> +# only inputs part of the information, this will match all traffic.
> +#
> +# Returns: Nothing on success
> +#
> +# Since: 6.1
> +#
> +# Example:
> +#
> +# -> { "execute": "colo-passthrough-del",
> +#      "arguments": { "protocol": "tcp", "object-name": "object0",
> +#      "source": {"host": "192.168.1.1", "port": "1234"},
> +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
> +# <- { "return": {} }
> +#
> +##
> +{ 'command': 'colo-passthrough-del', 'boxed': true,
> +     'data': 'IPFlowSpec' }



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH V8 1/6] qapi/net: Add IPFlowSpec and QMP command for COLO passthrough
  2021-06-22  7:04         ` Jason Wang
@ 2021-06-22  7:38           ` chen.zhang
  2021-06-22  8:03             ` Jason Wang
  0 siblings, 1 reply; 25+ messages in thread
From: chen.zhang @ 2021-06-22  7:38 UTC (permalink / raw)
  To: Jason Wang, Dr. David Alan Gilbert, Markus Armbruster
  Cc: Lukas Straub, Daniel P. Berrangé,
	Li Zhijian, qemu-dev, Gerd Hoffmann, Zhang Chen, Eric Blake


On 6/22/21 3:04 PM, Jason Wang wrote:
> 在 2021/6/22 下午2:01, chen.zhang@intel.com 写道:
>> On 6/21/21 7:30 PM, Dr. David Alan Gilbert wrote:
>>> * Markus Armbruster (armbru@redhat.com) wrote:
>>>> Zhang Chen <chen.zhang@intel.com> writes:
>>>>
>>>>> Since the real user scenario does not need COLO to monitor all
>>>>> traffic.
>>>>> Add colo-passthrough-add and colo-passthrough-del to maintain
>>>>> a COLO network passthrough list. Add IPFlowSpec struct for all QMP
>>>>> commands.
>>>>> All the fields of IPFlowSpec are optional.
>>>>>
>>>>> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
>>>>> ---
>>>> The QAPI schema looks good to me, but the interface documentation is
>>>> still not quite clear enough.  To make progress, I'm going to make
>>>> concrete suggestions wherever I can despite being quite clueless about
>>>> the subject matter.  Risks me writing something that's clearer, but
>>>> wrong.  Keep that in mind, please.
>>>>
>>>>>    net/net.c     | 10 +++++++
>>>>>    qapi/net.json | 74
>>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>    2 files changed, 84 insertions(+)
>>>>>
>>>>> diff --git a/net/net.c b/net/net.c
>>>>> index 76bbb7c31b..f913e97983 100644
>>>>> --- a/net/net.c
>>>>> +++ b/net/net.c
>>>>> @@ -1195,6 +1195,16 @@ void qmp_netdev_del(const char *id, Error
>>>>> **errp)
>>>>>        }
>>>>>    }
>>>>>    +void qmp_colo_passthrough_add(IPFlowSpec *spec, Error **errp)
>>>>> +{
>>>>> +    /* TODO implement setup passthrough rule */
>>>>> +}
>>>>> +
>>>>> +void qmp_colo_passthrough_del(IPFlowSpec *spec, Error **errp)
>>>>> +{
>>>>> +    /* TODO implement delete passthrough rule */
>>>>> +}
>>>>> +
>>>>>    static void netfilter_print_info(Monitor *mon, NetFilterState *nf)
>>>>>    {
>>>>>        char *str;
>>>>> diff --git a/qapi/net.json b/qapi/net.json
>>>>> index 7fab2e7cd8..91f2e1495a 100644
>>>>> --- a/qapi/net.json
>>>>> +++ b/qapi/net.json
>>>>> @@ -7,6 +7,7 @@
>>>>>    ##
>>>>>      { 'include': 'common.json' }
>>>>> +{ 'include': 'sockets.json' }
>>>>>      ##
>>>>>    # @set_link:
>>>>> @@ -696,3 +697,76 @@
>>>>>    ##
>>>>>    { 'event': 'FAILOVER_NEGOTIATED',
>>>>>      'data': {'device-id': 'str'} }
>>>>> +
>>>>> +##
>>>>> +# @IPFlowSpec:
>>>>> +#
>>>>> +# IP flow specification.
>>>>> +#
>>>>> +# @protocol: Transport layer protocol like TCP/UDP, etc. The
>>>>> protocol is the
>>>>> +#            string instead of enum, because it can be passed to
>>>>> getprotobyname(3)
>>>>> +#            and avoid duplication with /etc/protocols.
>>>> The rationale is good, but it doesn't really belong into the interface
>>>> documentation.  Suggest:
>>>>
>>>>      # @protocol: Transport layer protocol like TCP/UDP, etc. This
>>>> will be
>>>>      #            passed to getprotobyname(3).
>>>>
>>>>
>>>>> +#
>>>>> +# @object-name: The @object-name means packet handler in Qemu.
>
> I think we need clarify "packet handler" here. It does not look like a
> Qemu terminology.


OK.

The @object-name means a qemu object with network packet processing function, for example colo-compare, filtr-mirror, etc.

I will add it in the next version.


>
>
>>>>> Because not
>>>>> +#               all the network packet must pass the colo-compare
>>>>> module,
>>>>> +#               the net-filters are same situation. There modules
>>>>> attach to
>>>>> +#               netdev or chardev to work, VM can run multiple
>>>>> modules
>>>>> +#               at the same time.
>
> This sentence needs some tweak since I fail to understand it's meaning.
>

OK. Change to:

VM can running with multi network packet processing function objects.

They can control different network data paths from netdev or chardev.

So it needs the object-name to set the effective module.


>>>>> So it needs the object-name to set
>>>>> +#               the effective module.
>>>> I still don't understand this, and I'm too ignorant of COLO and
>>>> networking to suggest improvements.
>>>>
>>>> Jason or David, perhaps?
>>> I'll leave Jason to check on the object behaviour (and I see the rest of
>>> the thread); but at a high level, COLO is deciding whether to do VM
>>> synchronisation on whether the network behaviour of the two VMs get out
>>> of sync (e.g. due to randomness in the flow of requests); if you don't
>>> sync then when you fail-over, you'll get TCP errors/inconsistencies in
>>> the stream view from the secondary; but this patch series is saying
>>> you don't care if some TCP connections fail like that, you only care
>>> about maybe the main sockets the application is providing.
>> Yes, you are right.
>
> I wonder if it's the best to introduce colo specific command here.
> Instead, can we introduce commands to set and get netfilter properties?

This series has not added pass-through support for other netfilters,

Can we change the qmp command to passthrough-filter-add/del as markus's 
comments in this series,

And enable filters pass-through capability and add properties in next 
series. Step by step.


Thanks

Chen


>
> Thanks
>
>
>>
>> Thanks
>>
>> Chen
>>
>>
>>> Dave
>>>
>>>>> +#
>>>>> +# @source: Source address and port.
>>>>> +#
>>>>> +# @destination: Destination address and port.
>>>>> +#
>>>>> +# Since: 6.1
>>>>> +##
>>>>> +{ 'struct': 'IPFlowSpec',
>>>>> +  'data': { '*protocol': 'str', '*object-name': 'str',
>>>>> +    '*source': 'InetSocketAddressBase',
>>>>> +    '*destination': 'InetSocketAddressBase' } }
>>>>> +
>>>>> +##
>>>>> +# @colo-passthrough-add:
>>>>> +#
>>>>> +# Add passthrough entry IPFlowSpec to the COLO-compare instance.
>>>>> +# The protocol and source/destination IP/ports are optional. if
>>>>> the user
>>>>> +# only inputs part of the information, this will match all traffic.
>>>> Actually, all arguments are optional.
>>>>
>>>> Suggest:
>>>>
>>>>      # Add an entry to the COLO network passthrough list.
>>>>      # Absent protocol, host addresses and ports match anything.
>>>>
>>>> If there is more than one such list, then "to a COLO network
>>>> passthrough
>>>> list" instead.
>>>>
>>>> Still missing then: meaning of absent @object-name.  Does it select the
>>>> COLO network passthrough list, perhaps?
>>>>
>>>>> +#
>>>>> +# Returns: Nothing on success
>>>>> +#
>>>>> +# Since: 6.1
>>>>> +#
>>>>> +# Example:
>>>>> +#
>>>>> +# -> { "execute": "colo-passthrough-add",
>>>>> +#      "arguments": { "protocol": "tcp", "object-name": "object0",
>>>>> +#      "source": {"host": "192.168.1.1", "port": "1234"},
>>>>> +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
>>>>> +# <- { "return": {} }
>>>>> +#
>>>>> +##
>>>>> +{ 'command': 'colo-passthrough-add', 'boxed': true,
>>>>> +     'data': 'IPFlowSpec' }
>>>>> +
>>>>> +##
>>>>> +# @colo-passthrough-del:
>>>>> +#
>>>>> +# Delete passthrough entry IPFlowSpec to the COLO-compare instance.
>>>>> +# The protocol and source/destination IP/ports are optional. if
>>>>> the user
>>>>> +# only inputs part of the information, this will match all traffic.
>>>> I suspect this command doesn't actually match traffic, it matches
>>>> entries added with colo-passthrough-add.
>>>>
>>>> Can it delete more than one such entry?
>>>>
>>>> Suggest:
>>>>
>>>>      # Delete an entry from the COLO network passthrough list.
>>>>
>>>> and then explain how the command arguments select entries.
>>>>
>>>>> +#
>>>>> +# Returns: Nothing on success
>>>>> +#
>>>>> +# Since: 6.1
>>>>> +#
>>>>> +# Example:
>>>>> +#
>>>>> +# -> { "execute": "colo-passthrough-del",
>>>>> +#      "arguments": { "protocol": "tcp", "object-name": "object0",
>>>>> +#      "source": {"host": "192.168.1.1", "port": "1234"},
>>>>> +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
>>>>> +# <- { "return": {} }
>>>>> +#
>>>>> +##
>>>>> +{ 'command': 'colo-passthrough-del', 'boxed': true,
>>>>> +     'data': 'IPFlowSpec' }
>


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH V8 1/6] qapi/net: Add IPFlowSpec and QMP command for COLO passthrough
  2021-06-22  7:05   ` Jason Wang
@ 2021-06-22  7:41     ` chen.zhang
  0 siblings, 0 replies; 25+ messages in thread
From: chen.zhang @ 2021-06-22  7:41 UTC (permalink / raw)
  To: Jason Wang, qemu-dev, Eric Blake, Dr. David Alan Gilbert,
	Markus Armbruster, Daniel P. Berrangé,
	Gerd Hoffmann, Li Zhijian
  Cc: Lukas Straub, Zhang Chen


On 6/22/21 3:05 PM, Jason Wang wrote:
> 在 2021/6/15 下午7:37, Zhang Chen 写道:
>> Since the real user scenario does not need COLO to monitor all traffic.
>> Add colo-passthrough-add and colo-passthrough-del to maintain
>> a COLO network passthrough list. Add IPFlowSpec struct for all QMP commands.
>> All the fields of IPFlowSpec are optional.
>>
>> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
>> ---
>>    net/net.c     | 10 +++++++
>
> Let's avoid to have colo stuffs in the general net codes.


As we will change the command to "passthrough-filter-add/del", the data 
structure and commands are the general net codes.


Thanks

Chen


>
> Thanks
>
>
>>    qapi/net.json | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>    2 files changed, 84 insertions(+)
>>
>> diff --git a/net/net.c b/net/net.c
>> index 76bbb7c31b..f913e97983 100644
>> --- a/net/net.c
>> +++ b/net/net.c
>> @@ -1195,6 +1195,16 @@ void qmp_netdev_del(const char *id, Error **errp)
>>        }
>>    }
>>    
>> +void qmp_colo_passthrough_add(IPFlowSpec *spec, Error **errp)
>> +{
>> +    /* TODO implement setup passthrough rule */
>> +}
>> +
>> +void qmp_colo_passthrough_del(IPFlowSpec *spec, Error **errp)
>> +{
>> +    /* TODO implement delete passthrough rule */
>> +}
>> +
>>    static void netfilter_print_info(Monitor *mon, NetFilterState *nf)
>>    {
>>        char *str;
>> diff --git a/qapi/net.json b/qapi/net.json
>> index 7fab2e7cd8..91f2e1495a 100644
>> --- a/qapi/net.json
>> +++ b/qapi/net.json
>> @@ -7,6 +7,7 @@
>>    ##
>>    
>>    { 'include': 'common.json' }
>> +{ 'include': 'sockets.json' }
>>    
>>    ##
>>    # @set_link:
>> @@ -696,3 +697,76 @@
>>    ##
>>    { 'event': 'FAILOVER_NEGOTIATED',
>>      'data': {'device-id': 'str'} }
>> +
>> +##
>> +# @IPFlowSpec:
>> +#
>> +# IP flow specification.
>> +#
>> +# @protocol: Transport layer protocol like TCP/UDP, etc. The protocol is the
>> +#            string instead of enum, because it can be passed to getprotobyname(3)
>> +#            and avoid duplication with /etc/protocols.
>> +#
>> +# @object-name: The @object-name means packet handler in Qemu. Because not
>> +#               all the network packet must pass the colo-compare module,
>> +#               the net-filters are same situation. There modules attach to
>> +#               netdev or chardev to work, VM can run multiple modules
>> +#               at the same time. So it needs the object-name to set
>> +#               the effective module.
>> +#
>> +# @source: Source address and port.
>> +#
>> +# @destination: Destination address and port.
>> +#
>> +# Since: 6.1
>> +##
>> +{ 'struct': 'IPFlowSpec',
>> +  'data': { '*protocol': 'str', '*object-name': 'str',
>> +    '*source': 'InetSocketAddressBase',
>> +    '*destination': 'InetSocketAddressBase' } }
>> +
>> +##
>> +# @colo-passthrough-add:
>> +#
>> +# Add passthrough entry IPFlowSpec to the COLO-compare instance.
>> +# The protocol and source/destination IP/ports are optional. if the user
>> +# only inputs part of the information, this will match all traffic.
>> +#
>> +# Returns: Nothing on success
>> +#
>> +# Since: 6.1
>> +#
>> +# Example:
>> +#
>> +# -> { "execute": "colo-passthrough-add",
>> +#      "arguments": { "protocol": "tcp", "object-name": "object0",
>> +#      "source": {"host": "192.168.1.1", "port": "1234"},
>> +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
>> +# <- { "return": {} }
>> +#
>> +##
>> +{ 'command': 'colo-passthrough-add', 'boxed': true,
>> +     'data': 'IPFlowSpec' }
>> +
>> +##
>> +# @colo-passthrough-del:
>> +#
>> +# Delete passthrough entry IPFlowSpec to the COLO-compare instance.
>> +# The protocol and source/destination IP/ports are optional. if the user
>> +# only inputs part of the information, this will match all traffic.
>> +#
>> +# Returns: Nothing on success
>> +#
>> +# Since: 6.1
>> +#
>> +# Example:
>> +#
>> +# -> { "execute": "colo-passthrough-del",
>> +#      "arguments": { "protocol": "tcp", "object-name": "object0",
>> +#      "source": {"host": "192.168.1.1", "port": "1234"},
>> +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
>> +# <- { "return": {} }
>> +#
>> +##
>> +{ 'command': 'colo-passthrough-del', 'boxed': true,
>> +     'data': 'IPFlowSpec' }


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH V8 1/6] qapi/net: Add IPFlowSpec and QMP command for COLO passthrough
  2021-06-22  8:03             ` Jason Wang
@ 2021-06-22  8:02               ` chen.zhang
  0 siblings, 0 replies; 25+ messages in thread
From: chen.zhang @ 2021-06-22  8:02 UTC (permalink / raw)
  To: Jason Wang, Dr. David Alan Gilbert, Markus Armbruster
  Cc: Lukas Straub, Daniel P. Berrangé,
	Li Zhijian, qemu-dev, Gerd Hoffmann, Zhang Chen, Eric Blake


On 6/22/21 4:03 PM, Jason Wang wrote:
> 在 2021/6/22 下午3:38, chen.zhang@intel.com 写道:
>> On 6/22/21 3:04 PM, Jason Wang wrote:
>>> 在 2021/6/22 下午2:01, chen.zhang@intel.com 写道:
>>>> On 6/21/21 7:30 PM, Dr. David Alan Gilbert wrote:
>>>>> * Markus Armbruster (armbru@redhat.com) wrote:
>>>>>> Zhang Chen <chen.zhang@intel.com> writes:
>>>>>>
>>>>>>> Since the real user scenario does not need COLO to monitor all
>>>>>>> traffic.
>>>>>>> Add colo-passthrough-add and colo-passthrough-del to maintain
>>>>>>> a COLO network passthrough list. Add IPFlowSpec struct for all QMP
>>>>>>> commands.
>>>>>>> All the fields of IPFlowSpec are optional.
>>>>>>>
>>>>>>> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
>>>>>>> ---
>>>>>> The QAPI schema looks good to me, but the interface documentation is
>>>>>> still not quite clear enough.  To make progress, I'm going to make
>>>>>> concrete suggestions wherever I can despite being quite clueless
>>>>>> about
>>>>>> the subject matter.  Risks me writing something that's clearer, but
>>>>>> wrong.  Keep that in mind, please.
>>>>>>
>>>>>>>     net/net.c     | 10 +++++++
>>>>>>>     qapi/net.json | 74
>>>>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>     2 files changed, 84 insertions(+)
>>>>>>>
>>>>>>> diff --git a/net/net.c b/net/net.c
>>>>>>> index 76bbb7c31b..f913e97983 100644
>>>>>>> --- a/net/net.c
>>>>>>> +++ b/net/net.c
>>>>>>> @@ -1195,6 +1195,16 @@ void qmp_netdev_del(const char *id, Error
>>>>>>> **errp)
>>>>>>>         }
>>>>>>>     }
>>>>>>>     +void qmp_colo_passthrough_add(IPFlowSpec *spec, Error **errp)
>>>>>>> +{
>>>>>>> +    /* TODO implement setup passthrough rule */
>>>>>>> +}
>>>>>>> +
>>>>>>> +void qmp_colo_passthrough_del(IPFlowSpec *spec, Error **errp)
>>>>>>> +{
>>>>>>> +    /* TODO implement delete passthrough rule */
>>>>>>> +}
>>>>>>> +
>>>>>>>     static void netfilter_print_info(Monitor *mon, NetFilterState
>>>>>>> *nf)
>>>>>>>     {
>>>>>>>         char *str;
>>>>>>> diff --git a/qapi/net.json b/qapi/net.json
>>>>>>> index 7fab2e7cd8..91f2e1495a 100644
>>>>>>> --- a/qapi/net.json
>>>>>>> +++ b/qapi/net.json
>>>>>>> @@ -7,6 +7,7 @@
>>>>>>>     ##
>>>>>>>       { 'include': 'common.json' }
>>>>>>> +{ 'include': 'sockets.json' }
>>>>>>>       ##
>>>>>>>     # @set_link:
>>>>>>> @@ -696,3 +697,76 @@
>>>>>>>     ##
>>>>>>>     { 'event': 'FAILOVER_NEGOTIATED',
>>>>>>>       'data': {'device-id': 'str'} }
>>>>>>> +
>>>>>>> +##
>>>>>>> +# @IPFlowSpec:
>>>>>>> +#
>>>>>>> +# IP flow specification.
>>>>>>> +#
>>>>>>> +# @protocol: Transport layer protocol like TCP/UDP, etc. The
>>>>>>> protocol is the
>>>>>>> +#            string instead of enum, because it can be passed to
>>>>>>> getprotobyname(3)
>>>>>>> +#            and avoid duplication with /etc/protocols.
>>>>>> The rationale is good, but it doesn't really belong into the
>>>>>> interface
>>>>>> documentation.  Suggest:
>>>>>>
>>>>>>       # @protocol: Transport layer protocol like TCP/UDP, etc. This
>>>>>> will be
>>>>>>       #            passed to getprotobyname(3).
>>>>>>
>>>>>>
>>>>>>> +#
>>>>>>> +# @object-name: The @object-name means packet handler in Qemu.
>>> I think we need clarify "packet handler" here. It does not look like a
>>> Qemu terminology.
>>
>> OK.
>>
>> The @object-name means a qemu object with network packet processing
>> function, for example colo-compare, filtr-mirror, etc.
>>
>> I will add it in the next version.
>
> So if two net-filters want to use exact the same set of flowspec, we
> need to specify them twice?


Yes. Each filter has it's own pass-through list.


Thanks

Chen


>
>>
>>>
>>>>>>> Because not
>>>>>>> +#               all the network packet must pass the colo-compare
>>>>>>> module,
>>>>>>> +#               the net-filters are same situation. There modules
>>>>>>> attach to
>>>>>>> +#               netdev or chardev to work, VM can run multiple
>>>>>>> modules
>>>>>>> +#               at the same time.
>>> This sentence needs some tweak since I fail to understand it's meaning.
>>>
>> OK. Change to:
>>
>> VM can running with multi network packet processing function objects.
>>
>> They can control different network data paths from netdev or chardev.
>>
>> So it needs the object-name to set the effective module.
>>
>>
>>>>>>> So it needs the object-name to set
>>>>>>> +#               the effective module.
>>>>>> I still don't understand this, and I'm too ignorant of COLO and
>>>>>> networking to suggest improvements.
>>>>>>
>>>>>> Jason or David, perhaps?
>>>>> I'll leave Jason to check on the object behaviour (and I see the
>>>>> rest of
>>>>> the thread); but at a high level, COLO is deciding whether to do VM
>>>>> synchronisation on whether the network behaviour of the two VMs get
>>>>> out
>>>>> of sync (e.g. due to randomness in the flow of requests); if you don't
>>>>> sync then when you fail-over, you'll get TCP errors/inconsistencies in
>>>>> the stream view from the secondary; but this patch series is saying
>>>>> you don't care if some TCP connections fail like that, you only care
>>>>> about maybe the main sockets the application is providing.
>>>> Yes, you are right.
>>> I wonder if it's the best to introduce colo specific command here.
>>> Instead, can we introduce commands to set and get netfilter properties?
>> This series has not added pass-through support for other netfilters,
>>
>> Can we change the qmp command to passthrough-filter-add/del as
>> markus's comments in this series,
>
> It looks more general, so I think it should work.
>
> Thanks
>
>
>> And enable filters pass-through capability and add properties in next
>> series. Step by step.
>>
>>
>> Thanks
>>
>> Chen
>>
>>
>>> Thanks
>>>
>>>
>>>> Thanks
>>>>
>>>> Chen
>>>>
>>>>
>>>>> Dave
>>>>>
>>>>>>> +#
>>>>>>> +# @source: Source address and port.
>>>>>>> +#
>>>>>>> +# @destination: Destination address and port.
>>>>>>> +#
>>>>>>> +# Since: 6.1
>>>>>>> +##
>>>>>>> +{ 'struct': 'IPFlowSpec',
>>>>>>> +  'data': { '*protocol': 'str', '*object-name': 'str',
>>>>>>> +    '*source': 'InetSocketAddressBase',
>>>>>>> +    '*destination': 'InetSocketAddressBase' } }
>>>>>>> +
>>>>>>> +##
>>>>>>> +# @colo-passthrough-add:
>>>>>>> +#
>>>>>>> +# Add passthrough entry IPFlowSpec to the COLO-compare instance.
>>>>>>> +# The protocol and source/destination IP/ports are optional. if
>>>>>>> the user
>>>>>>> +# only inputs part of the information, this will match all traffic.
>>>>>> Actually, all arguments are optional.
>>>>>>
>>>>>> Suggest:
>>>>>>
>>>>>>       # Add an entry to the COLO network passthrough list.
>>>>>>       # Absent protocol, host addresses and ports match anything.
>>>>>>
>>>>>> If there is more than one such list, then "to a COLO network
>>>>>> passthrough
>>>>>> list" instead.
>>>>>>
>>>>>> Still missing then: meaning of absent @object-name.  Does it
>>>>>> select the
>>>>>> COLO network passthrough list, perhaps?
>>>>>>
>>>>>>> +#
>>>>>>> +# Returns: Nothing on success
>>>>>>> +#
>>>>>>> +# Since: 6.1
>>>>>>> +#
>>>>>>> +# Example:
>>>>>>> +#
>>>>>>> +# -> { "execute": "colo-passthrough-add",
>>>>>>> +#      "arguments": { "protocol": "tcp", "object-name": "object0",
>>>>>>> +#      "source": {"host": "192.168.1.1", "port": "1234"},
>>>>>>> +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
>>>>>>> +# <- { "return": {} }
>>>>>>> +#
>>>>>>> +##
>>>>>>> +{ 'command': 'colo-passthrough-add', 'boxed': true,
>>>>>>> +     'data': 'IPFlowSpec' }
>>>>>>> +
>>>>>>> +##
>>>>>>> +# @colo-passthrough-del:
>>>>>>> +#
>>>>>>> +# Delete passthrough entry IPFlowSpec to the COLO-compare instance.
>>>>>>> +# The protocol and source/destination IP/ports are optional. if
>>>>>>> the user
>>>>>>> +# only inputs part of the information, this will match all traffic.
>>>>>> I suspect this command doesn't actually match traffic, it matches
>>>>>> entries added with colo-passthrough-add.
>>>>>>
>>>>>> Can it delete more than one such entry?
>>>>>>
>>>>>> Suggest:
>>>>>>
>>>>>>       # Delete an entry from the COLO network passthrough list.
>>>>>>
>>>>>> and then explain how the command arguments select entries.
>>>>>>
>>>>>>> +#
>>>>>>> +# Returns: Nothing on success
>>>>>>> +#
>>>>>>> +# Since: 6.1
>>>>>>> +#
>>>>>>> +# Example:
>>>>>>> +#
>>>>>>> +# -> { "execute": "colo-passthrough-del",
>>>>>>> +#      "arguments": { "protocol": "tcp", "object-name": "object0",
>>>>>>> +#      "source": {"host": "192.168.1.1", "port": "1234"},
>>>>>>> +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
>>>>>>> +# <- { "return": {} }
>>>>>>> +#
>>>>>>> +##
>>>>>>> +{ 'command': 'colo-passthrough-del', 'boxed': true,
>>>>>>> +     'data': 'IPFlowSpec' }


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH V8 1/6] qapi/net: Add IPFlowSpec and QMP command for COLO passthrough
  2021-06-22  7:38           ` chen.zhang
@ 2021-06-22  8:03             ` Jason Wang
  2021-06-22  8:02               ` chen.zhang
  0 siblings, 1 reply; 25+ messages in thread
From: Jason Wang @ 2021-06-22  8:03 UTC (permalink / raw)
  To: chen.zhang, Dr. David Alan Gilbert, Markus Armbruster
  Cc: Lukas Straub, Daniel P. Berrangé,
	Li Zhijian, qemu-dev, Gerd Hoffmann, Zhang Chen, Eric Blake


在 2021/6/22 下午3:38, chen.zhang@intel.com 写道:
>
> On 6/22/21 3:04 PM, Jason Wang wrote:
>> 在 2021/6/22 下午2:01, chen.zhang@intel.com 写道:
>>> On 6/21/21 7:30 PM, Dr. David Alan Gilbert wrote:
>>>> * Markus Armbruster (armbru@redhat.com) wrote:
>>>>> Zhang Chen <chen.zhang@intel.com> writes:
>>>>>
>>>>>> Since the real user scenario does not need COLO to monitor all
>>>>>> traffic.
>>>>>> Add colo-passthrough-add and colo-passthrough-del to maintain
>>>>>> a COLO network passthrough list. Add IPFlowSpec struct for all QMP
>>>>>> commands.
>>>>>> All the fields of IPFlowSpec are optional.
>>>>>>
>>>>>> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
>>>>>> ---
>>>>> The QAPI schema looks good to me, but the interface documentation is
>>>>> still not quite clear enough.  To make progress, I'm going to make
>>>>> concrete suggestions wherever I can despite being quite clueless 
>>>>> about
>>>>> the subject matter.  Risks me writing something that's clearer, but
>>>>> wrong.  Keep that in mind, please.
>>>>>
>>>>>>    net/net.c     | 10 +++++++
>>>>>>    qapi/net.json | 74
>>>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>    2 files changed, 84 insertions(+)
>>>>>>
>>>>>> diff --git a/net/net.c b/net/net.c
>>>>>> index 76bbb7c31b..f913e97983 100644
>>>>>> --- a/net/net.c
>>>>>> +++ b/net/net.c
>>>>>> @@ -1195,6 +1195,16 @@ void qmp_netdev_del(const char *id, Error
>>>>>> **errp)
>>>>>>        }
>>>>>>    }
>>>>>>    +void qmp_colo_passthrough_add(IPFlowSpec *spec, Error **errp)
>>>>>> +{
>>>>>> +    /* TODO implement setup passthrough rule */
>>>>>> +}
>>>>>> +
>>>>>> +void qmp_colo_passthrough_del(IPFlowSpec *spec, Error **errp)
>>>>>> +{
>>>>>> +    /* TODO implement delete passthrough rule */
>>>>>> +}
>>>>>> +
>>>>>>    static void netfilter_print_info(Monitor *mon, NetFilterState 
>>>>>> *nf)
>>>>>>    {
>>>>>>        char *str;
>>>>>> diff --git a/qapi/net.json b/qapi/net.json
>>>>>> index 7fab2e7cd8..91f2e1495a 100644
>>>>>> --- a/qapi/net.json
>>>>>> +++ b/qapi/net.json
>>>>>> @@ -7,6 +7,7 @@
>>>>>>    ##
>>>>>>      { 'include': 'common.json' }
>>>>>> +{ 'include': 'sockets.json' }
>>>>>>      ##
>>>>>>    # @set_link:
>>>>>> @@ -696,3 +697,76 @@
>>>>>>    ##
>>>>>>    { 'event': 'FAILOVER_NEGOTIATED',
>>>>>>      'data': {'device-id': 'str'} }
>>>>>> +
>>>>>> +##
>>>>>> +# @IPFlowSpec:
>>>>>> +#
>>>>>> +# IP flow specification.
>>>>>> +#
>>>>>> +# @protocol: Transport layer protocol like TCP/UDP, etc. The
>>>>>> protocol is the
>>>>>> +#            string instead of enum, because it can be passed to
>>>>>> getprotobyname(3)
>>>>>> +#            and avoid duplication with /etc/protocols.
>>>>> The rationale is good, but it doesn't really belong into the 
>>>>> interface
>>>>> documentation.  Suggest:
>>>>>
>>>>>      # @protocol: Transport layer protocol like TCP/UDP, etc. This
>>>>> will be
>>>>>      #            passed to getprotobyname(3).
>>>>>
>>>>>
>>>>>> +#
>>>>>> +# @object-name: The @object-name means packet handler in Qemu.
>>
>> I think we need clarify "packet handler" here. It does not look like a
>> Qemu terminology.
>
>
> OK.
>
> The @object-name means a qemu object with network packet processing 
> function, for example colo-compare, filtr-mirror, etc.
>
> I will add it in the next version.


So if two net-filters want to use exact the same set of flowspec, we 
need to specify them twice?


>
>
>>
>>
>>>>>> Because not
>>>>>> +#               all the network packet must pass the colo-compare
>>>>>> module,
>>>>>> +#               the net-filters are same situation. There modules
>>>>>> attach to
>>>>>> +#               netdev or chardev to work, VM can run multiple
>>>>>> modules
>>>>>> +#               at the same time.
>>
>> This sentence needs some tweak since I fail to understand it's meaning.
>>
>
> OK. Change to:
>
> VM can running with multi network packet processing function objects.
>
> They can control different network data paths from netdev or chardev.
>
> So it needs the object-name to set the effective module.
>
>
>>>>>> So it needs the object-name to set
>>>>>> +#               the effective module.
>>>>> I still don't understand this, and I'm too ignorant of COLO and
>>>>> networking to suggest improvements.
>>>>>
>>>>> Jason or David, perhaps?
>>>> I'll leave Jason to check on the object behaviour (and I see the 
>>>> rest of
>>>> the thread); but at a high level, COLO is deciding whether to do VM
>>>> synchronisation on whether the network behaviour of the two VMs get 
>>>> out
>>>> of sync (e.g. due to randomness in the flow of requests); if you don't
>>>> sync then when you fail-over, you'll get TCP errors/inconsistencies in
>>>> the stream view from the secondary; but this patch series is saying
>>>> you don't care if some TCP connections fail like that, you only care
>>>> about maybe the main sockets the application is providing.
>>> Yes, you are right.
>>
>> I wonder if it's the best to introduce colo specific command here.
>> Instead, can we introduce commands to set and get netfilter properties?
>
> This series has not added pass-through support for other netfilters,
>
> Can we change the qmp command to passthrough-filter-add/del as 
> markus's comments in this series,


It looks more general, so I think it should work.

Thanks


>
> And enable filters pass-through capability and add properties in next 
> series. Step by step.
>
>
> Thanks
>
> Chen
>
>
>>
>> Thanks
>>
>>
>>>
>>> Thanks
>>>
>>> Chen
>>>
>>>
>>>> Dave
>>>>
>>>>>> +#
>>>>>> +# @source: Source address and port.
>>>>>> +#
>>>>>> +# @destination: Destination address and port.
>>>>>> +#
>>>>>> +# Since: 6.1
>>>>>> +##
>>>>>> +{ 'struct': 'IPFlowSpec',
>>>>>> +  'data': { '*protocol': 'str', '*object-name': 'str',
>>>>>> +    '*source': 'InetSocketAddressBase',
>>>>>> +    '*destination': 'InetSocketAddressBase' } }
>>>>>> +
>>>>>> +##
>>>>>> +# @colo-passthrough-add:
>>>>>> +#
>>>>>> +# Add passthrough entry IPFlowSpec to the COLO-compare instance.
>>>>>> +# The protocol and source/destination IP/ports are optional. if
>>>>>> the user
>>>>>> +# only inputs part of the information, this will match all traffic.
>>>>> Actually, all arguments are optional.
>>>>>
>>>>> Suggest:
>>>>>
>>>>>      # Add an entry to the COLO network passthrough list.
>>>>>      # Absent protocol, host addresses and ports match anything.
>>>>>
>>>>> If there is more than one such list, then "to a COLO network
>>>>> passthrough
>>>>> list" instead.
>>>>>
>>>>> Still missing then: meaning of absent @object-name.  Does it 
>>>>> select the
>>>>> COLO network passthrough list, perhaps?
>>>>>
>>>>>> +#
>>>>>> +# Returns: Nothing on success
>>>>>> +#
>>>>>> +# Since: 6.1
>>>>>> +#
>>>>>> +# Example:
>>>>>> +#
>>>>>> +# -> { "execute": "colo-passthrough-add",
>>>>>> +#      "arguments": { "protocol": "tcp", "object-name": "object0",
>>>>>> +#      "source": {"host": "192.168.1.1", "port": "1234"},
>>>>>> +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
>>>>>> +# <- { "return": {} }
>>>>>> +#
>>>>>> +##
>>>>>> +{ 'command': 'colo-passthrough-add', 'boxed': true,
>>>>>> +     'data': 'IPFlowSpec' }
>>>>>> +
>>>>>> +##
>>>>>> +# @colo-passthrough-del:
>>>>>> +#
>>>>>> +# Delete passthrough entry IPFlowSpec to the COLO-compare instance.
>>>>>> +# The protocol and source/destination IP/ports are optional. if
>>>>>> the user
>>>>>> +# only inputs part of the information, this will match all traffic.
>>>>> I suspect this command doesn't actually match traffic, it matches
>>>>> entries added with colo-passthrough-add.
>>>>>
>>>>> Can it delete more than one such entry?
>>>>>
>>>>> Suggest:
>>>>>
>>>>>      # Delete an entry from the COLO network passthrough list.
>>>>>
>>>>> and then explain how the command arguments select entries.
>>>>>
>>>>>> +#
>>>>>> +# Returns: Nothing on success
>>>>>> +#
>>>>>> +# Since: 6.1
>>>>>> +#
>>>>>> +# Example:
>>>>>> +#
>>>>>> +# -> { "execute": "colo-passthrough-del",
>>>>>> +#      "arguments": { "protocol": "tcp", "object-name": "object0",
>>>>>> +#      "source": {"host": "192.168.1.1", "port": "1234"},
>>>>>> +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
>>>>>> +# <- { "return": {} }
>>>>>> +#
>>>>>> +##
>>>>>> +{ 'command': 'colo-passthrough-del', 'boxed': true,
>>>>>> +     'data': 'IPFlowSpec' }
>>
>



^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2021-06-22  8:10 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-15 11:37 [PATCH V8 0/6] Passthrough specific network traffic in COLO Zhang Chen
2021-06-15 11:37 ` [PATCH V8 1/6] qapi/net: Add IPFlowSpec and QMP command for COLO passthrough Zhang Chen
2021-06-15 14:43   ` Markus Armbruster
2021-06-16  2:12     ` Zhang, Chen
2021-06-16  6:04       ` Markus Armbruster
2021-06-16  6:45         ` Zhang, Chen
2021-06-16 13:26           ` Markus Armbruster
2021-06-17  3:27             ` Zhang, Chen
2021-06-17 11:03               ` Markus Armbruster
2021-06-22  5:58                 ` chen.zhang
2021-06-21 11:30     ` Dr. David Alan Gilbert
2021-06-22  6:01       ` chen.zhang
2021-06-22  7:04         ` Jason Wang
2021-06-22  7:38           ` chen.zhang
2021-06-22  8:03             ` Jason Wang
2021-06-22  8:02               ` chen.zhang
2021-06-15 15:01   ` Lukas Straub
2021-06-16  1:20     ` Zhang, Chen
2021-06-22  7:05   ` Jason Wang
2021-06-22  7:41     ` chen.zhang
2021-06-15 11:37 ` [PATCH V8 2/6] util/qemu-sockets.c: Add inet_parse_base to handle InetSocketAddressBase Zhang Chen
2021-06-15 11:37 ` [PATCH V8 3/6] hmp-commands: Add new HMP command for COLO passthrough Zhang Chen
2021-06-15 11:37 ` [PATCH V8 4/6] net/colo-compare: Move data structure and define to .h file Zhang Chen
2021-06-15 11:37 ` [PATCH V8 5/6] net/colo-compare: Add passthrough list to CompareState Zhang Chen
2021-06-15 11:37 ` [PATCH V8 6/6] net/net.c: Add handler for COLO passthrough connection Zhang Chen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.