[PATCH V4 0/7] Bypass specific network traffic in COLO

[PATCH V4 0/7] Bypass specific network traffic in COLO

[Zhang Chen]

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

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

  V3:
    - Add COLO passthrough list lock.
    - Add usage demo and more comments.

  V2:
    - Add the n-tuple support.
    - Add some qapi definitions.
    - Support multi colo-compare objects.
    - Support setup each rules for each objects individually.
    - Clean up COLO compare definition to .h file.
    - Rebase HMP command for stable tree.
    - Add redundant rules check.


Zhang Chen (7):
  qapi/net.json: Add IP_PROTOCOL definition
  qapi/net.json: Add L4_Connection definition
  qapi/net: Add new QMP command for COLO passthrough
  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 +
 monitor/hmp-cmds.c    |  34 +++++++++
 net/colo-compare.c    | 135 ++++++++--------------------------
 net/colo-compare.h    | 117 ++++++++++++++++++++++++++++++
 net/net.c             | 163 ++++++++++++++++++++++++++++++++++++++++++
 qapi/net.json         |  97 +++++++++++++++++++++++++
 7 files changed, 468 insertions(+), 106 deletions(-)

-- 
2.25.1

[PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition

[Zhang Chen]

Add IP_PROTOCOL as enum include TCP,UDP, ICMP... for other QMP commands.

Signed-off-by: Zhang Chen <chen.zhang@intel.com>
---
 qapi/net.json | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/qapi/net.json b/qapi/net.json
index 87361ebd9a..498ea7aa72 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -794,3 +794,34 @@
 #
 ##
 { 'command': 'query-netdev', 'returns': ['NetdevInfo'] }
+
+##
+# @IP_PROTOCOL:
+#
+# Transport layer protocol.
+#
+# Just for IPv4.
+#
+# @tcp: Transmission Control Protocol.
+#
+# @udp: User Datagram Protocol.
+#
+# @dccp: Datagram Congestion Control Protocol.
+#
+# @sctp: Stream Control Transmission Protocol.
+#
+# @udplite: Lightweight User Datagram Protocol.
+#
+# @icmp: Internet Control Message Protocol.
+#
+# @igmp: Internet Group Management Protocol.
+#
+# @ipv6: IPv6 Encapsulation.
+#
+# TODO: Need to add more transport layer protocol.
+#
+# Since: 6.1
+##
+{ 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite',
+    'icmp', 'igmp', 'ipv6' ] }
+
-- 
2.25.1

[PATCH V4 2/7] qapi/net.json: Add L4_Connection definition

[Zhang Chen]

Add L4_Connection struct for other QMP commands.
Except protocol field is necessary, other fields are optional.

Signed-off-by: Zhang Chen <chen.zhang@intel.com>
---
 qapi/net.json | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/qapi/net.json b/qapi/net.json
index 498ea7aa72..cd4a8ed95e 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -825,3 +825,29 @@
 { 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite',
     'icmp', 'igmp', 'ipv6' ] }
 
+##
+# @L4_Connection:
+#
+# Layer 4 network connection.
+#
+# Just for IPv4.
+#
+# @protocol: Transport layer protocol like TCP/UDP...
+#
+# @id: For specific module with Qemu object ID, If there is no such part,
+#      it means global rules.
+#
+# @src_ip: Source IP.
+#
+# @dst_ip: Destination IP.
+#
+# @src_port: Source port.
+#
+# @dst_port: Destination port.
+#
+# Since: 6.1
+##
+{ 'struct': 'L4_Connection',
+  'data': { 'protocol': 'IP_PROTOCOL', '*id': 'str', '*src_ip': 'str', '*dst_ip': 'str',
+    '*src_port': 'int', '*dst_port': 'int' } }
+
-- 
2.25.1

[PATCH V4 3/7] qapi/net: Add new QMP command for COLO passthrough

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

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

diff --git a/net/net.c b/net/net.c
index 725a4e1450..7c7cefe0e0 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1199,6 +1199,16 @@ void qmp_netdev_del(const char *id, Error **errp)
     }
 }
 
+void qmp_colo_passthrough_add(L4_Connection *conn, Error **errp)
+{
+    /* Setup passthrough connection */
+}
+
+void qmp_colo_passthrough_del(L4_Connection *conn, Error **errp)
+{
+    /* Delete passthrough connection */
+}
+
 static void netfilter_print_info(Monitor *mon, NetFilterState *nf)
 {
     char *str;
diff --git a/qapi/net.json b/qapi/net.json
index cd4a8ed95e..ec7d3b1128 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -851,3 +851,43 @@
   'data': { 'protocol': 'IP_PROTOCOL', '*id': 'str', '*src_ip': 'str', '*dst_ip': 'str',
     '*src_port': 'int', '*dst_port': 'int' } }
 
+##
+# @colo-passthrough-add:
+#
+# Add passthrough entry according to customer's needs in COLO-compare.
+#
+# Returns: Nothing on success
+#
+# Since: 6.1
+#
+# Example:
+#
+# -> { "execute": "colo-passthrough-add",
+#      "arguments": { "protocol": "tcp", "id": "object0", "src_ip": "192.168.1.1",
+#      "dst_ip": "192.168.1.2", "src_port": 1234, "dst_port": 4321 } }
+# <- { "return": {} }
+#
+##
+{ 'command': 'colo-passthrough-add', 'boxed': true,
+     'data': 'L4_Connection' }
+
+##
+# @colo-passthrough-del:
+#
+# Delete passthrough entry according to customer's needs in COLO-compare.
+#
+# Returns: Nothing on success
+#
+# Since: 6.1
+#
+# Example:
+#
+# -> { "execute": "colo-passthrough-del",
+#      "arguments": { "protocol": "tcp", "id": "object0", "src_ip": "192.168.1.1",
+#      "dst_ip": "192.168.1.2", "src_port": 1234, "dst_port": 4321 } }
+# <- { "return": {} }
+#
+##
+{ 'command': 'colo-passthrough-del', 'boxed': true,
+     'data': 'L4_Connection' }
+
-- 
2.25.1

[PATCH V4 4/7] hmp-commands: Add new HMP command for COLO passthrough

[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    | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 62 insertions(+)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index d4001f9c5d..b67a5a04cb 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1335,6 +1335,32 @@ SRST
   Remove host network device.
 ERST
 
+    {
+        .name       = "colo_passthrough_add",
+        .args_type  = "protocol:s,id:s?,src_ip:s?,dst_ip:s?,src_port:i?,dst_port:i?",
+        .params     = "protocol [id] [src_ip] [dst_ip] [src_port] [dst_port]",
+        .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,id:s?,src_ip:s?,dst_ip:s?,src_port:i?,dst_port:i?",
+        .params     = "protocol [id] [src_ip] [dst_ip] [src_port] [dst_port]",
+        .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:O",
diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index ed2913fd18..3c4943b09f 100644
--- a/include/monitor/hmp.h
+++ b/include/monitor/hmp.h
@@ -81,6 +81,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 3c88a4faef..b57e3430ab 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1668,6 +1668,40 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, err);
 }
 
+void hmp_colo_passthrough_add(Monitor *mon, const QDict *qdict)
+{
+    const char *prot = qdict_get_str(qdict, "protocol");
+    L4_Connection *l4_conn = g_new0(L4_Connection, 1);
+    Error *err = NULL;
+
+    l4_conn->id = g_strdup(qdict_get_try_str(qdict, "id"));
+    l4_conn->protocol = qapi_enum_parse(&IP_PROTOCOL_lookup, prot, -1, &err);
+    l4_conn->src_ip = g_strdup(qdict_get_try_str(qdict, "src_ip"));
+    l4_conn->dst_ip = g_strdup(qdict_get_try_str(qdict, "dst_ip"));
+    l4_conn->src_port = qdict_get_try_int(qdict, "src_port", 0);
+    l4_conn->dst_port = qdict_get_try_int(qdict, "dst_port", 0);
+
+    qmp_colo_passthrough_add(l4_conn, &err);
+    hmp_handle_error(mon, err);
+}
+
+void hmp_colo_passthrough_del(Monitor *mon, const QDict *qdict)
+{
+    const char *prot = qdict_get_str(qdict, "protocol");
+    L4_Connection *l4_conn = g_new0(L4_Connection, 1);
+    Error *err = NULL;
+
+    l4_conn->id = g_strdup(qdict_get_try_str(qdict, "id"));
+    l4_conn->protocol = qapi_enum_parse(&IP_PROTOCOL_lookup, prot, -1, &err);
+    l4_conn->src_ip = g_strdup(qdict_get_try_str(qdict, "src_ip"));
+    l4_conn->dst_ip = g_strdup(qdict_get_try_str(qdict, "dst_ip"));
+    l4_conn->src_port = qdict_get_try_int(qdict, "src_port", 0);
+    l4_conn->dst_port = qdict_get_try_int(qdict, "dst_port", 0);
+
+    qmp_colo_passthrough_del(l4_conn, &err);
+    hmp_handle_error(mon, err);
+}
+
 void hmp_object_add(Monitor *mon, const QDict *qdict)
 {
     Error *err = NULL;
-- 
2.25.1

[PATCH V4 5/7] net/colo-compare: Move data structure and define to .h file.

[Zhang Chen]

Make other modules can reuse COLO code.

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

diff --git a/net/colo-compare.c b/net/colo-compare.c
index 84db4978ac..a803f8b888 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -17,44 +17,24 @@
 #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);
 
 static NotifierList colo_compare_notifiers =
     NOTIFIER_LIST_INITIALIZER(colo_compare_notifiers);
 
-#define COMPARE_READ_LEN_MAX NET_BUFSIZE
-#define MAX_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 DEBUG_COLO_PACKETS */
 
 static QemuMutex colo_compare_mutex;
@@ -64,92 +44,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,
-};
-
 static const char *colo_mode[] = {
     [PRIMARY_IN] = "primary",
     [SECONDARY_IN] = "secondary",
diff --git a/net/colo-compare.h b/net/colo-compare.h
index 22ddd512e2..2a9dcac0a7 100644
--- a/net/colo-compare.h
+++ b/net/colo-compare.h
@@ -17,6 +17,112 @@
 #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)
+
+#define COMPARE_READ_LEN_MAX NET_BUFSIZE
+#define MAX_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
+
+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;
+
+/*
+ *  + 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;
+    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,
+};
+
 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

[PATCH V4 6/7] net/colo-compare: Add passthrough list to CompareState

[Zhang Chen]

Add passthrough list for each CompareState.

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

diff --git a/net/colo-compare.c b/net/colo-compare.c
index a803f8b888..40af8cd501 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -141,6 +141,7 @@ static int packet_enqueue(CompareState *s, int mode, Connection **con)
     ConnectionKey key;
     Packet *pkt = NULL;
     Connection *conn;
+    PassthroughEntry *bypass, *next;
     int ret;
 
     if (mode == PRIMARY_IN) {
@@ -160,6 +161,32 @@ static int packet_enqueue(CompareState *s, int mode, Connection **con)
     }
     fill_connection_key(pkt, &key);
 
+    /* Check COLO passthrough connenction */
+    qemu_mutex_lock(&s->passthroughlist_mutex);
+    if (!QLIST_EMPTY(&s->passthroughlist)) {
+        QLIST_FOREACH_SAFE(bypass, &s->passthroughlist, node, next) {
+            if (((key.ip_proto == IPPROTO_TCP) && (bypass->l4_protocol == 0)) ||
+                ((key.ip_proto == IPPROTO_UDP) && (bypass->l4_protocol == 1))) {
+                if (bypass->src_port == 0 || bypass->src_port == key.dst_port) {
+                    if (bypass->src_ip.s_addr == 0 ||
+                        bypass->src_ip.s_addr == key.src.s_addr) {
+                        if (bypass->dst_port == 0 ||
+                            bypass->dst_port == key.src_port) {
+                            if (bypass->dst_ip.s_addr == 0 ||
+                                bypass->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);
@@ -1224,6 +1251,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,
@@ -1236,6 +1264,7 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
     if (!colo_compare_active) {
         qemu_mutex_init(&event_mtx);
         qemu_cond_init(&event_complete_cond);
+        qemu_mutex_init(&s->passthroughlist_mutex);
         colo_compare_active = true;
     }
     QTAILQ_INSERT_TAIL(&net_compares, s, next);
diff --git a/net/colo-compare.h b/net/colo-compare.h
index 2a9dcac0a7..2259abcd63 100644
--- a/net/colo-compare.h
+++ b/net/colo-compare.h
@@ -54,6 +54,15 @@ typedef struct SendEntry {
     uint8_t *buf;
 } SendEntry;
 
+typedef struct PassthroughEntry {
+    int l4_protocol;
+    int src_port;
+    int dst_port;
+    struct in_addr src_ip;
+    struct in_addr dst_ip;
+    QLIST_ENTRY(PassthroughEntry) node;
+} PassthroughEntry;
+
 /*
  *  + CompareState ++
  *  |               |
@@ -110,6 +119,8 @@ struct CompareState {
 
     QEMUBH *event_bh;
     enum colo_event event;
+    QLIST_HEAD(, PassthroughEntry) passthroughlist;
+    QemuMutex passthroughlist_mutex;
 
     QTAILQ_ENTRY(CompareState) next;
 };
-- 
2.25.1

[PATCH V4 7/7] net/net.c: Add handler for COLO passthrough connection

[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 | 153 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 153 insertions(+)

diff --git a/net/net.c b/net/net.c
index 7c7cefe0e0..4a9ab29cca 100644
--- a/net/net.c
+++ b/net/net.c
@@ -56,6 +56,8 @@
 #include "net/filter.h"
 #include "qapi/string-output-visitor.h"
 #include "qapi/hmp-output-visitor.h"
+#include "net/colo-compare.h"
+#include "qom/object_interfaces.h"
 
 /* Net bridge is currently not supported for W32. */
 #if !defined(_WIN32)
@@ -1199,14 +1201,165 @@ void qmp_netdev_del(const char *id, Error **errp)
     }
 }
 
+static CompareState *colo_passthrough_check(L4_Connection *conn, Error **errp)
+{
+    Object *container;
+    Object *obj;
+    CompareState *s;
+
+    if (!conn->id) {
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "id",
+                   "Need input colo-compare object id");
+        return NULL;
+    }
+
+    container = object_get_objects_root();
+    obj = object_resolve_path_component(container, conn->id);
+    if (!obj) {
+        error_setg(errp, "colo-compare '%s' not found", conn->id);
+        return NULL;
+    }
+
+    s = COLO_COMPARE(obj);
+
+    if (conn->protocol == -1) {
+        error_setg(errp, "COLO pass through get wrong protocol");
+        return NULL;
+    }
+
+    if ((conn->src_ip && !qemu_isdigit(conn->src_ip[0])) ||
+        (conn->dst_ip && !qemu_isdigit(conn->dst_ip[0]))) {
+        error_setg(errp, "COLO pass through get wrong IP");
+        return NULL;
+    }
+
+    if (conn->src_port > 65536 || conn->src_port < 0 ||
+        conn->dst_port > 65536 || conn->dst_port < 0) {
+        error_setg(errp, "COLO pass through get wrong port");
+        return NULL;
+    }
+
+    return s;
+}
+
+static void compare_passthrough_add(CompareState *s,
+                                    L4_Connection *conn,
+                                    Error **errp)
+{
+    PassthroughEntry *bypass = NULL, *next = NULL, *origin = NULL;
+
+    bypass = g_new0(PassthroughEntry, 1);
+
+    bypass->l4_protocol = conn->protocol;
+    bypass->src_port = conn->src_port;
+    bypass->dst_port = conn->dst_port;
+
+    if (!inet_aton(conn->src_ip, &bypass->src_ip)) {
+        bypass->src_ip.s_addr = 0;
+    }
+
+    if (!inet_aton(conn->dst_ip, &bypass->dst_ip)) {
+        bypass->dst_ip.s_addr = 0;
+    }
+
+    qemu_mutex_lock(&s->passthroughlist_mutex);
+    if (!QLIST_EMPTY(&s->passthroughlist)) {
+        QLIST_FOREACH_SAFE(origin, &s->passthroughlist, node, next) {
+            if ((bypass->l4_protocol == origin->l4_protocol) &&
+                (bypass->src_port == origin->src_port) &&
+                (bypass->src_ip.s_addr == origin->src_ip.s_addr) &&
+                (bypass->dst_ip.s_addr == origin->dst_ip.s_addr)) {
+                error_setg(errp, "The pass through connection already exists");
+                g_free(bypass);
+                qemu_mutex_unlock(&s->passthroughlist_mutex);
+                return;
+            }
+        }
+    }
+
+    QLIST_INSERT_HEAD(&s->passthroughlist, bypass, node);
+    qemu_mutex_unlock(&s->passthroughlist_mutex);
+}
+
+static void compare_passthrough_del(CompareState *s,
+                                    L4_Connection *conn,
+                                    Error **errp)
+{
+    PassthroughEntry *bypass = NULL, *next = NULL, *origin = NULL;
+
+    bypass = g_new0(PassthroughEntry, 1);
+
+    bypass->l4_protocol = conn->protocol;
+    bypass->src_port = conn->src_port;
+    bypass->dst_port = conn->dst_port;
+
+    if (!inet_aton(conn->src_ip, &bypass->src_ip)) {
+        bypass->src_ip.s_addr = 0;
+    }
+
+    if (!inet_aton(conn->dst_ip, &bypass->dst_ip)) {
+        bypass->dst_ip.s_addr = 0;
+    }
+
+    qemu_mutex_lock(&s->passthroughlist_mutex);
+    if (!QLIST_EMPTY(&s->passthroughlist)) {
+        QLIST_FOREACH_SAFE(origin, &s->passthroughlist, node, next) {
+            if ((bypass->l4_protocol == origin->l4_protocol) &&
+                (bypass->src_port == origin->src_port) &&
+                (bypass->src_ip.s_addr == origin->src_ip.s_addr) &&
+                (bypass->dst_ip.s_addr == origin->dst_ip.s_addr)) {
+                QLIST_REMOVE(origin, node);
+                g_free(origin);
+                g_free(bypass);
+                qemu_mutex_unlock(&s->passthroughlist_mutex);
+                return;
+            }
+        }
+        error_setg(errp, "The pass through list can't find the connection");
+    } else {
+        error_setg(errp, "The pass through connection list is empty");
+    }
+
+    g_free(bypass);
+    qemu_mutex_unlock(&s->passthroughlist_mutex);
+}
+
 void qmp_colo_passthrough_add(L4_Connection *conn, Error **errp)
 {
     /* Setup passthrough connection */
+    CompareState *s;
+    Error *err = NULL;
+
+    s = colo_passthrough_check(conn, &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+
+    compare_passthrough_add(s, conn, &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
 }
 
 void qmp_colo_passthrough_del(L4_Connection *conn, Error **errp)
 {
     /* Delete passthrough connection */
+    CompareState *s;
+    Error *err = NULL;
+
+    s = colo_passthrough_check(conn, &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+
+    compare_passthrough_del(s, conn, &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
 }
 
 static void netfilter_print_info(Monitor *mon, NetFilterState *nf)
-- 
2.25.1

Re: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition

[Markus Armbruster]

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

> Add IP_PROTOCOL as enum include TCP,UDP, ICMP... for other QMP commands.
>
> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> ---
>  qapi/net.json | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>
> diff --git a/qapi/net.json b/qapi/net.json
> index 87361ebd9a..498ea7aa72 100644
> --- a/qapi/net.json
> +++ b/qapi/net.json
> @@ -794,3 +794,34 @@
>  #
>  ##
>  { 'command': 'query-netdev', 'returns': ['NetdevInfo'] }
> +
> +##
> +# @IP_PROTOCOL:
> +#
> +# Transport layer protocol.
> +#
> +# Just for IPv4.

Really?

> +#
> +# @tcp: Transmission Control Protocol.
> +#
> +# @udp: User Datagram Protocol.
> +#
> +# @dccp: Datagram Congestion Control Protocol.
> +#
> +# @sctp: Stream Control Transmission Protocol.
> +#
> +# @udplite: Lightweight User Datagram Protocol.
> +#
> +# @icmp: Internet Control Message Protocol.
> +#
> +# @igmp: Internet Group Management Protocol.
> +#
> +# @ipv6: IPv6 Encapsulation.
> +#
> +# TODO: Need to add more transport layer protocol.
> +#
> +# Since: 6.1
> +##
> +{ 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite',
> +    'icmp', 'igmp', 'ipv6' ] }
> +

docs/devel/qapi-code-gen.txt: "type definitions should always use
CamelCase".

Make this something like 'enum': 'IpProtocol', please.

Re: [PATCH V4 2/7] qapi/net.json: Add L4_Connection definition

[Markus Armbruster]

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

> Add L4_Connection struct for other QMP commands.
> Except protocol field is necessary, other fields are optional.
>
> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> ---
>  qapi/net.json | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> diff --git a/qapi/net.json b/qapi/net.json
> index 498ea7aa72..cd4a8ed95e 100644
> --- a/qapi/net.json
> +++ b/qapi/net.json
> @@ -825,3 +825,29 @@
>  { 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite',
>      'icmp', 'igmp', 'ipv6' ] }
>  
> +##
> +# @L4_Connection:
> +#
> +# Layer 4 network connection.
> +#
> +# Just for IPv4.
> +#
> +# @protocol: Transport layer protocol like TCP/UDP...
> +#
> +# @id: For specific module with Qemu object ID, If there is no such part,
> +#      it means global rules.

Clear as mud.

See my review of PATCH 3.

> +#
> +# @src_ip: Source IP.
> +#
> +# @dst_ip: Destination IP.
> +#
> +# @src_port: Source port.
> +#
> +# @dst_port: Destination port.
> +#
> +# Since: 6.1
> +##
> +{ 'struct': 'L4_Connection',
> +  'data': { 'protocol': 'IP_PROTOCOL', '*id': 'str', '*src_ip': 'str', '*dst_ip': 'str',
> +    '*src_port': 'int', '*dst_port': 'int' } }
> +

Re: [PATCH V4 2/7] qapi/net.json: Add L4_Connection definition

[Markus Armbruster]

One more little thing...

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

> Add L4_Connection struct for other QMP commands.
> Except protocol field is necessary, other fields are optional.
>
> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> ---
>  qapi/net.json | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> diff --git a/qapi/net.json b/qapi/net.json
> index 498ea7aa72..cd4a8ed95e 100644
> --- a/qapi/net.json
> +++ b/qapi/net.json
> @@ -825,3 +825,29 @@
>  { 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite',
>      'icmp', 'igmp', 'ipv6' ] }
>  
> +##
> +# @L4_Connection:
> +#
> +# Layer 4 network connection.
> +#
> +# Just for IPv4.
> +#
> +# @protocol: Transport layer protocol like TCP/UDP...
> +#
> +# @id: For specific module with Qemu object ID, If there is no such part,
> +#      it means global rules.
> +#
> +# @src_ip: Source IP.
> +#
> +# @dst_ip: Destination IP.
> +#
> +# @src_port: Source port.
> +#
> +# @dst_port: Destination port.
> +#
> +# Since: 6.1
> +##
> +{ 'struct': 'L4_Connection',
> +  'data': { 'protocol': 'IP_PROTOCOL', '*id': 'str', '*src_ip': 'str', '*dst_ip': 'str',
> +    '*src_port': 'int', '*dst_port': 'int' } }
> +

Please avoid the long line, e.g. like this:

   { 'struct': 'L4_Connection',
     'data': {
       'protocol': 'IP_PROTOCOL', '*id': 'str', '*src_ip': 'str',
       '*dst_ip': 'str', '*src_port': 'int', '*dst_port': 'int' } }

Re: [PATCH V4 3/7] qapi/net: Add new QMP command for COLO passthrough

[Markus Armbruster]

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.
>
> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> ---
>  net/net.c     | 10 ++++++++++
>  qapi/net.json | 40 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+)
>
> diff --git a/net/net.c b/net/net.c
> index 725a4e1450..7c7cefe0e0 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1199,6 +1199,16 @@ void qmp_netdev_del(const char *id, Error **errp)
>      }
>  }
>  
> +void qmp_colo_passthrough_add(L4_Connection *conn, Error **errp)
> +{
> +    /* Setup passthrough connection */

Do you mean to say

       /* TODO implement */

?

> +}
> +
> +void qmp_colo_passthrough_del(L4_Connection *conn, Error **errp)
> +{
> +    /* Delete passthrough connection */
> +}

Likewise.

> +
>  static void netfilter_print_info(Monitor *mon, NetFilterState *nf)
>  {
>      char *str;
> diff --git a/qapi/net.json b/qapi/net.json
> index cd4a8ed95e..ec7d3b1128 100644
> --- a/qapi/net.json
> +++ b/qapi/net.json
> @@ -851,3 +851,43 @@
>    'data': { 'protocol': 'IP_PROTOCOL', '*id': 'str', '*src_ip': 'str', '*dst_ip': 'str',
>      '*src_port': 'int', '*dst_port': 'int' } }
>  
> +##
> +# @colo-passthrough-add:
> +#
> +# Add passthrough entry according to customer's needs in COLO-compare.

QEMU doesn't have customers, it has users :)

> +#
> +# Returns: Nothing on success
> +#
> +# Since: 6.1
> +#
> +# Example:
> +#
> +# -> { "execute": "colo-passthrough-add",
> +#      "arguments": { "protocol": "tcp", "id": "object0", "src_ip": "192.168.1.1",
> +#      "dst_ip": "192.168.1.2", "src_port": 1234, "dst_port": 4321 } }
> +# <- { "return": {} }
> +#
> +##
> +{ 'command': 'colo-passthrough-add', 'boxed': true,
> +     'data': 'L4_Connection' }
> +
> +##
> +# @colo-passthrough-del:
> +#
> +# Delete passthrough entry according to customer's needs in COLO-compare.
> +#
> +# Returns: Nothing on success
> +#
> +# Since: 6.1
> +#
> +# Example:
> +#
> +# -> { "execute": "colo-passthrough-del",
> +#      "arguments": { "protocol": "tcp", "id": "object0", "src_ip": "192.168.1.1",
> +#      "dst_ip": "192.168.1.2", "src_port": 1234, "dst_port": 4321 } }
> +# <- { "return": {} }
> +#
> +##
> +{ 'command': 'colo-passthrough-del', 'boxed': true,
> +     'data': 'L4_Connection' }
> +

To make sense of this, I have to refer back to PATCH 1 and 2:

   { 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite',
       'icmp', 'igmp', 'ipv6' ] }

   { 'struct': 'L4_Connection',
     'data': { 'protocol': 'IP_PROTOCOL', '*id': 'str', '*src_ip': 'str', '*dst_ip': 'str',
       '*src_port': 'int', '*dst_port': 'int' } }

Please squash the three patches together.

I figure colo-passthrough-add adds some kind of packet matching thingy
that can match packets by source IP, source port, destination IP,
destination port, and protocol.  Correct?

The protocol is mandatory, all others are optional.  What does it mean
to omit an optional one?  Match all?

I have no idea what @id is supposed to mean.  Please explain intended
use.

I'm ignoring colo-passthrough-del for now, because I feel need to
understand -add first.

RE: [PATCH V4 3/7] qapi/net: Add new QMP command for COLO passthrough

[Zhang, Chen]

> -----Original Message-----
> From: Markus Armbruster <armbru@redhat.com>
> Sent: Saturday, March 20, 2021 12:03 AM
> 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>; Li Zhijian <lizhijian@cn.fujitsu.com>; Lukas
> Straub <lukasstraub2@web.de>; Zhang Chen <zhangckid@gmail.com>
> Subject: Re: [PATCH V4 3/7] qapi/net: Add new 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.
> >
> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> > ---
> >  net/net.c     | 10 ++++++++++
> >  qapi/net.json | 40 ++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 50 insertions(+)
> >
> > diff --git a/net/net.c b/net/net.c
> > index 725a4e1450..7c7cefe0e0 100644
> > --- a/net/net.c
> > +++ b/net/net.c
> > @@ -1199,6 +1199,16 @@ void qmp_netdev_del(const char *id, Error
> **errp)
> >      }
> >  }
> >
> > +void qmp_colo_passthrough_add(L4_Connection *conn, Error **errp) {
> > +    /* Setup passthrough connection */
> 
> Do you mean to say
> 
>        /* TODO implement */
> 
> ?

Yes, I will input real code here in 7/7 patch.

> 
> > +}
> > +
> > +void qmp_colo_passthrough_del(L4_Connection *conn, Error **errp) {
> > +    /* Delete passthrough connection */ }
> 
> Likewise.
> 
> > +
> >  static void netfilter_print_info(Monitor *mon, NetFilterState *nf)  {
> >      char *str;
> > diff --git a/qapi/net.json b/qapi/net.json index
> > cd4a8ed95e..ec7d3b1128 100644
> > --- a/qapi/net.json
> > +++ b/qapi/net.json
> > @@ -851,3 +851,43 @@
> >    'data': { 'protocol': 'IP_PROTOCOL', '*id': 'str', '*src_ip': 'str', '*dst_ip': 'str',
> >      '*src_port': 'int', '*dst_port': 'int' } }
> >
> > +##
> > +# @colo-passthrough-add:
> > +#
> > +# Add passthrough entry according to customer's needs in COLO-compare.
> 
> QEMU doesn't have customers, it has users :)

Thanks note.

> 
> > +#
> > +# Returns: Nothing on success
> > +#
> > +# Since: 6.1
> > +#
> > +# Example:
> > +#
> > +# -> { "execute": "colo-passthrough-add",
> > +#      "arguments": { "protocol": "tcp", "id": "object0", "src_ip":
> "192.168.1.1",
> > +#      "dst_ip": "192.168.1.2", "src_port": 1234, "dst_port": 4321 } }
> > +# <- { "return": {} }
> > +#
> > +##
> > +{ 'command': 'colo-passthrough-add', 'boxed': true,
> > +     'data': 'L4_Connection' }
> > +
> > +##
> > +# @colo-passthrough-del:
> > +#
> > +# Delete passthrough entry according to customer's needs in COLO-
> compare.
> > +#
> > +# Returns: Nothing on success
> > +#
> > +# Since: 6.1
> > +#
> > +# Example:
> > +#
> > +# -> { "execute": "colo-passthrough-del",
> > +#      "arguments": { "protocol": "tcp", "id": "object0", "src_ip":
> "192.168.1.1",
> > +#      "dst_ip": "192.168.1.2", "src_port": 1234, "dst_port": 4321 } }
> > +# <- { "return": {} }
> > +#
> > +##
> > +{ 'command': 'colo-passthrough-del', 'boxed': true,
> > +     'data': 'L4_Connection' }
> > +
> 
> To make sense of this, I have to refer back to PATCH 1 and 2:
> 
>    { 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite',
>        'icmp', 'igmp', 'ipv6' ] }
> 
>    { 'struct': 'L4_Connection',
>      'data': { 'protocol': 'IP_PROTOCOL', '*id': 'str', '*src_ip': 'str', '*dst_ip': 'str',
>        '*src_port': 'int', '*dst_port': 'int' } }
> 
> Please squash the three patches together.

OK.

> 
> I figure colo-passthrough-add adds some kind of packet matching thingy that
> can match packets by source IP, source port, destination IP, destination port,
> and protocol.  Correct?

Yes, you are right.

> 
> The protocol is mandatory, all others are optional.  What does it mean to omit
> an optional one?  Match all?

Yes, match all. The idea from Jason Wang, for example:
User just set the protocol/source IP(tcp/192.168.1.1) , others empty.
The rule will bypass all the TCP packet from the source IP.

> 
> I have no idea what @id is supposed to mean.  Please explain intended use.

The @id means packet hander in Qemu. Because not all the guest network packet into the colo-compare module, the net-filters are same cases.
There modules attach to NIC or chardev socket to work, VM maybe have multi modules running. So we use the ID to set the rule to the specific module. 

Thanks
Chen

> 
> I'm ignoring colo-passthrough-del for now, because I feel need to
> understand -add first.

RE: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition

[Zhang, Chen]

> -----Original Message-----
> From: Markus Armbruster <armbru@redhat.com>
> Sent: Friday, March 19, 2021 11:47 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>; Li Zhijian <lizhijian@cn.fujitsu.com>; Lukas
> Straub <lukasstraub2@web.de>; Zhang Chen <zhangckid@gmail.com>
> Subject: Re: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition
> 
> Zhang Chen <chen.zhang@intel.com> writes:
> 
> > Add IP_PROTOCOL as enum include TCP,UDP, ICMP... for other QMP
> commands.
> >
> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> > ---
> >  qapi/net.json | 31 +++++++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >
> > diff --git a/qapi/net.json b/qapi/net.json index
> > 87361ebd9a..498ea7aa72 100644
> > --- a/qapi/net.json
> > +++ b/qapi/net.json
> > @@ -794,3 +794,34 @@
> >  #
> >  ##
> >  { 'command': 'query-netdev', 'returns': ['NetdevInfo'] }
> > +
> > +##
> > +# @IP_PROTOCOL:
> > +#
> > +# Transport layer protocol.
> > +#
> > +# Just for IPv4.
> 
> Really?

Current tcp/udp/icmp field from IPv4 header definition,
I think maybe we need add more to support IPv6.
So, looks change to #TODO support IPv6 part is better?

> 
> > +#
> > +# @tcp: Transmission Control Protocol.
> > +#
> > +# @udp: User Datagram Protocol.
> > +#
> > +# @dccp: Datagram Congestion Control Protocol.
> > +#
> > +# @sctp: Stream Control Transmission Protocol.
> > +#
> > +# @udplite: Lightweight User Datagram Protocol.
> > +#
> > +# @icmp: Internet Control Message Protocol.
> > +#
> > +# @igmp: Internet Group Management Protocol.
> > +#
> > +# @ipv6: IPv6 Encapsulation.
> > +#
> > +# TODO: Need to add more transport layer protocol.
> > +#
> > +# Since: 6.1
> > +##
> > +{ 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite',
> > +    'icmp', 'igmp', 'ipv6' ] }
> > +
> 
> docs/devel/qapi-code-gen.txt: "type definitions should always use
> CamelCase".
> 
> Make this something like 'enum': 'IpProtocol', please.

OK, I will fix it in next version.

Thanks
Chen

RE: [PATCH V4 2/7] qapi/net.json: Add L4_Connection definition

[Zhang, Chen]

> -----Original Message-----
> From: Markus Armbruster <armbru@redhat.com>
> Sent: Friday, March 19, 2021 11:48 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>;
> Li Zhijian <lizhijian@cn.fujitsu.com>; Lukas Straub <lukasstraub2@web.de>;
> Zhang Chen <zhangckid@gmail.com>
> Subject: Re: [PATCH V4 2/7] qapi/net.json: Add L4_Connection definition
> 
> Zhang Chen <chen.zhang@intel.com> writes:
> 
> > Add L4_Connection struct for other QMP commands.
> > Except protocol field is necessary, other fields are optional.
> >
> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> > ---
> >  qapi/net.json | 26 ++++++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> >
> > diff --git a/qapi/net.json b/qapi/net.json index
> > 498ea7aa72..cd4a8ed95e 100644
> > --- a/qapi/net.json
> > +++ b/qapi/net.json
> > @@ -825,3 +825,29 @@
> >  { 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite',
> >      'icmp', 'igmp', 'ipv6' ] }
> >
> > +##
> > +# @L4_Connection:
> > +#
> > +# Layer 4 network connection.
> > +#
> > +# Just for IPv4.
> > +#
> > +# @protocol: Transport layer protocol like TCP/UDP...
> > +#
> > +# @id: For specific module with Qemu object ID, If there is no such part,
> > +#      it means global rules.
> 
> Clear as mud.

Sorry, let me re-clear it.
If I understand correctly, The ID shouldn't be here, but I found the 'boxed' flag just can add only one 'data' like this:
+##
+{ 'command': 'colo-passthrough-add', 'boxed': true,
+     'data': 'L4_Connection' }

I original want to this:
+##
+{ 'command': 'colo-passthrough-add', 
+     'data': { 'id': 'str', 'boxed': false, 'conn': 'L4_Connection', 'boxed': true  }

So, I add the @id as an optional argument here.

rewrite the comments:
+# @id: Assign the rule to Qemu network handle module object ID. Like colo-compare, net-filter. 

Please see the ID details in patch3 too. 

Thanks
Chen

> 
> See my review of PATCH 3.
> 
> > +#
> > +# @src_ip: Source IP.
> > +#
> > +# @dst_ip: Destination IP.
> > +#
> > +# @src_port: Source port.
> > +#
> > +# @dst_port: Destination port.
> > +#
> > +# Since: 6.1
> > +##
> > +{ 'struct': 'L4_Connection',
> > +  'data': { 'protocol': 'IP_PROTOCOL', '*id': 'str', '*src_ip': 'str', '*dst_ip': 'str',
> > +    '*src_port': 'int', '*dst_port': 'int' } }
> > +

Re: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition

[Markus Armbruster]

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

>> -----Original Message-----
>> From: Markus Armbruster <armbru@redhat.com>
>> Sent: Friday, March 19, 2021 11:47 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>; Li Zhijian <lizhijian@cn.fujitsu.com>; Lukas
>> Straub <lukasstraub2@web.de>; Zhang Chen <zhangckid@gmail.com>
>> Subject: Re: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition
>> 
>> Zhang Chen <chen.zhang@intel.com> writes:
>> 
>> > Add IP_PROTOCOL as enum include TCP,UDP, ICMP... for other QMP
>> commands.
>> >
>> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
>> > ---
>> >  qapi/net.json | 31 +++++++++++++++++++++++++++++++
>> >  1 file changed, 31 insertions(+)
>> >
>> > diff --git a/qapi/net.json b/qapi/net.json index
>> > 87361ebd9a..498ea7aa72 100644
>> > --- a/qapi/net.json
>> > +++ b/qapi/net.json
>> > @@ -794,3 +794,34 @@
>> >  #
>> >  ##
>> >  { 'command': 'query-netdev', 'returns': ['NetdevInfo'] }
>> > +
>> > +##
>> > +# @IP_PROTOCOL:
>> > +#
>> > +# Transport layer protocol.
>> > +#
>> > +# Just for IPv4.
>> 
>> Really?
>
> Current tcp/udp/icmp field from IPv4 header definition,
> I think maybe we need add more to support IPv6.
> So, looks change to #TODO support IPv6 part is better?

IPv4 and IPv6 share internet protocol numbers.  IPv4 has it in header
field "protocol", IPv6 in "next header".

Canonical registry:
https://www.iana.org/assignments/protocol-numbers/protocol-numbers.xhtml

>> > +#
>> > +# @tcp: Transmission Control Protocol.
>> > +#
>> > +# @udp: User Datagram Protocol.
>> > +#
>> > +# @dccp: Datagram Congestion Control Protocol.
>> > +#
>> > +# @sctp: Stream Control Transmission Protocol.
>> > +#
>> > +# @udplite: Lightweight User Datagram Protocol.
>> > +#
>> > +# @icmp: Internet Control Message Protocol.
>> > +#
>> > +# @igmp: Internet Group Management Protocol.
>> > +#
>> > +# @ipv6: IPv6 Encapsulation.
>> > +#
>> > +# TODO: Need to add more transport layer protocol.

If there's a need *now*, we should add them now.  If the may be a need
in the future, then this isn't a TODO.  Perhaps

      # Additional protocols may be added as needed.

How did you pick the ones to add now?

What if a user wants to use a protocol number not in this enum?  If that
makes no sense (say because use requires code in QEMU), fine.  If it
does make sense, we need to talk.  You tell me :)

>> > +#
>> > +# Since: 6.1
>> > +##
>> > +{ 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite',
>> > +    'icmp', 'igmp', 'ipv6' ] }
>> > +
>> 
>> docs/devel/qapi-code-gen.txt: "type definitions should always use
>> CamelCase".
>> 
>> Make this something like 'enum': 'IpProtocol', please.
>
> OK, I will fix it in next version.
>
> Thanks
> Chen

Re: [PATCH V4 3/7] qapi/net: Add new QMP command for COLO passthrough

[Markus Armbruster]

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

>> -----Original Message-----
>> From: Markus Armbruster <armbru@redhat.com>
>> Sent: Saturday, March 20, 2021 12:03 AM
>> 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>; Li Zhijian <lizhijian@cn.fujitsu.com>; Lukas
>> Straub <lukasstraub2@web.de>; Zhang Chen <zhangckid@gmail.com>
>> Subject: Re: [PATCH V4 3/7] qapi/net: Add new 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.
>> >
>> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
>> > ---
>> >  net/net.c     | 10 ++++++++++
>> >  qapi/net.json | 40 ++++++++++++++++++++++++++++++++++++++++
>> >  2 files changed, 50 insertions(+)
>> >
>> > diff --git a/net/net.c b/net/net.c
>> > index 725a4e1450..7c7cefe0e0 100644
>> > --- a/net/net.c
>> > +++ b/net/net.c
>> > @@ -1199,6 +1199,16 @@ void qmp_netdev_del(const char *id, Error
>> **errp)
>> >      }
>> >  }
>> >
>> > +void qmp_colo_passthrough_add(L4_Connection *conn, Error **errp) {
>> > +    /* Setup passthrough connection */
>> 
>> Do you mean to say
>> 
>>        /* TODO implement */
>> 
>> ?
>
> Yes, I will input real code here in 7/7 patch.

Use a TODO comment then.

>> 
>> > +}
>> > +
>> > +void qmp_colo_passthrough_del(L4_Connection *conn, Error **errp) {
>> > +    /* Delete passthrough connection */ }
>> 
>> Likewise.
>> 
>> > +
>> >  static void netfilter_print_info(Monitor *mon, NetFilterState *nf)  {
>> >      char *str;
>> > diff --git a/qapi/net.json b/qapi/net.json index
>> > cd4a8ed95e..ec7d3b1128 100644
>> > --- a/qapi/net.json
>> > +++ b/qapi/net.json
>> > @@ -851,3 +851,43 @@
>> >    'data': { 'protocol': 'IP_PROTOCOL', '*id': 'str', '*src_ip': 'str', '*dst_ip': 'str',
>> >      '*src_port': 'int', '*dst_port': 'int' } }
>> >
>> > +##
>> > +# @colo-passthrough-add:
>> > +#
>> > +# Add passthrough entry according to customer's needs in COLO-compare.
>> 
>> QEMU doesn't have customers, it has users :)
>
> Thanks note.
>
>> 
>> > +#
>> > +# Returns: Nothing on success
>> > +#
>> > +# Since: 6.1
>> > +#
>> > +# Example:
>> > +#
>> > +# -> { "execute": "colo-passthrough-add",
>> > +#      "arguments": { "protocol": "tcp", "id": "object0", "src_ip": "192.168.1.1",
>> > +#      "dst_ip": "192.168.1.2", "src_port": 1234, "dst_port": 4321 } }
>> > +# <- { "return": {} }
>> > +#
>> > +##
>> > +{ 'command': 'colo-passthrough-add', 'boxed': true,
>> > +     'data': 'L4_Connection' }
>> > +
>> > +##
>> > +# @colo-passthrough-del:
>> > +#
>> > +# Delete passthrough entry according to customer's needs in COLO-compare.
>> > +#
>> > +# Returns: Nothing on success
>> > +#
>> > +# Since: 6.1
>> > +#
>> > +# Example:
>> > +#
>> > +# -> { "execute": "colo-passthrough-del",
>> > +#      "arguments": { "protocol": "tcp", "id": "object0", "src_ip": "192.168.1.1",
>> > +#      "dst_ip": "192.168.1.2", "src_port": 1234, "dst_port": 4321 } }
>> > +# <- { "return": {} }
>> > +#
>> > +##
>> > +{ 'command': 'colo-passthrough-del', 'boxed': true,
>> > +     'data': 'L4_Connection' }
>> > +
>> 
>> To make sense of this, I have to refer back to PATCH 1 and 2:
>> 
>>    { 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite',
>>        'icmp', 'igmp', 'ipv6' ] }
>> 
>>    { 'struct': 'L4_Connection',
>>      'data': { 'protocol': 'IP_PROTOCOL', '*id': 'str', '*src_ip': 'str', '*dst_ip': 'str',
>>        '*src_port': 'int', '*dst_port': 'int' } }
>> 
>> Please squash the three patches together.
>
> OK.
>
>> 
>> I figure colo-passthrough-add adds some kind of packet matching thingy that
>> can match packets by source IP, source port, destination IP, destination port,
>> and protocol.  Correct?
>
> Yes, you are right.
>
>> 
>> The protocol is mandatory, all others are optional.  What does it mean to omit
>> an optional one?  Match all?
>
> Yes, match all. The idea from Jason Wang, for example:
> User just set the protocol/source IP(tcp/192.168.1.1) , others empty.
> The rule will bypass all the TCP packet from the source IP.

Work this into the doc comment, please.

>> I have no idea what @id is supposed to mean.  Please explain intended use.
>
> The @id means packet hander in Qemu. Because not all the guest network packet into the colo-compare module, the net-filters are same cases.
> There modules attach to NIC or chardev socket to work, VM maybe have multi modules running. So we use the ID to set the rule to the specific module. 

I'm not sure I understand, but then I'm a QEMU networking ignoramus :)

Work it into the doc comment.

> Thanks
> Chen
>
>> 
>> I'm ignoring colo-passthrough-del for now, because I feel need to
>> understand -add first.

Re: [PATCH V4 2/7] qapi/net.json: Add L4_Connection definition

[Markus Armbruster]

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

>> -----Original Message-----
>> From: Markus Armbruster <armbru@redhat.com>
>> Sent: Friday, March 19, 2021 11:48 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>;
>> Li Zhijian <lizhijian@cn.fujitsu.com>; Lukas Straub <lukasstraub2@web.de>;
>> Zhang Chen <zhangckid@gmail.com>
>> Subject: Re: [PATCH V4 2/7] qapi/net.json: Add L4_Connection definition
>> 
>> Zhang Chen <chen.zhang@intel.com> writes:
>> 
>> > Add L4_Connection struct for other QMP commands.
>> > Except protocol field is necessary, other fields are optional.
>> >
>> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
>> > ---
>> >  qapi/net.json | 26 ++++++++++++++++++++++++++
>> >  1 file changed, 26 insertions(+)
>> >
>> > diff --git a/qapi/net.json b/qapi/net.json index
>> > 498ea7aa72..cd4a8ed95e 100644
>> > --- a/qapi/net.json
>> > +++ b/qapi/net.json
>> > @@ -825,3 +825,29 @@
>> >  { 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite',
>> >      'icmp', 'igmp', 'ipv6' ] }
>> >
>> > +##
>> > +# @L4_Connection:
>> > +#
>> > +# Layer 4 network connection.
>> > +#
>> > +# Just for IPv4.
>> > +#
>> > +# @protocol: Transport layer protocol like TCP/UDP...
>> > +#
>> > +# @id: For specific module with Qemu object ID, If there is no such part,
>> > +#      it means global rules.
>> 
>> Clear as mud.
>
> Sorry, let me re-clear it.
> If I understand correctly, The ID shouldn't be here, but I found the 'boxed' flag just can add only one 'data' like this:
> +##
> +{ 'command': 'colo-passthrough-add', 'boxed': true,
> +     'data': 'L4_Connection' }
>
> I original want to this:
> +##
> +{ 'command': 'colo-passthrough-add', 
> +     'data': { 'id': 'str', 'boxed': false, 'conn': 'L4_Connection', 'boxed': true  }
>
> So, I add the @id as an optional argument here.
>
> rewrite the comments:
> +# @id: Assign the rule to Qemu network handle module object ID. Like colo-compare, net-filter. 
>
> Please see the ID details in patch3 too. 

So, colo-passthrough-add takes an @id argument (to be tacked onto
packets to help with further processing, I understand), and arguments to
match packets.

Naming the argument type L4_Connection is misleading.

Even naming the match arguments L4_Connection would be misleading.
"Connection" has a specific meaning in networking.  There are TCP
connections.  There is no such thing as an UDP connection.

A TCP connection is uniquely identified by a pair of endpoints, i.e. by
source address, source port, destination address, destination port.
Same for other connection-oriented protocols.  The protocol is not part
of the connection.  Thus, L4_Connection would be misleading even for the
connection-oriented case.

You need a named type for colo-passthrough-add's argument because you
share it with colo-passthrough-del.  I'm not sure that's what we want
(I'm going to write more on that in a moment).  If it is what we want,
then please pick a another, descriptive name.

Re: [PATCH V4 3/7] qapi/net: Add new QMP command for COLO passthrough

[Markus Armbruster]

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.
>
> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> ---
>  net/net.c     | 10 ++++++++++
>  qapi/net.json | 40 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+)
>
> diff --git a/net/net.c b/net/net.c
> index 725a4e1450..7c7cefe0e0 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1199,6 +1199,16 @@ void qmp_netdev_del(const char *id, Error **errp)
>      }
>  }
>  
> +void qmp_colo_passthrough_add(L4_Connection *conn, Error **errp)
> +{
> +    /* Setup passthrough connection */
> +}
> +
> +void qmp_colo_passthrough_del(L4_Connection *conn, Error **errp)
> +{
> +    /* Delete passthrough connection */
> +}
> +
>  static void netfilter_print_info(Monitor *mon, NetFilterState *nf)
>  {
>      char *str;
> diff --git a/qapi/net.json b/qapi/net.json
> index cd4a8ed95e..ec7d3b1128 100644
> --- a/qapi/net.json
> +++ b/qapi/net.json
> @@ -851,3 +851,43 @@
>    'data': { 'protocol': 'IP_PROTOCOL', '*id': 'str', '*src_ip': 'str', '*dst_ip': 'str',
>      '*src_port': 'int', '*dst_port': 'int' } }
>  
> +##
> +# @colo-passthrough-add:
> +#
> +# Add passthrough entry according to customer's needs in COLO-compare.
> +#
> +# Returns: Nothing on success
> +#
> +# Since: 6.1
> +#
> +# Example:
> +#
> +# -> { "execute": "colo-passthrough-add",
> +#      "arguments": { "protocol": "tcp", "id": "object0", "src_ip": "192.168.1.1",
> +#      "dst_ip": "192.168.1.2", "src_port": 1234, "dst_port": 4321 } }
> +# <- { "return": {} }
> +#
> +##
> +{ 'command': 'colo-passthrough-add', 'boxed': true,
> +     'data': 'L4_Connection' }
> +
> +##
> +# @colo-passthrough-del:
> +#
> +# Delete passthrough entry according to customer's needs in COLO-compare.
> +#
> +# Returns: Nothing on success
> +#
> +# Since: 6.1
> +#
> +# Example:
> +#
> +# -> { "execute": "colo-passthrough-del",
> +#      "arguments": { "protocol": "tcp", "id": "object0", "src_ip": "192.168.1.1",
> +#      "dst_ip": "192.168.1.2", "src_port": 1234, "dst_port": 4321 } }
> +# <- { "return": {} }
> +#
> +##
> +{ 'command': 'colo-passthrough-del', 'boxed': true,
> +     'data': 'L4_Connection' }
> +

Now let's look at colo-passthrough-del.  I figure it is for deleting the
kind of things colo-passthrough-add adds.

What exactly is deleted?  The thing created with the exact same
arguments?

This would be unusual.  Commonly, FOO-add and FOO-del both take a string
ID argument.  The FOO created by FOO-add remembers its ID, and FOO-del
deletes by ID.

Re: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition

[Daniel P. Berrangé]

On Mon, Mar 22, 2021 at 09:59:54AM +0000, Zhang, Chen wrote:
> 
> 
> > -----Original Message-----
> > From: Markus Armbruster <armbru@redhat.com>
> > Sent: Friday, March 19, 2021 11:47 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>; Li Zhijian <lizhijian@cn.fujitsu.com>; Lukas
> > Straub <lukasstraub2@web.de>; Zhang Chen <zhangckid@gmail.com>
> > Subject: Re: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition
> > 
> > Zhang Chen <chen.zhang@intel.com> writes:
> > 
> > > Add IP_PROTOCOL as enum include TCP,UDP, ICMP... for other QMP
> > commands.
> > >
> > > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> > > ---
> > >  qapi/net.json | 31 +++++++++++++++++++++++++++++++
> > >  1 file changed, 31 insertions(+)
> > >
> > > diff --git a/qapi/net.json b/qapi/net.json index
> > > 87361ebd9a..498ea7aa72 100644
> > > --- a/qapi/net.json
> > > +++ b/qapi/net.json
> > > @@ -794,3 +794,34 @@
> > >  #
> > >  ##
> > >  { 'command': 'query-netdev', 'returns': ['NetdevInfo'] }
> > > +
> > > +##
> > > +# @IP_PROTOCOL:
> > > +#
> > > +# Transport layer protocol.
> > > +#
> > > +# Just for IPv4.
> > 
> > Really?
> 
> Current tcp/udp/icmp field from IPv4 header definition,
> I think maybe we need add more to support IPv6.
> So, looks change to #TODO support IPv6 part is better?

I'm inclined to say that this should implement IPv6 from the start.

No modern software should be implementing network functionality
for IPv4 only anymore IMHO.   Especially when there are potential
implications for the design of public APIs like QMP, we should
expect dual-stack support from the start.

> 
> > 
> > > +#
> > > +# @tcp: Transmission Control Protocol.
> > > +#
> > > +# @udp: User Datagram Protocol.
> > > +#
> > > +# @dccp: Datagram Congestion Control Protocol.
> > > +#
> > > +# @sctp: Stream Control Transmission Protocol.
> > > +#
> > > +# @udplite: Lightweight User Datagram Protocol.
> > > +#
> > > +# @icmp: Internet Control Message Protocol.
> > > +#
> > > +# @igmp: Internet Group Management Protocol.
> > > +#
> > > +# @ipv6: IPv6 Encapsulation.
> > > +#
> > > +# TODO: Need to add more transport layer protocol.
> > > +#
> > > +# Since: 6.1
> > > +##
> > > +{ 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite',
> > > +    'icmp', 'igmp', 'ipv6' ] }
> > > +
> > 
> > docs/devel/qapi-code-gen.txt: "type definitions should always use
> > CamelCase".
> > 
> > Make this something like 'enum': 'IpProtocol', please.
> 
> OK, I will fix it in next version.
> 
> Thanks
> Chen
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

RE: [PATCH V4 2/7] qapi/net.json: Add L4_Connection definition

[Zhang, Chen]

> -----Original Message-----
> From: Markus Armbruster <armbru@redhat.com>
> Sent: Monday, March 22, 2021 8:31 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: Lukas Straub <lukasstraub2@web.de>; Li Zhijian
> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; qemu-
> dev <qemu-devel@nongnu.org>; Dr. David Alan Gilbert
> <dgilbert@redhat.com>; Zhang Chen <zhangckid@gmail.com>
> Subject: Re: [PATCH V4 2/7] qapi/net.json: Add L4_Connection definition
> 
> "Zhang, Chen" <chen.zhang@intel.com> writes:
> 
> >> -----Original Message-----
> >> From: Markus Armbruster <armbru@redhat.com>
> >> Sent: Friday, March 19, 2021 11:48 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>;
> >> Li Zhijian <lizhijian@cn.fujitsu.com>; Lukas Straub
> >> <lukasstraub2@web.de>; Zhang Chen <zhangckid@gmail.com>
> >> Subject: Re: [PATCH V4 2/7] qapi/net.json: Add L4_Connection
> >> definition
> >>
> >> Zhang Chen <chen.zhang@intel.com> writes:
> >>
> >> > Add L4_Connection struct for other QMP commands.
> >> > Except protocol field is necessary, other fields are optional.
> >> >
> >> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> >> > ---
> >> >  qapi/net.json | 26 ++++++++++++++++++++++++++
> >> >  1 file changed, 26 insertions(+)
> >> >
> >> > diff --git a/qapi/net.json b/qapi/net.json index
> >> > 498ea7aa72..cd4a8ed95e 100644
> >> > --- a/qapi/net.json
> >> > +++ b/qapi/net.json
> >> > @@ -825,3 +825,29 @@
> >> >  { 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite',
> >> >      'icmp', 'igmp', 'ipv6' ] }
> >> >
> >> > +##
> >> > +# @L4_Connection:
> >> > +#
> >> > +# Layer 4 network connection.
> >> > +#
> >> > +# Just for IPv4.
> >> > +#
> >> > +# @protocol: Transport layer protocol like TCP/UDP...
> >> > +#
> >> > +# @id: For specific module with Qemu object ID, If there is no such part,
> >> > +#      it means global rules.
> >>
> >> Clear as mud.
> >
> > Sorry, let me re-clear it.
> > If I understand correctly, The ID shouldn't be here, but I found the 'boxed'
> flag just can add only one 'data' like this:
> > +##
> > +{ 'command': 'colo-passthrough-add', 'boxed': true,
> > +     'data': 'L4_Connection' }
> >
> > I original want to this:
> > +##
> > +{ 'command': 'colo-passthrough-add',
> > +     'data': { 'id': 'str', 'boxed': false, 'conn': 'L4_Connection',
> > +'boxed': true  }
> >
> > So, I add the @id as an optional argument here.
> >
> > rewrite the comments:
> > +# @id: Assign the rule to Qemu network handle module object ID. Like
> colo-compare, net-filter.
> >
> > Please see the ID details in patch3 too.
> 
> So, colo-passthrough-add takes an @id argument (to be tacked onto packets
> to help with further processing, I understand), and arguments to match
> packets.

Yes.

> 
> Naming the argument type L4_Connection is misleading.
> 
> Even naming the match arguments L4_Connection would be misleading.
> "Connection" has a specific meaning in networking.  There are TCP
> connections.  There is no such thing as an UDP connection.
> 
> A TCP connection is uniquely identified by a pair of endpoints, i.e. by source
> address, source port, destination address, destination port.
> Same for other connection-oriented protocols.  The protocol is not part of
> the connection.  Thus, L4_Connection would be misleading even for the
> connection-oriented case.
> 
> You need a named type for colo-passthrough-add's argument because you
> share it with colo-passthrough-del.  I'm not sure that's what we want (I'm
> going to write more on that in a moment).  If it is what we want, then please
> pick a another, descriptive name.

What do you think the "L4BypassRule" or "NetworkRule" ?

Thanks
Chen

RE: [PATCH V4 3/7] qapi/net: Add new QMP command for COLO passthrough

[Zhang, Chen]

> -----Original Message-----
> From: Markus Armbruster <armbru@redhat.com>
> Sent: Monday, March 22, 2021 8:16 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: Lukas Straub <lukasstraub2@web.de>; Li Zhijian
> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; qemu-
> dev <qemu-devel@nongnu.org>; Dr. David Alan Gilbert
> <dgilbert@redhat.com>; Zhang Chen <zhangckid@gmail.com>
> Subject: Re: [PATCH V4 3/7] qapi/net: Add new QMP command for COLO
> passthrough
> 
> "Zhang, Chen" <chen.zhang@intel.com> writes:
> 
> >> -----Original Message-----
> >> From: Markus Armbruster <armbru@redhat.com>
> >> Sent: Saturday, March 20, 2021 12:03 AM
> >> 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>; Li Zhijian <lizhijian@cn.fujitsu.com>;
> >> Lukas Straub <lukasstraub2@web.de>; Zhang Chen
> <zhangckid@gmail.com>
> >> Subject: Re: [PATCH V4 3/7] qapi/net: Add new 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.
> >> >
> >> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> >> > ---
> >> >  net/net.c     | 10 ++++++++++
> >> >  qapi/net.json | 40 ++++++++++++++++++++++++++++++++++++++++
> >> >  2 files changed, 50 insertions(+)
> >> >
> >> > diff --git a/net/net.c b/net/net.c
> >> > index 725a4e1450..7c7cefe0e0 100644
> >> > --- a/net/net.c
> >> > +++ b/net/net.c
> >> > @@ -1199,6 +1199,16 @@ void qmp_netdev_del(const char *id, Error
> >> **errp)
> >> >      }
> >> >  }
> >> >
> >> > +void qmp_colo_passthrough_add(L4_Connection *conn, Error **errp)
> {
> >> > +    /* Setup passthrough connection */
> >>
> >> Do you mean to say
> >>
> >>        /* TODO implement */
> >>
> >> ?
> >
> > Yes, I will input real code here in 7/7 patch.
> 
> Use a TODO comment then.
> 
> >>
> >> > +}
> >> > +
> >> > +void qmp_colo_passthrough_del(L4_Connection *conn, Error **errp)
> {
> >> > +    /* Delete passthrough connection */ }
> >>
> >> Likewise.
> >>
> >> > +
> >> >  static void netfilter_print_info(Monitor *mon, NetFilterState *nf)  {
> >> >      char *str;
> >> > diff --git a/qapi/net.json b/qapi/net.json index
> >> > cd4a8ed95e..ec7d3b1128 100644
> >> > --- a/qapi/net.json
> >> > +++ b/qapi/net.json
> >> > @@ -851,3 +851,43 @@
> >> >    'data': { 'protocol': 'IP_PROTOCOL', '*id': 'str', '*src_ip': 'str', '*dst_ip':
> 'str',
> >> >      '*src_port': 'int', '*dst_port': 'int' } }
> >> >
> >> > +##
> >> > +# @colo-passthrough-add:
> >> > +#
> >> > +# Add passthrough entry according to customer's needs in COLO-
> compare.
> >>
> >> QEMU doesn't have customers, it has users :)
> >
> > Thanks note.
> >
> >>
> >> > +#
> >> > +# Returns: Nothing on success
> >> > +#
> >> > +# Since: 6.1
> >> > +#
> >> > +# Example:
> >> > +#
> >> > +# -> { "execute": "colo-passthrough-add",
> >> > +#      "arguments": { "protocol": "tcp", "id": "object0", "src_ip":
> "192.168.1.1",
> >> > +#      "dst_ip": "192.168.1.2", "src_port": 1234, "dst_port": 4321 } }
> >> > +# <- { "return": {} }
> >> > +#
> >> > +##
> >> > +{ 'command': 'colo-passthrough-add', 'boxed': true,
> >> > +     'data': 'L4_Connection' }
> >> > +
> >> > +##
> >> > +# @colo-passthrough-del:
> >> > +#
> >> > +# Delete passthrough entry according to customer's needs in COLO-
> compare.
> >> > +#
> >> > +# Returns: Nothing on success
> >> > +#
> >> > +# Since: 6.1
> >> > +#
> >> > +# Example:
> >> > +#
> >> > +# -> { "execute": "colo-passthrough-del",
> >> > +#      "arguments": { "protocol": "tcp", "id": "object0", "src_ip":
> "192.168.1.1",
> >> > +#      "dst_ip": "192.168.1.2", "src_port": 1234, "dst_port": 4321 } }
> >> > +# <- { "return": {} }
> >> > +#
> >> > +##
> >> > +{ 'command': 'colo-passthrough-del', 'boxed': true,
> >> > +     'data': 'L4_Connection' }
> >> > +
> >>
> >> To make sense of this, I have to refer back to PATCH 1 and 2:
> >>
> >>    { 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite',
> >>        'icmp', 'igmp', 'ipv6' ] }
> >>
> >>    { 'struct': 'L4_Connection',
> >>      'data': { 'protocol': 'IP_PROTOCOL', '*id': 'str', '*src_ip': 'str', '*dst_ip':
> 'str',
> >>        '*src_port': 'int', '*dst_port': 'int' } }
> >>
> >> Please squash the three patches together.
> >
> > OK.
> >
> >>
> >> I figure colo-passthrough-add adds some kind of packet matching
> >> thingy that can match packets by source IP, source port, destination
> >> IP, destination port, and protocol.  Correct?
> >
> > Yes, you are right.
> >
> >>
> >> The protocol is mandatory, all others are optional.  What does it
> >> mean to omit an optional one?  Match all?
> >
> > Yes, match all. The idea from Jason Wang, for example:
> > User just set the protocol/source IP(tcp/192.168.1.1) , others empty.
> > The rule will bypass all the TCP packet from the source IP.
> 
> Work this into the doc comment, please.

OK.

> 
> >> I have no idea what @id is supposed to mean.  Please explain intended
> use.
> >
> > The @id means packet hander in Qemu. Because not all the guest network
> packet into the colo-compare module, the net-filters are same cases.
> > There modules attach to NIC or chardev socket to work, VM maybe have
> multi modules running. So we use the ID to set the rule to the specific
> module.
> 
> I'm not sure I understand, but then I'm a QEMU networking ignoramus :)
> 
> Work it into the doc comment.

Sure, I will add more comments in qapi/net.json next version.

Thanks
Chen

> 
> > Thanks
> > Chen
> >
> >>
> >> I'm ignoring colo-passthrough-del for now, because I feel need to
> >> understand -add first.

RE: [PATCH V4 3/7] qapi/net: Add new QMP command for COLO passthrough

[Zhang, Chen]

> -----Original Message-----
> From: Markus Armbruster <armbru@redhat.com>
> Sent: Monday, March 22, 2021 8:36 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>; Li Zhijian <lizhijian@cn.fujitsu.com>; Lukas
> Straub <lukasstraub2@web.de>; Zhang Chen <zhangckid@gmail.com>
> Subject: Re: [PATCH V4 3/7] qapi/net: Add new 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.
> >
> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> > ---
> >  net/net.c     | 10 ++++++++++
> >  qapi/net.json | 40 ++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 50 insertions(+)
> >
> > diff --git a/net/net.c b/net/net.c
> > index 725a4e1450..7c7cefe0e0 100644
> > --- a/net/net.c
> > +++ b/net/net.c
> > @@ -1199,6 +1199,16 @@ void qmp_netdev_del(const char *id, Error
> **errp)
> >      }
> >  }
> >
> > +void qmp_colo_passthrough_add(L4_Connection *conn, Error **errp) {
> > +    /* Setup passthrough connection */ }
> > +
> > +void qmp_colo_passthrough_del(L4_Connection *conn, Error **errp) {
> > +    /* Delete passthrough connection */ }
> > +
> >  static void netfilter_print_info(Monitor *mon, NetFilterState *nf)  {
> >      char *str;
> > diff --git a/qapi/net.json b/qapi/net.json index
> > cd4a8ed95e..ec7d3b1128 100644
> > --- a/qapi/net.json
> > +++ b/qapi/net.json
> > @@ -851,3 +851,43 @@
> >    'data': { 'protocol': 'IP_PROTOCOL', '*id': 'str', '*src_ip': 'str', '*dst_ip': 'str',
> >      '*src_port': 'int', '*dst_port': 'int' } }
> >
> > +##
> > +# @colo-passthrough-add:
> > +#
> > +# Add passthrough entry according to customer's needs in COLO-compare.
> > +#
> > +# Returns: Nothing on success
> > +#
> > +# Since: 6.1
> > +#
> > +# Example:
> > +#
> > +# -> { "execute": "colo-passthrough-add",
> > +#      "arguments": { "protocol": "tcp", "id": "object0", "src_ip":
> "192.168.1.1",
> > +#      "dst_ip": "192.168.1.2", "src_port": 1234, "dst_port": 4321 } }
> > +# <- { "return": {} }
> > +#
> > +##
> > +{ 'command': 'colo-passthrough-add', 'boxed': true,
> > +     'data': 'L4_Connection' }
> > +
> > +##
> > +# @colo-passthrough-del:
> > +#
> > +# Delete passthrough entry according to customer's needs in COLO-
> compare.
> > +#
> > +# Returns: Nothing on success
> > +#
> > +# Since: 6.1
> > +#
> > +# Example:
> > +#
> > +# -> { "execute": "colo-passthrough-del",
> > +#      "arguments": { "protocol": "tcp", "id": "object0", "src_ip":
> "192.168.1.1",
> > +#      "dst_ip": "192.168.1.2", "src_port": 1234, "dst_port": 4321 } }
> > +# <- { "return": {} }
> > +#
> > +##
> > +{ 'command': 'colo-passthrough-del', 'boxed': true,
> > +     'data': 'L4_Connection' }
> > +
> 
> Now let's look at colo-passthrough-del.  I figure it is for deleting the kind of
> things colo-passthrough-add adds.
> 

Yes.

> What exactly is deleted?  The thing created with the exact same arguments?
> 

Delete the rule from the module's private bypass list.
When user input a rule, the colo-passthrough-del will find the specific module by the object ID,
Then delete the rule.

> This would be unusual.  Commonly, FOO-add and FOO-del both take a string
> ID argument.  The FOO created by FOO-add remembers its ID, and FOO-del
> deletes by ID.

The ID not for rules itself, it just logged the modules(ID tagged) affected by the rule.

Thanks
Chen 

Re: [PATCH V4 2/7] qapi/net.json: Add L4_Connection definition

[Markus Armbruster]

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

>> -----Original Message-----
>> From: Markus Armbruster <armbru@redhat.com>
[...]
>> Naming the argument type L4_Connection is misleading.
>> 
>> Even naming the match arguments L4_Connection would be misleading.
>> "Connection" has a specific meaning in networking.  There are TCP
>> connections.  There is no such thing as an UDP connection.
>> 
>> A TCP connection is uniquely identified by a pair of endpoints, i.e. by source
>> address, source port, destination address, destination port.
>> Same for other connection-oriented protocols.  The protocol is not part of
>> the connection.  Thus, L4_Connection would be misleading even for the
>> connection-oriented case.
>> 
>> You need a named type for colo-passthrough-add's argument because you
>> share it with colo-passthrough-del.  I'm not sure that's what we want (I'm
>> going to write more on that in a moment).  If it is what we want, then please
>> pick a another, descriptive name.
>
> What do you think the "L4BypassRule" or "NetworkRule" ?

NetworkRule is too generic.

What about ColoPassthroughRule?

Re: [PATCH V4 3/7] qapi/net: Add new QMP command for COLO passthrough

[Markus Armbruster]

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

>> -----Original Message-----
>> From: Markus Armbruster <armbru@redhat.com>
[...]
>> Now let's look at colo-passthrough-del.  I figure it is for deleting the kind of
>> things colo-passthrough-add adds.
>> 
>
> Yes.
>
>> What exactly is deleted?  The thing created with the exact same arguments?
>> 
>
> Delete the rule from the module's private bypass list.
> When user input a rule, the colo-passthrough-del will find the specific module by the object ID,
> Then delete the rule.
>
>> This would be unusual.  Commonly, FOO-add and FOO-del both take a string
>> ID argument.  The FOO created by FOO-add remembers its ID, and FOO-del
>> deletes by ID.
>
> The ID not for rules itself, it just logged the modules(ID tagged) affected by the rule.

I'm not sure I understand.

If you're pointing out that existing colo-passthrough-del parameter @id
is not suitable for use as unique rule ID: you can always add another
parameter that is suitable.

Re: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition

[Dr. David Alan Gilbert]

* Zhang Chen (chen.zhang@intel.com) wrote:
> Add IP_PROTOCOL as enum include TCP,UDP, ICMP... for other QMP commands.
> 
> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> ---
>  qapi/net.json | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/qapi/net.json b/qapi/net.json
> index 87361ebd9a..498ea7aa72 100644
> --- a/qapi/net.json
> +++ b/qapi/net.json
> @@ -794,3 +794,34 @@
>  #
>  ##
>  { 'command': 'query-netdev', 'returns': ['NetdevInfo'] }
> +
> +##
> +# @IP_PROTOCOL:
> +#
> +# Transport layer protocol.
> +#
> +# Just for IPv4.
> +#
> +# @tcp: Transmission Control Protocol.
> +#
> +# @udp: User Datagram Protocol.
> +#
> +# @dccp: Datagram Congestion Control Protocol.
> +#
> +# @sctp: Stream Control Transmission Protocol.
> +#
> +# @udplite: Lightweight User Datagram Protocol.
> +#
> +# @icmp: Internet Control Message Protocol.
> +#
> +# @igmp: Internet Group Management Protocol.
> +#
> +# @ipv6: IPv6 Encapsulation.
> +#
> +# TODO: Need to add more transport layer protocol.
> +#
> +# Since: 6.1
> +##
> +{ 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite',
> +    'icmp', 'igmp', 'ipv6' ] }

Isn't the right thing to do here to use a string for protocol and then
pass it to getprotobyname;  that way your list is never out of date, and
you never have to translate between the order of this enum and the
integer assignment set in stone.

Dave

> +
> -- 
> 2.25.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH V4 2/7] qapi/net.json: Add L4_Connection definition

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> "Zhang, Chen" <chen.zhang@intel.com> writes:
> 
> >> -----Original Message-----
> >> From: Markus Armbruster <armbru@redhat.com>
> [...]
> >> Naming the argument type L4_Connection is misleading.
> >> 
> >> Even naming the match arguments L4_Connection would be misleading.
> >> "Connection" has a specific meaning in networking.  There are TCP
> >> connections.  There is no such thing as an UDP connection.
> >> 
> >> A TCP connection is uniquely identified by a pair of endpoints, i.e. by source
> >> address, source port, destination address, destination port.
> >> Same for other connection-oriented protocols.  The protocol is not part of
> >> the connection.  Thus, L4_Connection would be misleading even for the
> >> connection-oriented case.
> >> 
> >> You need a named type for colo-passthrough-add's argument because you
> >> share it with colo-passthrough-del.  I'm not sure that's what we want (I'm
> >> going to write more on that in a moment).  If it is what we want, then please
> >> pick a another, descriptive name.
> >
> > What do you think the "L4BypassRule" or "NetworkRule" ?
> 
> NetworkRule is too generic.
> 
> What about ColoPassthroughRule?

Which is a bit specific; there's not actually anything Colo specific in
there; can I suggest 'L4FlowSpec';  I think there should also be
a separate type that represents an IP address+port, so that what you end
up with is:

  IPFlowSpec
     ID
     Protocol
     Source
     Dest

Dave

-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

RE: [PATCH V4 2/7] qapi/net.json: Add L4_Connection definition

[Zhang, Chen]

> -----Original Message-----
> From: Markus Armbruster <armbru@redhat.com>
> Sent: Tuesday, March 23, 2021 5:55 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: Lukas Straub <lukasstraub2@web.de>; Li Zhijian
> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; qemu-
> dev <qemu-devel@nongnu.org>; Dr. David Alan Gilbert
> <dgilbert@redhat.com>; Zhang Chen <zhangckid@gmail.com>
> Subject: Re: [PATCH V4 2/7] qapi/net.json: Add L4_Connection definition
> 
> "Zhang, Chen" <chen.zhang@intel.com> writes:
> 
> >> -----Original Message-----
> >> From: Markus Armbruster <armbru@redhat.com>
> [...]
> >> Naming the argument type L4_Connection is misleading.
> >>
> >> Even naming the match arguments L4_Connection would be misleading.
> >> "Connection" has a specific meaning in networking.  There are TCP
> >> connections.  There is no such thing as an UDP connection.
> >>
> >> A TCP connection is uniquely identified by a pair of endpoints, i.e.
> >> by source address, source port, destination address, destination port.
> >> Same for other connection-oriented protocols.  The protocol is not
> >> part of the connection.  Thus, L4_Connection would be misleading even
> >> for the connection-oriented case.
> >>
> >> You need a named type for colo-passthrough-add's argument because
> you
> >> share it with colo-passthrough-del.  I'm not sure that's what we want
> >> (I'm going to write more on that in a moment).  If it is what we
> >> want, then please pick a another, descriptive name.
> >
> > What do you think the "L4BypassRule" or "NetworkRule" ?
> 
> NetworkRule is too generic.
> 
> What about ColoPassthroughRule?

It can be used by net filter modules(filter mirror,filter-dump....) in the future, that's not just for COLO.
PassthroughRule is better?

Thanks
Chen

Re: [PATCH V4 2/7] qapi/net.json: Add L4_Connection definition

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> "Zhang, Chen" <chen.zhang@intel.com> writes:
>> 
>> >> -----Original Message-----
>> >> From: Markus Armbruster <armbru@redhat.com>
>> [...]
>> >> Naming the argument type L4_Connection is misleading.
>> >> 
>> >> Even naming the match arguments L4_Connection would be misleading.
>> >> "Connection" has a specific meaning in networking.  There are TCP
>> >> connections.  There is no such thing as an UDP connection.
>> >> 
>> >> A TCP connection is uniquely identified by a pair of endpoints, i.e. by source
>> >> address, source port, destination address, destination port.
>> >> Same for other connection-oriented protocols.  The protocol is not part of
>> >> the connection.  Thus, L4_Connection would be misleading even for the
>> >> connection-oriented case.
>> >> 
>> >> You need a named type for colo-passthrough-add's argument because you
>> >> share it with colo-passthrough-del.  I'm not sure that's what we want (I'm
>> >> going to write more on that in a moment).  If it is what we want, then please
>> >> pick a another, descriptive name.
>> >
>> > What do you think the "L4BypassRule" or "NetworkRule" ?
>> 
>> NetworkRule is too generic.
>> 
>> What about ColoPassthroughRule?
>
> Which is a bit specific; there's not actually anything Colo specific in
> there; can I suggest 'L4FlowSpec';

"A bit too specific" is mostly harmless, since we can rename types at
any time (they are not visible in external interfaces).

This is *not* an objection to less specific names.  All I want is names
that don't give me wrong ideas on the thing's purpose.  L4FlowSpec and
IPFlowSpec (below) feel fine in that regard.

>                                     I think there should also beb
> a separate type that represents an IP address+port, so that what you end
> up with is:
>
>   IPFlowSpec
>      ID
>      Protocol
>      Source
>      Dest

I understand the motivation.  Three drawbacks, though.

One, it gets us another level of nesting on the wire, i.e. something
like

    {"source": {"address": SRC-ADDR, "port": SRC-PORT},
     "destination": {"address": DST-ADDR, "port": DST-PORT}}

instead of

    {"source-address": SRC-ADDR, "source-port": SRC-PORT,
     "destination-address": DST-ADDR, "destination-port": DST-PORT}

QMP clients shouldn't care.

Two, we have many (address, port) pairs in the schema that don't use
nesting.  Adding nesting sometimes makes QMP less consistent.

Three, human-friendly interface wrappers tend to dislike nesting.  This
particular case seems okay; we end up with dotted keys like
source.address instead of source-address.  In a case where we need just
one (address, port), we'd get some-silly-name.address instead of just
address, though.

I've occasionally felt a mild need for letting me say "this struct
member should be unboxed on the wire", i.e. have its curlies peeled off.
Never enough to justify the additional generator complexity, though.

Re: [PATCH V4 2/7] qapi/net.json: Add L4_Connection definition

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
[...]
>>                                     I think there should also beb
>> a separate type that represents an IP address+port, so that what you end
>> up with is:
>>
>>   IPFlowSpec
>>      ID
>>      Protocol
>>      Source
>>      Dest
>
> I understand the motivation.  Three drawbacks, though.
>
> One, it gets us another level of nesting on the wire, i.e. something
> like
>
>     {"source": {"address": SRC-ADDR, "port": SRC-PORT},
>      "destination": {"address": DST-ADDR, "port": DST-PORT}}
>
> instead of
>
>     {"source-address": SRC-ADDR, "source-port": SRC-PORT,
>      "destination-address": DST-ADDR, "destination-port": DST-PORT}
>
> QMP clients shouldn't care.
>
> Two, we have many (address, port) pairs in the schema that don't use
> nesting.  Adding nesting sometimes makes QMP less consistent.
>
> Three, human-friendly interface wrappers tend to dislike nesting.  This
> particular case seems okay; we end up with dotted keys like
> source.address instead of source-address.  In a case where we need just
> one (address, port), we'd get some-silly-name.address instead of just
> address, though.
>
> I've occasionally felt a mild need for letting me say "this struct
> member should be unboxed on the wire", i.e. have its curlies peeled off.
> Never enough to justify the additional generator complexity, though.

Just remembered we actually have

    ##
    # @InetSocketAddressBase:
    #
    # @host: host part of the address
    # @port: port part of the address
    ##
    { 'struct': 'InetSocketAddressBase',
      'data': {
        'host': 'str',
        'port': 'str' } }

It's from commit eb87203b64 "rbd: Reject -blockdev server.*.{numeric,
to, ipv4, ipv6}".

Re: [PATCH V4 2/7] qapi/net.json: Add L4_Connection definition

[Markus Armbruster]

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

> Add L4_Connection struct for other QMP commands.
> Except protocol field is necessary, other fields are optional.
>
> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> ---
>  qapi/net.json | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> diff --git a/qapi/net.json b/qapi/net.json
> index 498ea7aa72..cd4a8ed95e 100644
> --- a/qapi/net.json
> +++ b/qapi/net.json
> @@ -825,3 +825,29 @@
>  { 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite',
>      'icmp', 'igmp', 'ipv6' ] }
>  
> +##
> +# @L4_Connection:
> +#
> +# Layer 4 network connection.
> +#
> +# Just for IPv4.
> +#
> +# @protocol: Transport layer protocol like TCP/UDP...
> +#
> +# @id: For specific module with Qemu object ID, If there is no such part,
> +#      it means global rules.
> +#
> +# @src_ip: Source IP.
> +#
> +# @dst_ip: Destination IP.
> +#
> +# @src_port: Source port.
> +#
> +# @dst_port: Destination port.
> +#
> +# Since: 6.1
> +##
> +{ 'struct': 'L4_Connection',
> +  'data': { 'protocol': 'IP_PROTOCOL', '*id': 'str', '*src_ip': 'str', '*dst_ip': 'str',
> +    '*src_port': 'int', '*dst_port': 'int' } }
> +

Please use '-' instead of '_' in member names.  Patches to enforce this
rule just landed in master.

We tend to avoid abbreviations like src and dst in QMP.  Consider
spelling them out, except when it makes things less consistent.

Re: [PATCH V4 4/7] hmp-commands: Add new HMP command for COLO passthrough

[Dr. David Alan Gilbert]

* Zhang Chen (chen.zhang@intel.com) wrote:
> 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    | 34 ++++++++++++++++++++++++++++++++++
>  3 files changed, 62 insertions(+)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index d4001f9c5d..b67a5a04cb 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1335,6 +1335,32 @@ SRST
>    Remove host network device.
>  ERST
>  
> +    {
> +        .name       = "colo_passthrough_add",
> +        .args_type  = "protocol:s,id:s?,src_ip:s?,dst_ip:s?,src_port:i?,dst_port:i?",
> +        .params     = "protocol [id] [src_ip] [dst_ip] [src_port] [dst_port]",

That ordering is a bit unnatural; it's normal to keep ip and port
together;  maybe this would be better as:

   protocol:s,id:s,src:s?,dst:s?

then pass src and dst through inet_parse to get your hostname and port ?

Dave

> +        .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,id:s?,src_ip:s?,dst_ip:s?,src_port:i?,dst_port:i?",
> +        .params     = "protocol [id] [src_ip] [dst_ip] [src_port] [dst_port]",
> +        .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:O",
> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
> index ed2913fd18..3c4943b09f 100644
> --- a/include/monitor/hmp.h
> +++ b/include/monitor/hmp.h
> @@ -81,6 +81,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 3c88a4faef..b57e3430ab 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -1668,6 +1668,40 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
>      hmp_handle_error(mon, err);
>  }
>  
> +void hmp_colo_passthrough_add(Monitor *mon, const QDict *qdict)
> +{
> +    const char *prot = qdict_get_str(qdict, "protocol");
> +    L4_Connection *l4_conn = g_new0(L4_Connection, 1);
> +    Error *err = NULL;
> +
> +    l4_conn->id = g_strdup(qdict_get_try_str(qdict, "id"));
> +    l4_conn->protocol = qapi_enum_parse(&IP_PROTOCOL_lookup, prot, -1, &err);
> +    l4_conn->src_ip = g_strdup(qdict_get_try_str(qdict, "src_ip"));
> +    l4_conn->dst_ip = g_strdup(qdict_get_try_str(qdict, "dst_ip"));
> +    l4_conn->src_port = qdict_get_try_int(qdict, "src_port", 0);
> +    l4_conn->dst_port = qdict_get_try_int(qdict, "dst_port", 0);
> +
> +    qmp_colo_passthrough_add(l4_conn, &err);
> +    hmp_handle_error(mon, err);
> +}
> +
> +void hmp_colo_passthrough_del(Monitor *mon, const QDict *qdict)
> +{
> +    const char *prot = qdict_get_str(qdict, "protocol");
> +    L4_Connection *l4_conn = g_new0(L4_Connection, 1);
> +    Error *err = NULL;
> +
> +    l4_conn->id = g_strdup(qdict_get_try_str(qdict, "id"));
> +    l4_conn->protocol = qapi_enum_parse(&IP_PROTOCOL_lookup, prot, -1, &err);
> +    l4_conn->src_ip = g_strdup(qdict_get_try_str(qdict, "src_ip"));
> +    l4_conn->dst_ip = g_strdup(qdict_get_try_str(qdict, "dst_ip"));
> +    l4_conn->src_port = qdict_get_try_int(qdict, "src_port", 0);
> +    l4_conn->dst_port = qdict_get_try_int(qdict, "dst_port", 0);
> +
> +    qmp_colo_passthrough_del(l4_conn, &err);
> +    hmp_handle_error(mon, err);
> +}
> +
>  void hmp_object_add(Monitor *mon, const QDict *qdict)
>  {
>      Error *err = NULL;
> -- 
> 2.25.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH V4 5/7] net/colo-compare: Move data structure and define to .h file.

[Dr. David Alan Gilbert]

* Zhang Chen (chen.zhang@intel.com) wrote:
> Make other modules can reuse COLO code.
> 
> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> ---
>  net/colo-compare.c | 106 ---------------------------------------------
>  net/colo-compare.h | 106 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 106 insertions(+), 106 deletions(-)
> 

<snip>

> diff --git a/net/colo-compare.h b/net/colo-compare.h
> index 22ddd512e2..2a9dcac0a7 100644
> --- a/net/colo-compare.h
> +++ b/net/colo-compare.h
> @@ -17,6 +17,112 @@
>  #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)
> +
> +#define COMPARE_READ_LEN_MAX NET_BUFSIZE
> +#define MAX_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
> +
> +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;

^^^^^

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

 ^^^^^

For a header, these are too generic names - they need to have Colo in
them; e.g. MAX_COLOQUEUE_SIZE and COLOSendEntry etc

Dave

> +    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,
> +};
> +
>  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
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

RE: [PATCH V4 2/7] qapi/net.json: Add L4_Connection definition

[Zhang, Chen]

> -----Original Message-----
> From: Markus Armbruster <armbru@redhat.com>
> Sent: Wednesday, March 24, 2021 2:47 PM
> To: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Lukas Straub <lukasstraub2@web.de>; Li Zhijian
> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; qemu-
> dev <qemu-devel@nongnu.org>; Zhang, Chen <chen.zhang@intel.com>;
> Zhang Chen <zhangckid@gmail.com>
> Subject: Re: [PATCH V4 2/7] qapi/net.json: Add L4_Connection definition
> 
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> 
> > * Markus Armbruster (armbru@redhat.com) wrote:
> >> "Zhang, Chen" <chen.zhang@intel.com> writes:
> >>
> >> >> -----Original Message-----
> >> >> From: Markus Armbruster <armbru@redhat.com>
> >> [...]
> >> >> Naming the argument type L4_Connection is misleading.
> >> >>
> >> >> Even naming the match arguments L4_Connection would be
> misleading.
> >> >> "Connection" has a specific meaning in networking.  There are TCP
> >> >> connections.  There is no such thing as an UDP connection.
> >> >>
> >> >> A TCP connection is uniquely identified by a pair of endpoints,
> >> >> i.e. by source address, source port, destination address, destination
> port.
> >> >> Same for other connection-oriented protocols.  The protocol is not
> >> >> part of the connection.  Thus, L4_Connection would be misleading
> >> >> even for the connection-oriented case.
> >> >>
> >> >> You need a named type for colo-passthrough-add's argument because
> >> >> you share it with colo-passthrough-del.  I'm not sure that's what
> >> >> we want (I'm going to write more on that in a moment).  If it is
> >> >> what we want, then please pick a another, descriptive name.
> >> >
> >> > What do you think the "L4BypassRule" or "NetworkRule" ?
> >>
> >> NetworkRule is too generic.
> >>
> >> What about ColoPassthroughRule?
> >
> > Which is a bit specific; there's not actually anything Colo specific
> > in there; can I suggest 'L4FlowSpec';
> 
> "A bit too specific" is mostly harmless, since we can rename types at any time
> (they are not visible in external interfaces).
> 
> This is *not* an objection to less specific names.  All I want is names that
> don't give me wrong ideas on the thing's purpose.  L4FlowSpec and
> IPFlowSpec (below) feel fine in that regard.
> 
> >                                     I think there should also beb a
> > separate type that represents an IP address+port, so that what you end
> > up with is:
> >
> >   IPFlowSpec
> >      ID
> >      Protocol
> >      Source
> >      Dest
> 
> I understand the motivation.  Three drawbacks, though.
> 
> One, it gets us another level of nesting on the wire, i.e. something like
> 
>     {"source": {"address": SRC-ADDR, "port": SRC-PORT},
>      "destination": {"address": DST-ADDR, "port": DST-PORT}}
> 
> instead of
> 
>     {"source-address": SRC-ADDR, "source-port": SRC-PORT,
>      "destination-address": DST-ADDR, "destination-port": DST-PORT}
> 
> QMP clients shouldn't care.
> 
> Two, we have many (address, port) pairs in the schema that don't use
> nesting.  Adding nesting sometimes makes QMP less consistent.
> 
> Three, human-friendly interface wrappers tend to dislike nesting.  This
> particular case seems okay; we end up with dotted keys like source.address
> instead of source-address.  In a case where we need just one (address, port),
> we'd get some-silly-name.address instead of just address, though.
> 
> I've occasionally felt a mild need for letting me say "this struct member
> should be unboxed on the wire", i.e. have its curlies peeled off.
> Never enough to justify the additional generator complexity, though.

The initial patch of this series used unboxed struct, Eric's comments is change it to boxed.
I think it's OK, for the unused field we can keep 0 for it. The n-tuple(src IP, dst IP, src port, dst port, protocol)
will be used in many place on Qemu network related code(like migrate, NBD....).  
For the name, I think Dave's comments is well, for the @InetSocketAddressBase we can remove it and change it to use IPFlowSpec.
Markus, what do you think about it?

Thanks
Chen

RE: [PATCH V4 5/7] net/colo-compare: Move data structure and define to .h file.

[Zhang, Chen]

> -----Original Message-----
> From: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Sent: Wednesday, March 24, 2021 7: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>; Markus Armbruster
> <armbru@redhat.com>; Li Zhijian <lizhijian@cn.fujitsu.com>; Zhang Chen
> <zhangckid@gmail.com>; Lukas Straub <lukasstraub2@web.de>
> Subject: Re: [PATCH V4 5/7] net/colo-compare: Move data structure and
> define to .h file.
> 
> * Zhang Chen (chen.zhang@intel.com) wrote:
> > Make other modules can reuse COLO code.
> >
> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> > ---
> >  net/colo-compare.c | 106
> > ---------------------------------------------
> >  net/colo-compare.h | 106
> > +++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 106 insertions(+), 106 deletions(-)
> >
> 
> <snip>
> 
> > diff --git a/net/colo-compare.h b/net/colo-compare.h index
> > 22ddd512e2..2a9dcac0a7 100644
> > --- a/net/colo-compare.h
> > +++ b/net/colo-compare.h
> > @@ -17,6 +17,112 @@
> >  #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)
> > +
> > +#define COMPARE_READ_LEN_MAX NET_BUFSIZE #define
> MAX_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
> > +
> > +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;
> 
> ^^^^^
> 
> > +/*
> > + *  + 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 {
> 
>  ^^^^^
> 
> For a header, these are too generic names - they need to have Colo in them;
> e.g. MAX_COLOQUEUE_SIZE and COLOSendEntry etc

Thanks Dave, I will fix it in next version.

Thanks
Chen

> 
> Dave
> 
> > +    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,
> > +};
> > +
> >  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
> >
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

RE: [PATCH V4 3/7] qapi/net: Add new QMP command for COLO passthrough

[Zhang, Chen]

> -----Original Message-----
> From: Qemu-devel <qemu-devel-
> bounces+chen.zhang=intel.com@nongnu.org> On Behalf Of Markus
> Armbruster
> Sent: Tuesday, March 23, 2021 5:58 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: Lukas Straub <lukasstraub2@web.de>; Li Zhijian
> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; qemu-
> dev <qemu-devel@nongnu.org>; Dr. David Alan Gilbert
> <dgilbert@redhat.com>; Zhang Chen <zhangckid@gmail.com>
> Subject: Re: [PATCH V4 3/7] qapi/net: Add new QMP command for COLO
> passthrough
> 
> "Zhang, Chen" <chen.zhang@intel.com> writes:
> 
> >> -----Original Message-----
> >> From: Markus Armbruster <armbru@redhat.com>
> [...]
> >> Now let's look at colo-passthrough-del.  I figure it is for deleting
> >> the kind of things colo-passthrough-add adds.
> >>
> >
> > Yes.
> >
> >> What exactly is deleted?  The thing created with the exact same
> arguments?
> >>
> >
> > Delete the rule from the module's private bypass list.
> > When user input a rule, the colo-passthrough-del will find the
> > specific module by the object ID, Then delete the rule.
> >
> >> This would be unusual.  Commonly, FOO-add and FOO-del both take a
> >> string ID argument.  The FOO created by FOO-add remembers its ID, and
> >> FOO-del deletes by ID.
> >
> > The ID not for rules itself, it just logged the modules(ID tagged) affected by
> the rule.
> 
> I'm not sure I understand.
> 
> If you're pointing out that existing colo-passthrough-del parameter @id is not
> suitable for use as unique rule ID: you can always add another parameter
> that is suitable.

Sorry to missed this mail.

For example:
The VM running with filter-mirror(object id==0), filter-redirector(object id==1) and colo-compare(object id==2),
We use  colo-passthrough-add/del to add/del a rule with a ID, if the ID==2, the rule just affect to colo-compare.
The filter-mirror and filter-redirector feel nothing after the add/del.

Thanks
Chen

> 

Re: [PATCH V4 3/7] qapi/net: Add new QMP command for COLO passthrough

[Markus Armbruster]

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

>> -----Original Message-----
>> From: Qemu-devel <qemu-devel-
>> bounces+chen.zhang=intel.com@nongnu.org> On Behalf Of Markus
>> Armbruster
>> Sent: Tuesday, March 23, 2021 5:58 PM
>> To: Zhang, Chen <chen.zhang@intel.com>
>> Cc: Lukas Straub <lukasstraub2@web.de>; Li Zhijian
>> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; qemu-
>> dev <qemu-devel@nongnu.org>; Dr. David Alan Gilbert
>> <dgilbert@redhat.com>; Zhang Chen <zhangckid@gmail.com>
>> Subject: Re: [PATCH V4 3/7] qapi/net: Add new QMP command for COLO
>> passthrough
>> 
>> "Zhang, Chen" <chen.zhang@intel.com> writes:
>> 
>> >> -----Original Message-----
>> >> From: Markus Armbruster <armbru@redhat.com>
>> [...]
>> >> Now let's look at colo-passthrough-del.  I figure it is for deleting
>> >> the kind of things colo-passthrough-add adds.
>> >>
>> >
>> > Yes.
>> >
>> >> What exactly is deleted?  The thing created with the exact same
>> arguments?
>> >>
>> >
>> > Delete the rule from the module's private bypass list.
>> > When user input a rule, the colo-passthrough-del will find the
>> > specific module by the object ID, Then delete the rule.
>> >
>> >> This would be unusual.  Commonly, FOO-add and FOO-del both take a
>> >> string ID argument.  The FOO created by FOO-add remembers its ID, and
>> >> FOO-del deletes by ID.
>> >
>> > The ID not for rules itself, it just logged the modules(ID tagged) affected by
>> the rule.
>> 
>> I'm not sure I understand.
>> 
>> If you're pointing out that existing colo-passthrough-del parameter @id is not
>> suitable for use as unique rule ID: you can always add another parameter
>> that is suitable.
>
> Sorry to missed this mail.
>
> For example:
> The VM running with filter-mirror(object id==0), filter-redirector(object id==1) and colo-compare(object id==2),
> We use  colo-passthrough-add/del to add/del a rule with a ID, if the ID==2, the rule just affect to colo-compare.
> The filter-mirror and filter-redirector feel nothing after the add/del.

I think you're trying to explain existing parameter @id.  The point I
was trying to make is unrelated to this parameter, except by name
collision.

My point is: our existing "delete" operations select the object to be
deleted by some unique name that is assigned by the "add" operation.
The unique name is a property of the object.  The property name is
often, but not always "id".

Examples:

    device_add argument "id" sets the device's unique name.
    device_del argument "id" selects the device to delete by its name.

    blockdev-add argument "node-name" sets the block backend device's
    unique name.
    blockdev-del argument "node-name" selects the block backend device
    to delete by its name.

Is there any particular reason why deletion of your kind of object can't
work the same way?

RE: [PATCH V4 3/7] qapi/net: Add new QMP command for COLO passthrough

[Zhang, Chen]

> -----Original Message-----
> From: Markus Armbruster <armbru@redhat.com>
> Sent: Tuesday, April 6, 2021 4:01 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: Lukas Straub <lukasstraub2@web.de>; Li Zhijian
> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; qemu-
> dev <qemu-devel@nongnu.org>; Dr. David Alan Gilbert
> <dgilbert@redhat.com>; Zhang Chen <zhangckid@gmail.com>
> Subject: Re: [PATCH V4 3/7] qapi/net: Add new QMP command for COLO
> passthrough
> 
> "Zhang, Chen" <chen.zhang@intel.com> writes:
> 
> >> -----Original Message-----
> >> From: Qemu-devel <qemu-devel-
> >> bounces+chen.zhang=intel.com@nongnu.org> On Behalf Of Markus
> >> Armbruster
> >> Sent: Tuesday, March 23, 2021 5:58 PM
> >> To: Zhang, Chen <chen.zhang@intel.com>
> >> Cc: Lukas Straub <lukasstraub2@web.de>; Li Zhijian
> >> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; qemu-
> >> dev <qemu-devel@nongnu.org>; Dr. David Alan Gilbert
> >> <dgilbert@redhat.com>; Zhang Chen <zhangckid@gmail.com>
> >> Subject: Re: [PATCH V4 3/7] qapi/net: Add new QMP command for COLO
> >> passthrough
> >>
> >> "Zhang, Chen" <chen.zhang@intel.com> writes:
> >>
> >> >> -----Original Message-----
> >> >> From: Markus Armbruster <armbru@redhat.com>
> >> [...]
> >> >> Now let's look at colo-passthrough-del.  I figure it is for
> >> >> deleting the kind of things colo-passthrough-add adds.
> >> >>
> >> >
> >> > Yes.
> >> >
> >> >> What exactly is deleted?  The thing created with the exact same
> >> arguments?
> >> >>
> >> >
> >> > Delete the rule from the module's private bypass list.
> >> > When user input a rule, the colo-passthrough-del will find the
> >> > specific module by the object ID, Then delete the rule.
> >> >
> >> >> This would be unusual.  Commonly, FOO-add and FOO-del both take a
> >> >> string ID argument.  The FOO created by FOO-add remembers its ID,
> >> >> and FOO-del deletes by ID.
> >> >
> >> > The ID not for rules itself, it just logged the modules(ID tagged)
> >> > affected by
> >> the rule.
> >>
> >> I'm not sure I understand.
> >>
> >> If you're pointing out that existing colo-passthrough-del parameter
> >> @id is not suitable for use as unique rule ID: you can always add
> >> another parameter that is suitable.
> >
> > Sorry to missed this mail.
> >
> > For example:
> > The VM running with filter-mirror(object id==0),
> > filter-redirector(object id==1) and colo-compare(object id==2), We use
> colo-passthrough-add/del to add/del a rule with a ID, if the ID==2, the rule
> just affect to colo-compare.
> > The filter-mirror and filter-redirector feel nothing after the add/del.
> 
> I think you're trying to explain existing parameter @id.  The point I was trying
> to make is unrelated to this parameter, except by name collision.
> 
> My point is: our existing "delete" operations select the object to be deleted
> by some unique name that is assigned by the "add" operation.
> The unique name is a property of the object.  The property name is often,
> but not always "id".
> 
> Examples:
> 
>     device_add argument "id" sets the device's unique name.
>     device_del argument "id" selects the device to delete by its name.
> 
>     blockdev-add argument "node-name" sets the block backend device's
>     unique name.
>     blockdev-del argument "node-name" selects the block backend device
>     to delete by its name.
> 
> Is there any particular reason why deletion of your kind of object can't work
> the same way?

Current command can work in this way, It seems that name "ID" can be misunderstood.
The id=object0 is OK here. I will change the "id" to "object-name".
Thank you for clear the comments.

Thanks   
Chen

RE: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition

[Zhang, Chen]

> -----Original Message-----
> From: Qemu-devel <qemu-devel-
> bounces+chen.zhang=intel.com@nongnu.org> On Behalf Of Dr. David Alan
> Gilbert
> Sent: Wednesday, March 24, 2021 4:01 AM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: Lukas Straub <lukasstraub2@web.de>; Li Zhijian
> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; qemu-
> dev <qemu-devel@nongnu.org>; Markus Armbruster
> <armbru@redhat.com>; Zhang Chen <zhangckid@gmail.com>
> Subject: Re: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition
> 
> * Zhang Chen (chen.zhang@intel.com) wrote:
> > Add IP_PROTOCOL as enum include TCP,UDP, ICMP... for other QMP
> commands.
> >
> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> > ---
> >  qapi/net.json | 31 +++++++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >
> > diff --git a/qapi/net.json b/qapi/net.json index
> > 87361ebd9a..498ea7aa72 100644
> > --- a/qapi/net.json
> > +++ b/qapi/net.json
> > @@ -794,3 +794,34 @@
> >  #
> >  ##
> >  { 'command': 'query-netdev', 'returns': ['NetdevInfo'] }
> > +
> > +##
> > +# @IP_PROTOCOL:
> > +#
> > +# Transport layer protocol.
> > +#
> > +# Just for IPv4.
> > +#
> > +# @tcp: Transmission Control Protocol.
> > +#
> > +# @udp: User Datagram Protocol.
> > +#
> > +# @dccp: Datagram Congestion Control Protocol.
> > +#
> > +# @sctp: Stream Control Transmission Protocol.
> > +#
> > +# @udplite: Lightweight User Datagram Protocol.
> > +#
> > +# @icmp: Internet Control Message Protocol.
> > +#
> > +# @igmp: Internet Group Management Protocol.
> > +#
> > +# @ipv6: IPv6 Encapsulation.
> > +#
> > +# TODO: Need to add more transport layer protocol.
> > +#
> > +# Since: 6.1
> > +##
> > +{ 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite',
> > +    'icmp', 'igmp', 'ipv6' ] }
> 
> Isn't the right thing to do here to use a string for protocol and then pass it to
> getprotobyname;  that way your list is never out of date, and you never have
> to translate between the order of this enum and the integer assignment set
> in stone.
> 

Hi Dave,

Considering that most of the scenes in Qemu don't call the getprotobyname, looks keep the string are more readable.
Please review the V5 patches, Thanks.

Thanks
Chen

> Dave
> 
> > +
> > --
> > 2.25.1
> >
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 

RE: [PATCH V4 4/7] hmp-commands: Add new HMP command for COLO passthrough

[Zhang, Chen]

> -----Original Message-----
> From: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Sent: Wednesday, March 24, 2021 6:40 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>; Markus Armbruster
> <armbru@redhat.com>; Li Zhijian <lizhijian@cn.fujitsu.com>; Zhang Chen
> <zhangckid@gmail.com>; Lukas Straub <lukasstraub2@web.de>
> Subject: Re: [PATCH V4 4/7] hmp-commands: Add new HMP command for
> COLO passthrough
> 
> * Zhang Chen (chen.zhang@intel.com) wrote:
> > 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    | 34 ++++++++++++++++++++++++++++++++++
> >  3 files changed, 62 insertions(+)
> >
> > diff --git a/hmp-commands.hx b/hmp-commands.hx index
> > d4001f9c5d..b67a5a04cb 100644
> > --- a/hmp-commands.hx
> > +++ b/hmp-commands.hx
> > @@ -1335,6 +1335,32 @@ SRST
> >    Remove host network device.
> >  ERST
> >
> > +    {
> > +        .name       = "colo_passthrough_add",
> > +        .args_type  =
> "protocol:s,id:s?,src_ip:s?,dst_ip:s?,src_port:i?,dst_port:i?",
> > +        .params     = "protocol [id] [src_ip] [dst_ip] [src_port] [dst_port]",
> 
> That ordering is a bit unnatural; it's normal to keep ip and port together;
> maybe this would be better as:
> 
>    protocol:s,id:s,src:s?,dst:s?
> 
> then pass src and dst through inet_parse to get your hostname and port ?

OK, already update to V5, please review it.

Thanks
Chen

> 
> Dave
> 
> > +        .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,id:s?,src_ip:s?,dst_ip:s?,src_port:i?,dst_port:i?",
> > +        .params     = "protocol [id] [src_ip] [dst_ip] [src_port] [dst_port]",
> > +        .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:O",
> > diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h index
> > ed2913fd18..3c4943b09f 100644
> > --- a/include/monitor/hmp.h
> > +++ b/include/monitor/hmp.h
> > @@ -81,6 +81,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
> 3c88a4faef..b57e3430ab
> > 100644
> > --- a/monitor/hmp-cmds.c
> > +++ b/monitor/hmp-cmds.c
> > @@ -1668,6 +1668,40 @@ void hmp_netdev_del(Monitor *mon, const
> QDict *qdict)
> >      hmp_handle_error(mon, err);
> >  }
> >
> > +void hmp_colo_passthrough_add(Monitor *mon, const QDict *qdict) {
> > +    const char *prot = qdict_get_str(qdict, "protocol");
> > +    L4_Connection *l4_conn = g_new0(L4_Connection, 1);
> > +    Error *err = NULL;
> > +
> > +    l4_conn->id = g_strdup(qdict_get_try_str(qdict, "id"));
> > +    l4_conn->protocol = qapi_enum_parse(&IP_PROTOCOL_lookup, prot, -
> 1, &err);
> > +    l4_conn->src_ip = g_strdup(qdict_get_try_str(qdict, "src_ip"));
> > +    l4_conn->dst_ip = g_strdup(qdict_get_try_str(qdict, "dst_ip"));
> > +    l4_conn->src_port = qdict_get_try_int(qdict, "src_port", 0);
> > +    l4_conn->dst_port = qdict_get_try_int(qdict, "dst_port", 0);
> > +
> > +    qmp_colo_passthrough_add(l4_conn, &err);
> > +    hmp_handle_error(mon, err);
> > +}
> > +
> > +void hmp_colo_passthrough_del(Monitor *mon, const QDict *qdict) {
> > +    const char *prot = qdict_get_str(qdict, "protocol");
> > +    L4_Connection *l4_conn = g_new0(L4_Connection, 1);
> > +    Error *err = NULL;
> > +
> > +    l4_conn->id = g_strdup(qdict_get_try_str(qdict, "id"));
> > +    l4_conn->protocol = qapi_enum_parse(&IP_PROTOCOL_lookup, prot, -
> 1, &err);
> > +    l4_conn->src_ip = g_strdup(qdict_get_try_str(qdict, "src_ip"));
> > +    l4_conn->dst_ip = g_strdup(qdict_get_try_str(qdict, "dst_ip"));
> > +    l4_conn->src_port = qdict_get_try_int(qdict, "src_port", 0);
> > +    l4_conn->dst_port = qdict_get_try_int(qdict, "dst_port", 0);
> > +
> > +    qmp_colo_passthrough_del(l4_conn, &err);
> > +    hmp_handle_error(mon, err);
> > +}
> > +
> >  void hmp_object_add(Monitor *mon, const QDict *qdict)  {
> >      Error *err = NULL;
> > --
> > 2.25.1
> >
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition

[Markus Armbruster]

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

>> -----Original Message-----
>> From: Qemu-devel <qemu-devel-
>> bounces+chen.zhang=intel.com@nongnu.org> On Behalf Of Dr. David Alan
>> Gilbert
>> Sent: Wednesday, March 24, 2021 4:01 AM
>> To: Zhang, Chen <chen.zhang@intel.com>
>> Cc: Lukas Straub <lukasstraub2@web.de>; Li Zhijian
>> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; qemu-
>> dev <qemu-devel@nongnu.org>; Markus Armbruster
>> <armbru@redhat.com>; Zhang Chen <zhangckid@gmail.com>
>> Subject: Re: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition
>> 
>> * Zhang Chen (chen.zhang@intel.com) wrote:
>> > Add IP_PROTOCOL as enum include TCP,UDP, ICMP... for other QMP
>> commands.
>> >
>> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
>> > ---
>> >  qapi/net.json | 31 +++++++++++++++++++++++++++++++
>> >  1 file changed, 31 insertions(+)
>> >
>> > diff --git a/qapi/net.json b/qapi/net.json index
>> > 87361ebd9a..498ea7aa72 100644
>> > --- a/qapi/net.json
>> > +++ b/qapi/net.json
>> > @@ -794,3 +794,34 @@
>> >  #
>> >  ##
>> >  { 'command': 'query-netdev', 'returns': ['NetdevInfo'] }
>> > +
>> > +##
>> > +# @IP_PROTOCOL:
>> > +#
>> > +# Transport layer protocol.
>> > +#
>> > +# Just for IPv4.
>> > +#
>> > +# @tcp: Transmission Control Protocol.
>> > +#
>> > +# @udp: User Datagram Protocol.
>> > +#
>> > +# @dccp: Datagram Congestion Control Protocol.
>> > +#
>> > +# @sctp: Stream Control Transmission Protocol.
>> > +#
>> > +# @udplite: Lightweight User Datagram Protocol.
>> > +#
>> > +# @icmp: Internet Control Message Protocol.
>> > +#
>> > +# @igmp: Internet Group Management Protocol.
>> > +#
>> > +# @ipv6: IPv6 Encapsulation.
>> > +#
>> > +# TODO: Need to add more transport layer protocol.
>> > +#
>> > +# Since: 6.1
>> > +##
>> > +{ 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite',
>> > +    'icmp', 'igmp', 'ipv6' ] }
>> 
>> Isn't the right thing to do here to use a string for protocol and then pass it to
>> getprotobyname;  that way your list is never out of date, and you never have
>> to translate between the order of this enum and the integer assignment set
>> in stone.
>> 
>
> Hi Dave,
>
> Considering that most of the scenes in Qemu don't call the getprotobyname, looks keep the string are more readable.

Unless I'm missing something,

(1) this enum is only used for matching packets by source IP, source
port, destination IP, destination port, and protocol, and

(2) the packet matching works just fine for *any* protocol.

By using an enum for the protocol, you're limiting the matcher to
whatever protocols we bother to include in the enum for no particular
reason.  Feels wrong to me.

> Please review the V5 patches, Thanks.
>
> Thanks
> Chen
>
>> Dave
>> 
>> > +
>> > --
>> > 2.25.1
>> >
>> --
>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>> 

RE: [PATCH V4 4/7] hmp-commands: Add new HMP command for COLO passthrough

[Zhang, Chen]

> -----Original Message-----
> From: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Sent: Wednesday, March 24, 2021 6:40 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>; Markus Armbruster
> <armbru@redhat.com>; Li Zhijian <lizhijian@cn.fujitsu.com>; Zhang Chen
> <zhangckid@gmail.com>; Lukas Straub <lukasstraub2@web.de>
> Subject: Re: [PATCH V4 4/7] hmp-commands: Add new HMP command for
> COLO passthrough
> 
> * Zhang Chen (chen.zhang@intel.com) wrote:
> > 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    | 34 ++++++++++++++++++++++++++++++++++
> >  3 files changed, 62 insertions(+)
> >
> > diff --git a/hmp-commands.hx b/hmp-commands.hx index
> > d4001f9c5d..b67a5a04cb 100644
> > --- a/hmp-commands.hx
> > +++ b/hmp-commands.hx
> > @@ -1335,6 +1335,32 @@ SRST
> >    Remove host network device.
> >  ERST
> >
> > +    {
> > +        .name       = "colo_passthrough_add",
> > +        .args_type  =
> "protocol:s,id:s?,src_ip:s?,dst_ip:s?,src_port:i?,dst_port:i?",
> > +        .params     = "protocol [id] [src_ip] [dst_ip] [src_port] [dst_port]",
> 
> That ordering is a bit unnatural; it's normal to keep ip and port together;
> maybe this would be better as:
> 
>    protocol:s,id:s,src:s?,dst:s?
> 
> then pass src and dst through inet_parse to get your hostname and port ?

OK, already update to V5, please review it.

Thanks
Chen

> 
> Dave
> 
> > +        .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,id:s?,src_ip:s?,dst_ip:s?,src_port:i?,dst_port:i?",
> > +        .params     = "protocol [id] [src_ip] [dst_ip] [src_port] [dst_port]",
> > +        .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:O",
> > diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h index
> > ed2913fd18..3c4943b09f 100644
> > --- a/include/monitor/hmp.h
> > +++ b/include/monitor/hmp.h
> > @@ -81,6 +81,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
> 3c88a4faef..b57e3430ab
> > 100644
> > --- a/monitor/hmp-cmds.c
> > +++ b/monitor/hmp-cmds.c
> > @@ -1668,6 +1668,40 @@ void hmp_netdev_del(Monitor *mon, const
> QDict *qdict)
> >      hmp_handle_error(mon, err);
> >  }
> >
> > +void hmp_colo_passthrough_add(Monitor *mon, const QDict *qdict) {
> > +    const char *prot = qdict_get_str(qdict, "protocol");
> > +    L4_Connection *l4_conn = g_new0(L4_Connection, 1);
> > +    Error *err = NULL;
> > +
> > +    l4_conn->id = g_strdup(qdict_get_try_str(qdict, "id"));
> > +    l4_conn->protocol = qapi_enum_parse(&IP_PROTOCOL_lookup, prot, -
> 1, &err);
> > +    l4_conn->src_ip = g_strdup(qdict_get_try_str(qdict, "src_ip"));
> > +    l4_conn->dst_ip = g_strdup(qdict_get_try_str(qdict, "dst_ip"));
> > +    l4_conn->src_port = qdict_get_try_int(qdict, "src_port", 0);
> > +    l4_conn->dst_port = qdict_get_try_int(qdict, "dst_port", 0);
> > +
> > +    qmp_colo_passthrough_add(l4_conn, &err);
> > +    hmp_handle_error(mon, err);
> > +}
> > +
> > +void hmp_colo_passthrough_del(Monitor *mon, const QDict *qdict) {
> > +    const char *prot = qdict_get_str(qdict, "protocol");
> > +    L4_Connection *l4_conn = g_new0(L4_Connection, 1);
> > +    Error *err = NULL;
> > +
> > +    l4_conn->id = g_strdup(qdict_get_try_str(qdict, "id"));
> > +    l4_conn->protocol = qapi_enum_parse(&IP_PROTOCOL_lookup, prot, -
> 1, &err);
> > +    l4_conn->src_ip = g_strdup(qdict_get_try_str(qdict, "src_ip"));
> > +    l4_conn->dst_ip = g_strdup(qdict_get_try_str(qdict, "dst_ip"));
> > +    l4_conn->src_port = qdict_get_try_int(qdict, "src_port", 0);
> > +    l4_conn->dst_port = qdict_get_try_int(qdict, "dst_port", 0);
> > +
> > +    qmp_colo_passthrough_del(l4_conn, &err);
> > +    hmp_handle_error(mon, err);
> > +}
> > +
> >  void hmp_object_add(Monitor *mon, const QDict *qdict)  {
> >      Error *err = NULL;
> > --
> > 2.25.1
> >
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

RE: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition

[Zhang, Chen]

> -----Original Message-----
> From: Markus Armbruster <armbru@redhat.com>
> Sent: Thursday, April 15, 2021 11:15 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>; Lukas Straub
> <lukasstraub2@web.de>; Li Zhijian <lizhijian@cn.fujitsu.com>; Jason Wang
> <jasowang@redhat.com>; qemu-dev <qemu-devel@nongnu.org>; Zhang
> Chen <zhangckid@gmail.com>
> Subject: Re: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition
> 
> "Zhang, Chen" <chen.zhang@intel.com> writes:
> 
> >> -----Original Message-----
> >> From: Qemu-devel <qemu-devel-
> >> bounces+chen.zhang=intel.com@nongnu.org> On Behalf Of Dr. David
> Alan
> >> Gilbert
> >> Sent: Wednesday, March 24, 2021 4:01 AM
> >> To: Zhang, Chen <chen.zhang@intel.com>
> >> Cc: Lukas Straub <lukasstraub2@web.de>; Li Zhijian
> >> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; qemu-
> >> dev <qemu-devel@nongnu.org>; Markus Armbruster
> <armbru@redhat.com>;
> >> Zhang Chen <zhangckid@gmail.com>
> >> Subject: Re: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition
> >>
> >> * Zhang Chen (chen.zhang@intel.com) wrote:
> >> > Add IP_PROTOCOL as enum include TCP,UDP, ICMP... for other QMP
> >> commands.
> >> >
> >> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> >> > ---
> >> >  qapi/net.json | 31 +++++++++++++++++++++++++++++++
> >> >  1 file changed, 31 insertions(+)
> >> >
> >> > diff --git a/qapi/net.json b/qapi/net.json index
> >> > 87361ebd9a..498ea7aa72 100644
> >> > --- a/qapi/net.json
> >> > +++ b/qapi/net.json
> >> > @@ -794,3 +794,34 @@
> >> >  #
> >> >  ##
> >> >  { 'command': 'query-netdev', 'returns': ['NetdevInfo'] }
> >> > +
> >> > +##
> >> > +# @IP_PROTOCOL:
> >> > +#
> >> > +# Transport layer protocol.
> >> > +#
> >> > +# Just for IPv4.
> >> > +#
> >> > +# @tcp: Transmission Control Protocol.
> >> > +#
> >> > +# @udp: User Datagram Protocol.
> >> > +#
> >> > +# @dccp: Datagram Congestion Control Protocol.
> >> > +#
> >> > +# @sctp: Stream Control Transmission Protocol.
> >> > +#
> >> > +# @udplite: Lightweight User Datagram Protocol.
> >> > +#
> >> > +# @icmp: Internet Control Message Protocol.
> >> > +#
> >> > +# @igmp: Internet Group Management Protocol.
> >> > +#
> >> > +# @ipv6: IPv6 Encapsulation.
> >> > +#
> >> > +# TODO: Need to add more transport layer protocol.
> >> > +#
> >> > +# Since: 6.1
> >> > +##
> >> > +{ 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite',
> >> > +    'icmp', 'igmp', 'ipv6' ] }
> >>
> >> Isn't the right thing to do here to use a string for protocol and
> >> then pass it to getprotobyname;  that way your list is never out of
> >> date, and you never have to translate between the order of this enum
> >> and the integer assignment set in stone.
> >>
> >
> > Hi Dave,
> >
> > Considering that most of the scenes in Qemu don't call the
> getprotobyname, looks keep the string are more readable.
> 
> Unless I'm missing something,
> 
> (1) this enum is only used for matching packets by source IP, source port,
> destination IP, destination port, and protocol, and
> 
> (2) the packet matching works just fine for *any* protocol.
> 
> By using an enum for the protocol, you're limiting the matcher to whatever
> protocols we bother to include in the enum for no particular reason.  Feels
> wrong to me.

Should we remove the IP_PROTOCOL enum here? Make user input string protocol name for this field?
Or any other detailed comments here?

Thanks
Chen

> 
> > Please review the V5 patches, Thanks.
> >
> > Thanks
> > Chen
> >
> >> Dave
> >>
> >> > +
> >> > --
> >> > 2.25.1
> >> >
> >> --
> >> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >>

Re: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition

[Markus Armbruster]

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

>> -----Original Message-----
>> From: Markus Armbruster <armbru@redhat.com>
>> Sent: Thursday, April 15, 2021 11:15 PM
>> To: Zhang, Chen <chen.zhang@intel.com>
>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>; Lukas Straub
>> <lukasstraub2@web.de>; Li Zhijian <lizhijian@cn.fujitsu.com>; Jason Wang
>> <jasowang@redhat.com>; qemu-dev <qemu-devel@nongnu.org>; Zhang
>> Chen <zhangckid@gmail.com>
>> Subject: Re: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition
>> 
>> "Zhang, Chen" <chen.zhang@intel.com> writes:
>> 
>> >> -----Original Message-----
>> >> From: Qemu-devel <qemu-devel-
>> >> bounces+chen.zhang=intel.com@nongnu.org> On Behalf Of Dr. David
>> Alan
>> >> Gilbert
>> >> Sent: Wednesday, March 24, 2021 4:01 AM
>> >> To: Zhang, Chen <chen.zhang@intel.com>
>> >> Cc: Lukas Straub <lukasstraub2@web.de>; Li Zhijian
>> >> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; qemu-
>> >> dev <qemu-devel@nongnu.org>; Markus Armbruster
>> <armbru@redhat.com>;
>> >> Zhang Chen <zhangckid@gmail.com>
>> >> Subject: Re: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition
>> >>
>> >> * Zhang Chen (chen.zhang@intel.com) wrote:
>> >> > Add IP_PROTOCOL as enum include TCP,UDP, ICMP... for other QMP
>> >> commands.
>> >> >
>> >> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
>> >> > ---
>> >> >  qapi/net.json | 31 +++++++++++++++++++++++++++++++
>> >> >  1 file changed, 31 insertions(+)
>> >> >
>> >> > diff --git a/qapi/net.json b/qapi/net.json index
>> >> > 87361ebd9a..498ea7aa72 100644
>> >> > --- a/qapi/net.json
>> >> > +++ b/qapi/net.json
>> >> > @@ -794,3 +794,34 @@
>> >> >  #
>> >> >  ##
>> >> >  { 'command': 'query-netdev', 'returns': ['NetdevInfo'] }
>> >> > +
>> >> > +##
>> >> > +# @IP_PROTOCOL:
>> >> > +#
>> >> > +# Transport layer protocol.
>> >> > +#
>> >> > +# Just for IPv4.
>> >> > +#
>> >> > +# @tcp: Transmission Control Protocol.
>> >> > +#
>> >> > +# @udp: User Datagram Protocol.
>> >> > +#
>> >> > +# @dccp: Datagram Congestion Control Protocol.
>> >> > +#
>> >> > +# @sctp: Stream Control Transmission Protocol.
>> >> > +#
>> >> > +# @udplite: Lightweight User Datagram Protocol.
>> >> > +#
>> >> > +# @icmp: Internet Control Message Protocol.
>> >> > +#
>> >> > +# @igmp: Internet Group Management Protocol.
>> >> > +#
>> >> > +# @ipv6: IPv6 Encapsulation.
>> >> > +#
>> >> > +# TODO: Need to add more transport layer protocol.
>> >> > +#
>> >> > +# Since: 6.1
>> >> > +##
>> >> > +{ 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite',
>> >> > +    'icmp', 'igmp', 'ipv6' ] }
>> >>
>> >> Isn't the right thing to do here to use a string for protocol and
>> >> then pass it to getprotobyname;  that way your list is never out of
>> >> date, and you never have to translate between the order of this enum
>> >> and the integer assignment set in stone.
>> >>
>> >
>> > Hi Dave,
>> >
>> > Considering that most of the scenes in Qemu don't call the
>> getprotobyname, looks keep the string are more readable.
>> 
>> Unless I'm missing something,
>> 
>> (1) this enum is only used for matching packets by source IP, source port,
>> destination IP, destination port, and protocol, and
>> 
>> (2) the packet matching works just fine for *any* protocol.
>> 
>> By using an enum for the protocol, you're limiting the matcher to whatever
>> protocols we bother to include in the enum for no particular reason.  Feels
>> wrong to me.
>
> Should we remove the IP_PROTOCOL enum here? Make user input string protocol name for this field?
> Or any other detailed comments here?

I believe that's Dave's point.  I.e.:

    { 'struct': 'L4_Connection',
      'data': { 'protocol': 'str', ... }

If we ever need to specify protocols by number in addition to name, we
can compatibly evolve the 'str' into an alternation of 'str' and
'uint8'.  Unlikely.

Re: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> "Zhang, Chen" <chen.zhang@intel.com> writes:
> 
> >> -----Original Message-----
> >> From: Markus Armbruster <armbru@redhat.com>
> >> Sent: Thursday, April 15, 2021 11:15 PM
> >> To: Zhang, Chen <chen.zhang@intel.com>
> >> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>; Lukas Straub
> >> <lukasstraub2@web.de>; Li Zhijian <lizhijian@cn.fujitsu.com>; Jason Wang
> >> <jasowang@redhat.com>; qemu-dev <qemu-devel@nongnu.org>; Zhang
> >> Chen <zhangckid@gmail.com>
> >> Subject: Re: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition
> >> 
> >> "Zhang, Chen" <chen.zhang@intel.com> writes:
> >> 
> >> >> -----Original Message-----
> >> >> From: Qemu-devel <qemu-devel-
> >> >> bounces+chen.zhang=intel.com@nongnu.org> On Behalf Of Dr. David
> >> Alan
> >> >> Gilbert
> >> >> Sent: Wednesday, March 24, 2021 4:01 AM
> >> >> To: Zhang, Chen <chen.zhang@intel.com>
> >> >> Cc: Lukas Straub <lukasstraub2@web.de>; Li Zhijian
> >> >> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; qemu-
> >> >> dev <qemu-devel@nongnu.org>; Markus Armbruster
> >> <armbru@redhat.com>;
> >> >> Zhang Chen <zhangckid@gmail.com>
> >> >> Subject: Re: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition
> >> >>
> >> >> * Zhang Chen (chen.zhang@intel.com) wrote:
> >> >> > Add IP_PROTOCOL as enum include TCP,UDP, ICMP... for other QMP
> >> >> commands.
> >> >> >
> >> >> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> >> >> > ---
> >> >> >  qapi/net.json | 31 +++++++++++++++++++++++++++++++
> >> >> >  1 file changed, 31 insertions(+)
> >> >> >
> >> >> > diff --git a/qapi/net.json b/qapi/net.json index
> >> >> > 87361ebd9a..498ea7aa72 100644
> >> >> > --- a/qapi/net.json
> >> >> > +++ b/qapi/net.json
> >> >> > @@ -794,3 +794,34 @@
> >> >> >  #
> >> >> >  ##
> >> >> >  { 'command': 'query-netdev', 'returns': ['NetdevInfo'] }
> >> >> > +
> >> >> > +##
> >> >> > +# @IP_PROTOCOL:
> >> >> > +#
> >> >> > +# Transport layer protocol.
> >> >> > +#
> >> >> > +# Just for IPv4.
> >> >> > +#
> >> >> > +# @tcp: Transmission Control Protocol.
> >> >> > +#
> >> >> > +# @udp: User Datagram Protocol.
> >> >> > +#
> >> >> > +# @dccp: Datagram Congestion Control Protocol.
> >> >> > +#
> >> >> > +# @sctp: Stream Control Transmission Protocol.
> >> >> > +#
> >> >> > +# @udplite: Lightweight User Datagram Protocol.
> >> >> > +#
> >> >> > +# @icmp: Internet Control Message Protocol.
> >> >> > +#
> >> >> > +# @igmp: Internet Group Management Protocol.
> >> >> > +#
> >> >> > +# @ipv6: IPv6 Encapsulation.
> >> >> > +#
> >> >> > +# TODO: Need to add more transport layer protocol.
> >> >> > +#
> >> >> > +# Since: 6.1
> >> >> > +##
> >> >> > +{ 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite',
> >> >> > +    'icmp', 'igmp', 'ipv6' ] }
> >> >>
> >> >> Isn't the right thing to do here to use a string for protocol and
> >> >> then pass it to getprotobyname;  that way your list is never out of
> >> >> date, and you never have to translate between the order of this enum
> >> >> and the integer assignment set in stone.
> >> >>
> >> >
> >> > Hi Dave,
> >> >
> >> > Considering that most of the scenes in Qemu don't call the
> >> getprotobyname, looks keep the string are more readable.
> >> 
> >> Unless I'm missing something,
> >> 
> >> (1) this enum is only used for matching packets by source IP, source port,
> >> destination IP, destination port, and protocol, and
> >> 
> >> (2) the packet matching works just fine for *any* protocol.
> >> 
> >> By using an enum for the protocol, you're limiting the matcher to whatever
> >> protocols we bother to include in the enum for no particular reason.  Feels
> >> wrong to me.
> >
> > Should we remove the IP_PROTOCOL enum here? Make user input string protocol name for this field?
> > Or any other detailed comments here?
> 
> I believe that's Dave's point.  I.e.:
> 
>     { 'struct': 'L4_Connection',
>       'data': { 'protocol': 'str', ... }
> 
> If we ever need to specify protocols by number in addition to name, we
> can compatibly evolve the 'str' into an alternation of 'str' and
> 'uint8'.  Unlikely.

Right; just using a string and sending it via getprotobyname() actually
seems easier than using the enum and having all the conversions.

Dave

-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

RE: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition

[Zhang, Chen]

> -----Original Message-----
> From: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Sent: Tuesday, April 20, 2021 7:06 PM
> To: Markus Armbruster <armbru@redhat.com>
> Cc: Zhang, Chen <chen.zhang@intel.com>; Lukas Straub
> <lukasstraub2@web.de>; Li Zhijian <lizhijian@cn.fujitsu.com>; Jason Wang
> <jasowang@redhat.com>; qemu-dev <qemu-devel@nongnu.org>; Zhang
> Chen <zhangckid@gmail.com>
> Subject: Re: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition
> 
> * Markus Armbruster (armbru@redhat.com) wrote:
> > "Zhang, Chen" <chen.zhang@intel.com> writes:
> >
> > >> -----Original Message-----
> > >> From: Markus Armbruster <armbru@redhat.com>
> > >> Sent: Thursday, April 15, 2021 11:15 PM
> > >> To: Zhang, Chen <chen.zhang@intel.com>
> > >> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>; Lukas Straub
> > >> <lukasstraub2@web.de>; Li Zhijian <lizhijian@cn.fujitsu.com>; Jason
> > >> Wang <jasowang@redhat.com>; qemu-dev <qemu-
> devel@nongnu.org>; Zhang
> > >> Chen <zhangckid@gmail.com>
> > >> Subject: Re: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL
> > >> definition
> > >>
> > >> "Zhang, Chen" <chen.zhang@intel.com> writes:
> > >>
> > >> >> -----Original Message-----
> > >> >> From: Qemu-devel <qemu-devel-
> > >> >> bounces+chen.zhang=intel.com@nongnu.org> On Behalf Of Dr.
> David
> > >> Alan
> > >> >> Gilbert
> > >> >> Sent: Wednesday, March 24, 2021 4:01 AM
> > >> >> To: Zhang, Chen <chen.zhang@intel.com>
> > >> >> Cc: Lukas Straub <lukasstraub2@web.de>; Li Zhijian
> > >> >> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>;
> > >> >> qemu- dev <qemu-devel@nongnu.org>; Markus Armbruster
> > >> <armbru@redhat.com>;
> > >> >> Zhang Chen <zhangckid@gmail.com>
> > >> >> Subject: Re: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL
> > >> >> definition
> > >> >>
> > >> >> * Zhang Chen (chen.zhang@intel.com) wrote:
> > >> >> > Add IP_PROTOCOL as enum include TCP,UDP, ICMP... for other
> QMP
> > >> >> commands.
> > >> >> >
> > >> >> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> > >> >> > ---
> > >> >> >  qapi/net.json | 31 +++++++++++++++++++++++++++++++
> > >> >> >  1 file changed, 31 insertions(+)
> > >> >> >
> > >> >> > diff --git a/qapi/net.json b/qapi/net.json index
> > >> >> > 87361ebd9a..498ea7aa72 100644
> > >> >> > --- a/qapi/net.json
> > >> >> > +++ b/qapi/net.json
> > >> >> > @@ -794,3 +794,34 @@
> > >> >> >  #
> > >> >> >  ##
> > >> >> >  { 'command': 'query-netdev', 'returns': ['NetdevInfo'] }
> > >> >> > +
> > >> >> > +##
> > >> >> > +# @IP_PROTOCOL:
> > >> >> > +#
> > >> >> > +# Transport layer protocol.
> > >> >> > +#
> > >> >> > +# Just for IPv4.
> > >> >> > +#
> > >> >> > +# @tcp: Transmission Control Protocol.
> > >> >> > +#
> > >> >> > +# @udp: User Datagram Protocol.
> > >> >> > +#
> > >> >> > +# @dccp: Datagram Congestion Control Protocol.
> > >> >> > +#
> > >> >> > +# @sctp: Stream Control Transmission Protocol.
> > >> >> > +#
> > >> >> > +# @udplite: Lightweight User Datagram Protocol.
> > >> >> > +#
> > >> >> > +# @icmp: Internet Control Message Protocol.
> > >> >> > +#
> > >> >> > +# @igmp: Internet Group Management Protocol.
> > >> >> > +#
> > >> >> > +# @ipv6: IPv6 Encapsulation.
> > >> >> > +#
> > >> >> > +# TODO: Need to add more transport layer protocol.
> > >> >> > +#
> > >> >> > +# Since: 6.1
> > >> >> > +##
> > >> >> > +{ 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite',
> > >> >> > +    'icmp', 'igmp', 'ipv6' ] }
> > >> >>
> > >> >> Isn't the right thing to do here to use a string for protocol
> > >> >> and then pass it to getprotobyname;  that way your list is never
> > >> >> out of date, and you never have to translate between the order
> > >> >> of this enum and the integer assignment set in stone.
> > >> >>
> > >> >
> > >> > Hi Dave,
> > >> >
> > >> > Considering that most of the scenes in Qemu don't call the
> > >> getprotobyname, looks keep the string are more readable.
> > >>
> > >> Unless I'm missing something,
> > >>
> > >> (1) this enum is only used for matching packets by source IP,
> > >> source port, destination IP, destination port, and protocol, and
> > >>
> > >> (2) the packet matching works just fine for *any* protocol.
> > >>
> > >> By using an enum for the protocol, you're limiting the matcher to
> > >> whatever protocols we bother to include in the enum for no
> > >> particular reason.  Feels wrong to me.
> > >
> > > Should we remove the IP_PROTOCOL enum here? Make user input string
> protocol name for this field?
> > > Or any other detailed comments here?
> >
> > I believe that's Dave's point.  I.e.:
> >
> >     { 'struct': 'L4_Connection',
> >       'data': { 'protocol': 'str', ... }
> >
> > If we ever need to specify protocols by number in addition to name, we
> > can compatibly evolve the 'str' into an alternation of 'str' and
> > 'uint8'.  Unlikely.
> 
> Right; just using a string and sending it via getprotobyname() actually seems
> easier than using the enum and having all the conversions.

OK, Thanks for clear it.  I already fixed it.
Please review the V6 of this series.

Thanks
Chen

> 
> Dave
> 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK