[PATCH V4 0/3] net/filter: Optimize filters vnet_hdr support

[PATCH V4 0/3] net/filter: Optimize filters vnet_hdr support

[Zhang Chen]

This series make filters and colo-compare module support vnet_hdr by
default. And also support -device non-virtio-net(like e1000.) at the same time.
It can adapt -device automatically to avoid wrong setting between
different filters when enable/disable virtio-net-pci. So no need to keep the
"vnet_hdr_support" flag in filter's property. 

Optimize the filter transfer protocol from:
1.size -----> 2.real network payload.
to:
1.size -----> 2.vnet_hdr_len. -----> 3.real network payload.

When receiving node get the network packet, it will compare with
the local vnet_hdr_len. If they are not the same, report a error.
because this kind of packet cannot be correctly parsed by receiving
node. For the colo-compare, it need to compare whether the two sides
vnet_hdr_len are equal.


v4:
    Rewrite patches to impliment it in filter transfer protocol payload.
    Remove filters and colo-compare's "vnet_hdr_support" flag.

v3:
    Fix some typos.
    Rebased for Qemu 6.2.

v2:
    Detect virtio-net driver and apply vnet_hdr_support
    automatically. (Jason)

Zhang Chen (3):
  net/filter: Remove vnet_hdr from filter-mirror and filter-redirector
  net/filter: Remove vnet_hdr from filter-rewriter
  net/colo-compare.c: Remove vnet_hdr and check in payload from
    colo-compare

 net/colo-compare.c    | 41 +++++++-------------
 net/filter-mirror.c   | 88 ++++++++++---------------------------------
 net/filter-rewriter.c | 26 +------------
 qemu-options.hx       | 25 ++++++------
 4 files changed, 45 insertions(+), 135 deletions(-)

-- 
2.25.1

[PATCH V4 1/3] net/filter: Remove vnet_hdr from filter-mirror and filter-redirector

[Zhang Chen]

Make the vnet header a necessary part of filter transfer protocol.
So remove the module's vnet_hdr_support switch here.
It make other modules(like another filter-redirector,colo-compare...)
know how to parse net packet correctly. If local device is not the
virtio-net-pci, vnet_hdr_len will be 0.

Signed-off-by: Zhang Chen <chen.zhang@intel.com>
---
 net/filter-mirror.c | 88 ++++++++++-----------------------------------
 qemu-options.hx     | 14 ++++----
 2 files changed, 25 insertions(+), 77 deletions(-)

diff --git a/net/filter-mirror.c b/net/filter-mirror.c
index f20240cc9f..4f0e26cc92 100644
--- a/net/filter-mirror.c
+++ b/net/filter-mirror.c
@@ -39,7 +39,6 @@ struct MirrorState {
     CharBackend chr_in;
     CharBackend chr_out;
     SocketReadState rs;
-    bool vnet_hdr;
 };
 
 static int filter_send(MirrorState *s,
@@ -48,7 +47,7 @@ static int filter_send(MirrorState *s,
 {
     NetFilterState *nf = NETFILTER(s);
     int ret = 0;
-    ssize_t size = 0;
+    ssize_t size = 0, vnet_hdr_len = 0;
     uint32_t len = 0;
     char *buf;
 
@@ -63,21 +62,18 @@ static int filter_send(MirrorState *s,
         goto err;
     }
 
-    if (s->vnet_hdr) {
-        /*
-         * If vnet_hdr = on, we send vnet header len to make other
-         * module(like colo-compare) know how to parse net
-         * packet correctly.
-         */
-        ssize_t vnet_hdr_len;
-
-        vnet_hdr_len = nf->netdev->vnet_hdr_len;
+    /*
+     * The vnet header is the necessary part of filter transfer protocol.
+     * It make other module(like colo-compare) know how to parse net
+     * packet correctly. If device is not the virtio-net-pci,
+     * vnet_hdr_len will be 0.
+     */
+    vnet_hdr_len = nf->netdev->vnet_hdr_len;
 
-        len = htonl(vnet_hdr_len);
-        ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)&len, sizeof(len));
-        if (ret != sizeof(len)) {
-            goto err;
-        }
+    len = htonl(vnet_hdr_len);
+    ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)&len, sizeof(len));
+    if (ret != sizeof(len)) {
+        goto err;
     }
 
     buf = g_malloc(size);
@@ -232,6 +228,12 @@ static void redirector_rs_finalize(SocketReadState *rs)
     MirrorState *s = container_of(rs, MirrorState, rs);
     NetFilterState *nf = NETFILTER(s);
 
+    /* Check remote vnet_hdr */
+    if (rs->vnet_hdr_len != nf->netdev->vnet_hdr_len) {
+        error_report("filter redirector got a different packet vnet_hdr"
+        " from local, please check the -device configuration");
+    }
+
     redirector_to_filter(nf, rs->buf, rs->packet_len);
 }
 
@@ -252,7 +254,7 @@ static void filter_redirector_setup(NetFilterState *nf, Error **errp)
         }
     }
 
-    net_socket_rs_init(&s->rs, redirector_rs_finalize, s->vnet_hdr);
+    net_socket_rs_init(&s->rs, redirector_rs_finalize, true);
 
     if (s->indev) {
         chr = qemu_chr_find(s->indev);
@@ -323,20 +325,6 @@ static void filter_mirror_set_outdev(Object *obj,
     }
 }
 
-static bool filter_mirror_get_vnet_hdr(Object *obj, Error **errp)
-{
-    MirrorState *s = FILTER_MIRROR(obj);
-
-    return s->vnet_hdr;
-}
-
-static void filter_mirror_set_vnet_hdr(Object *obj, bool value, Error **errp)
-{
-    MirrorState *s = FILTER_MIRROR(obj);
-
-    s->vnet_hdr = value;
-}
-
 static char *filter_redirector_get_outdev(Object *obj, Error **errp)
 {
     MirrorState *s = FILTER_REDIRECTOR(obj);
@@ -354,31 +342,12 @@ static void filter_redirector_set_outdev(Object *obj,
     s->outdev = g_strdup(value);
 }
 
-static bool filter_redirector_get_vnet_hdr(Object *obj, Error **errp)
-{
-    MirrorState *s = FILTER_REDIRECTOR(obj);
-
-    return s->vnet_hdr;
-}
-
-static void filter_redirector_set_vnet_hdr(Object *obj,
-                                           bool value,
-                                           Error **errp)
-{
-    MirrorState *s = FILTER_REDIRECTOR(obj);
-
-    s->vnet_hdr = value;
-}
-
 static void filter_mirror_class_init(ObjectClass *oc, void *data)
 {
     NetFilterClass *nfc = NETFILTER_CLASS(oc);
 
     object_class_property_add_str(oc, "outdev", filter_mirror_get_outdev,
                                   filter_mirror_set_outdev);
-    object_class_property_add_bool(oc, "vnet_hdr_support",
-                                   filter_mirror_get_vnet_hdr,
-                                   filter_mirror_set_vnet_hdr);
 
     nfc->setup = filter_mirror_setup;
     nfc->cleanup = filter_mirror_cleanup;
@@ -393,29 +362,12 @@ static void filter_redirector_class_init(ObjectClass *oc, void *data)
                                   filter_redirector_set_indev);
     object_class_property_add_str(oc, "outdev", filter_redirector_get_outdev,
                                   filter_redirector_set_outdev);
-    object_class_property_add_bool(oc, "vnet_hdr_support",
-                                   filter_redirector_get_vnet_hdr,
-                                   filter_redirector_set_vnet_hdr);
 
     nfc->setup = filter_redirector_setup;
     nfc->cleanup = filter_redirector_cleanup;
     nfc->receive_iov = filter_redirector_receive_iov;
 }
 
-static void filter_mirror_init(Object *obj)
-{
-    MirrorState *s = FILTER_MIRROR(obj);
-
-    s->vnet_hdr = false;
-}
-
-static void filter_redirector_init(Object *obj)
-{
-    MirrorState *s = FILTER_REDIRECTOR(obj);
-
-    s->vnet_hdr = false;
-}
-
 static void filter_mirror_fini(Object *obj)
 {
     MirrorState *s = FILTER_MIRROR(obj);
@@ -435,7 +387,6 @@ static const TypeInfo filter_redirector_info = {
     .name = TYPE_FILTER_REDIRECTOR,
     .parent = TYPE_NETFILTER,
     .class_init = filter_redirector_class_init,
-    .instance_init = filter_redirector_init,
     .instance_finalize = filter_redirector_fini,
     .instance_size = sizeof(MirrorState),
 };
@@ -444,7 +395,6 @@ static const TypeInfo filter_mirror_info = {
     .name = TYPE_FILTER_MIRROR,
     .parent = TYPE_NETFILTER,
     .class_init = filter_mirror_class_init,
-    .instance_init = filter_mirror_init,
     .instance_finalize = filter_mirror_fini,
     .instance_size = sizeof(MirrorState),
 };
diff --git a/qemu-options.hx b/qemu-options.hx
index 5f375bbfa6..38c03812a7 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4946,18 +4946,16 @@ SRST
 
         ``behind``: insert behind the specified filter (default).
 
-    ``-object filter-mirror,id=id,netdev=netdevid,outdev=chardevid,queue=all|rx|tx[,vnet_hdr_support][,position=head|tail|id=<id>][,insert=behind|before]``
+    ``-object filter-mirror,id=id,netdev=netdevid,outdev=chardevid,queue=all|rx|tx[,position=head|tail|id=<id>][,insert=behind|before]``
         filter-mirror on netdev netdevid,mirror net packet to
-        chardevchardevid, if it has the vnet\_hdr\_support flag,
-        filter-mirror will mirror packet with vnet\_hdr\_len.
+        chardev chardevid, filter-mirror will mirror packet with vnet\_hdr\_len.
 
-    ``-object filter-redirector,id=id,netdev=netdevid,indev=chardevid,outdev=chardevid,queue=all|rx|tx[,vnet_hdr_support][,position=head|tail|id=<id>][,insert=behind|before]``
+    ``-object filter-redirector,id=id,netdev=netdevid,indev=chardevid,outdev=chardevid,queue=all|rx|tx[,position=head|tail|id=<id>][,insert=behind|before]``
         filter-redirector on netdev netdevid,redirect filter's net
         packet to chardev chardevid,and redirect indev's packet to
-        filter.if it has the vnet\_hdr\_support flag, filter-redirector
-        will redirect packet with vnet\_hdr\_len. Create a
-        filter-redirector we need to differ outdev id from indev id, id
-        can not be the same. we can just use indev or outdev, but at
+        filter. filter-redirector will redirect packet with vnet\_hdr\_len.
+        Create a filter-redirector we need to differ outdev id from indev id,
+        id can not be the same. we can just use indev or outdev, but at
         least one of indev or outdev need to be specified.
 
     ``-object filter-rewriter,id=id,netdev=netdevid,queue=all|rx|tx,[vnet_hdr_support][,position=head|tail|id=<id>][,insert=behind|before]``
-- 
2.25.1

[PATCH V4 2/3] net/filter: Remove vnet_hdr from filter-rewriter

[Zhang Chen]

Make the vnet header a necessary part of filter transfer protocol.
So we need remove the module switch here.

Signed-off-by: Zhang Chen <chen.zhang@intel.com>
---
 net/filter-rewriter.c | 26 +-------------------------
 qemu-options.hx       |  6 +++---
 2 files changed, 4 insertions(+), 28 deletions(-)

diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c
index cb3a96cde1..acc09f20fa 100644
--- a/net/filter-rewriter.c
+++ b/net/filter-rewriter.c
@@ -34,7 +34,6 @@ struct RewriterState {
     NetQueue *incoming_queue;
     /* hashtable to save connection */
     GHashTable *connection_track_table;
-    bool vnet_hdr;
     bool failover_mode;
 };
 
@@ -266,9 +265,7 @@ static ssize_t colo_rewriter_receive_iov(NetFilterState *nf,
 
     iov_to_buf(iov, iovcnt, 0, buf, size);
 
-    if (s->vnet_hdr) {
-        vnet_hdr_len = nf->netdev->vnet_hdr_len;
-    }
+    vnet_hdr_len = nf->netdev->vnet_hdr_len;
 
     pkt = packet_new_nocopy(buf, size, vnet_hdr_len);
 
@@ -395,27 +392,10 @@ static void colo_rewriter_setup(NetFilterState *nf, Error **errp)
     s->incoming_queue = qemu_new_net_queue(qemu_netfilter_pass_to_next, nf);
 }
 
-static bool filter_rewriter_get_vnet_hdr(Object *obj, Error **errp)
-{
-    RewriterState *s = FILTER_REWRITER(obj);
-
-    return s->vnet_hdr;
-}
-
-static void filter_rewriter_set_vnet_hdr(Object *obj,
-                                         bool value,
-                                         Error **errp)
-{
-    RewriterState *s = FILTER_REWRITER(obj);
-
-    s->vnet_hdr = value;
-}
-
 static void filter_rewriter_init(Object *obj)
 {
     RewriterState *s = FILTER_REWRITER(obj);
 
-    s->vnet_hdr = false;
     s->failover_mode = FAILOVER_MODE_OFF;
 }
 
@@ -423,10 +403,6 @@ static void colo_rewriter_class_init(ObjectClass *oc, void *data)
 {
     NetFilterClass *nfc = NETFILTER_CLASS(oc);
 
-    object_class_property_add_bool(oc, "vnet_hdr_support",
-                                   filter_rewriter_get_vnet_hdr,
-                                   filter_rewriter_set_vnet_hdr);
-
     nfc->setup = colo_rewriter_setup;
     nfc->cleanup = colo_rewriter_cleanup;
     nfc->receive_iov = colo_rewriter_receive_iov;
diff --git a/qemu-options.hx b/qemu-options.hx
index 38c03812a7..6d3b7ab8a0 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4958,12 +4958,12 @@ SRST
         id can not be the same. we can just use indev or outdev, but at
         least one of indev or outdev need to be specified.
 
-    ``-object filter-rewriter,id=id,netdev=netdevid,queue=all|rx|tx,[vnet_hdr_support][,position=head|tail|id=<id>][,insert=behind|before]``
+    ``-object filter-rewriter,id=id,netdev=netdevid,queue=all|rx|tx[,position=head|tail|id=<id>][,insert=behind|before]``
         Filter-rewriter is a part of COLO project.It will rewrite tcp
         packet to secondary from primary to keep secondary tcp
         connection,and rewrite tcp packet to primary from secondary make
-        tcp packet can be handled by client.if it has the
-        vnet\_hdr\_support flag, we can parse packet with vnet header.
+        tcp packet can be handled by client. Filter-rewriter support
+        parse packet with vnet header.
 
         usage: colo secondary: -object
         filter-redirector,id=f1,netdev=hn0,queue=tx,indev=red0 -object
-- 
2.25.1

[PATCH V4 3/3] net/colo-compare.c: Remove vnet_hdr and check in payload from colo-compare

[Zhang Chen]

Enable vnet_hdr in payload by default. Make it easier to find module
communication configuration errors.

Signed-off-by: Zhang Chen <chen.zhang@intel.com>
---
 net/colo-compare.c | 41 ++++++++++++++---------------------------
 qemu-options.hx    |  5 ++---
 2 files changed, 16 insertions(+), 30 deletions(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index b100e7b51f..ecb21917c6 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -119,7 +119,7 @@ struct CompareState {
     SocketReadState notify_rs;
     SendCo out_sendco;
     SendCo notify_sendco;
-    bool vnet_hdr;
+    int local_vnet_hdr_len;
     uint64_t compare_timeout;
     uint32_t expired_scan_cycle;
 
@@ -725,7 +725,6 @@ static void colo_compare_connection(void *opaque, void *user_data)
 static void coroutine_fn _compare_chr_send(void *opaque)
 {
     SendCo *sendco = opaque;
-    CompareState *s = sendco->s;
     int ret = 0;
 
     while (!g_queue_is_empty(&sendco->send_list)) {
@@ -740,7 +739,7 @@ static void coroutine_fn _compare_chr_send(void *opaque)
             goto err;
         }
 
-        if (!sendco->notify_remote_frame && s->vnet_hdr) {
+        if (!sendco->notify_remote_frame) {
             /*
              * We send vnet header len make other module(like filter-redirector)
              * know how to parse net packet correctly.
@@ -1034,22 +1033,6 @@ static void compare_set_outdev(Object *obj, const char *value, Error **errp)
     s->outdev = g_strdup(value);
 }
 
-static bool compare_get_vnet_hdr(Object *obj, Error **errp)
-{
-    CompareState *s = COLO_COMPARE(obj);
-
-    return s->vnet_hdr;
-}
-
-static void compare_set_vnet_hdr(Object *obj,
-                                 bool value,
-                                 Error **errp)
-{
-    CompareState *s = COLO_COMPARE(obj);
-
-    s->vnet_hdr = value;
-}
-
 static char *compare_get_notify_dev(Object *obj, Error **errp)
 {
     CompareState *s = COLO_COMPARE(obj);
@@ -1157,6 +1140,9 @@ static void compare_pri_rs_finalize(SocketReadState *pri_rs)
     CompareState *s = container_of(pri_rs, CompareState, pri_rs);
     Connection *conn = NULL;
 
+    /* Update colo-compare local vnet_hdr_len */
+    s->local_vnet_hdr_len = pri_rs->vnet_hdr_len;
+
     if (packet_enqueue(s, PRIMARY_IN, &conn)) {
         trace_colo_compare_main("primary: unsupported packet in");
         compare_chr_send(s,
@@ -1176,6 +1162,12 @@ static void compare_sec_rs_finalize(SocketReadState *sec_rs)
     CompareState *s = container_of(sec_rs, CompareState, sec_rs);
     Connection *conn = NULL;
 
+    /* Check the secondary vnet_hdr_len */
+    if (s->local_vnet_hdr_len != sec_rs->vnet_hdr_len) {
+        error_report("colo-compare got a different packet vnet_hdr_len"
+        " from local, please check the nodes -device configuration");
+    }
+
     if (packet_enqueue(s, SECONDARY_IN, &conn)) {
         trace_colo_compare_main("secondary: unsupported packet in");
     } else {
@@ -1289,8 +1281,8 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
         return;
     }
 
-    net_socket_rs_init(&s->pri_rs, compare_pri_rs_finalize, s->vnet_hdr);
-    net_socket_rs_init(&s->sec_rs, compare_sec_rs_finalize, s->vnet_hdr);
+    net_socket_rs_init(&s->pri_rs, compare_pri_rs_finalize, true);
+    net_socket_rs_init(&s->sec_rs, compare_sec_rs_finalize, true);
 
     /* Try to enable remote notify chardev, currently just for Xen COLO */
     if (s->notify_dev) {
@@ -1299,8 +1291,7 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
             return;
         }
 
-        net_socket_rs_init(&s->notify_rs, compare_notify_rs_finalize,
-                           s->vnet_hdr);
+        net_socket_rs_init(&s->notify_rs, compare_notify_rs_finalize, false);
     }
 
     s->out_sendco.s = s;
@@ -1396,10 +1387,6 @@ static void colo_compare_init(Object *obj)
     object_property_add(obj, "max_queue_size", "uint32",
                         get_max_queue_size,
                         set_max_queue_size, NULL, NULL);
-
-    s->vnet_hdr = false;
-    object_property_add_bool(obj, "vnet_hdr_support", compare_get_vnet_hdr,
-                             compare_set_vnet_hdr);
 }
 
 void colo_compare_cleanup(void)
diff --git a/qemu-options.hx b/qemu-options.hx
index 6d3b7ab8a0..c966035b4b 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4976,15 +4976,14 @@ SRST
         stored. The file format is libpcap, so it can be analyzed with
         tools such as tcpdump or Wireshark.
 
-    ``-object colo-compare,id=id,primary_in=chardevid,secondary_in=chardevid,outdev=chardevid,iothread=id[,vnet_hdr_support][,notify_dev=id][,compare_timeout=@var{ms}][,expired_scan_cycle=@var{ms}][,max_queue_size=@var{size}]``
+    ``-object colo-compare,id=id,primary_in=chardevid,secondary_in=chardevid,outdev=chardevid,iothread=id[,notify_dev=id][,compare_timeout=@var{ms}][,expired_scan_cycle=@var{ms}][,max_queue_size=@var{size}]``
         Colo-compare gets packet from primary\_in chardevid and
         secondary\_in, then compare whether the payload of primary packet
         and secondary packet are the same. If same, it will output
         primary packet to out\_dev, else it will notify COLO-framework to do
         checkpoint and send primary packet to out\_dev. In order to
         improve efficiency, we need to put the task of comparison in
-        another iothread. If it has the vnet\_hdr\_support flag,
-        colo compare will send/recv packet with vnet\_hdr\_len.
+        another iothread. colo compare will send/recv packet with vnet\_hdr\_len.
         The compare\_timeout=@var{ms} determines the maximum time of the
         colo-compare hold the packet. The expired\_scan\_cycle=@var{ms}
         is to set the period of scanning expired primary node network packets.
-- 
2.25.1

Re: [PATCH V4 1/3] net/filter: Remove vnet_hdr from filter-mirror and filter-redirector

[Jason Wang]

在 2021/10/27 上午2:17, Zhang Chen 写道:
> Make the vnet header a necessary part of filter transfer protocol.
> So remove the module's vnet_hdr_support switch here.
> It make other modules(like another filter-redirector,colo-compare...)
> know how to parse net packet correctly. If local device is not the
> virtio-net-pci, vnet_hdr_len will be 0.
>
> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> ---
>   net/filter-mirror.c | 88 ++++++++++-----------------------------------
>   qemu-options.hx     | 14 ++++----
>   2 files changed, 25 insertions(+), 77 deletions(-)
>
> diff --git a/net/filter-mirror.c b/net/filter-mirror.c
> index f20240cc9f..4f0e26cc92 100644
> --- a/net/filter-mirror.c
> +++ b/net/filter-mirror.c
> @@ -39,7 +39,6 @@ struct MirrorState {
>       CharBackend chr_in;
>       CharBackend chr_out;
>       SocketReadState rs;
> -    bool vnet_hdr;
>   };
>   
>   static int filter_send(MirrorState *s,
> @@ -48,7 +47,7 @@ static int filter_send(MirrorState *s,
>   {
>       NetFilterState *nf = NETFILTER(s);
>       int ret = 0;
> -    ssize_t size = 0;
> +    ssize_t size = 0, vnet_hdr_len = 0;
>       uint32_t len = 0;
>       char *buf;
>   
> @@ -63,21 +62,18 @@ static int filter_send(MirrorState *s,
>           goto err;
>       }
>   
> -    if (s->vnet_hdr) {
> -        /*
> -         * If vnet_hdr = on, we send vnet header len to make other
> -         * module(like colo-compare) know how to parse net
> -         * packet correctly.
> -         */
> -        ssize_t vnet_hdr_len;
> -
> -        vnet_hdr_len = nf->netdev->vnet_hdr_len;
> +    /*
> +     * The vnet header is the necessary part of filter transfer protocol.
> +     * It make other module(like colo-compare) know how to parse net
> +     * packet correctly. If device is not the virtio-net-pci,
> +     * vnet_hdr_len will be 0.
> +     */
> +    vnet_hdr_len = nf->netdev->vnet_hdr_len;
>   
> -        len = htonl(vnet_hdr_len);
> -        ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)&len, sizeof(len));
> -        if (ret != sizeof(len)) {
> -            goto err;
> -        }
> +    len = htonl(vnet_hdr_len);
> +    ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)&len, sizeof(len));
> +    if (ret != sizeof(len)) {
> +        goto err;
>       }
>   
>       buf = g_malloc(size);
> @@ -232,6 +228,12 @@ static void redirector_rs_finalize(SocketReadState *rs)
>       MirrorState *s = container_of(rs, MirrorState, rs);
>       NetFilterState *nf = NETFILTER(s);
>   
> +    /* Check remote vnet_hdr */
> +    if (rs->vnet_hdr_len != nf->netdev->vnet_hdr_len) {
> +        error_report("filter redirector got a different packet vnet_hdr"
> +        " from local, please check the -device configuration");
> +    }
> +
>       redirector_to_filter(nf, rs->buf, rs->packet_len);
>   }
>   
> @@ -252,7 +254,7 @@ static void filter_redirector_setup(NetFilterState *nf, Error **errp)
>           }
>       }
>   
> -    net_socket_rs_init(&s->rs, redirector_rs_finalize, s->vnet_hdr);
> +    net_socket_rs_init(&s->rs, redirector_rs_finalize, true);
>   
>       if (s->indev) {
>           chr = qemu_chr_find(s->indev);
> @@ -323,20 +325,6 @@ static void filter_mirror_set_outdev(Object *obj,
>       }
>   }
>   
> -static bool filter_mirror_get_vnet_hdr(Object *obj, Error **errp)
> -{
> -    MirrorState *s = FILTER_MIRROR(obj);
> -
> -    return s->vnet_hdr;
> -}
> -
> -static void filter_mirror_set_vnet_hdr(Object *obj, bool value, Error **errp)
> -{
> -    MirrorState *s = FILTER_MIRROR(obj);
> -
> -    s->vnet_hdr = value;
> -}
> -
>   static char *filter_redirector_get_outdev(Object *obj, Error **errp)
>   {
>       MirrorState *s = FILTER_REDIRECTOR(obj);
> @@ -354,31 +342,12 @@ static void filter_redirector_set_outdev(Object *obj,
>       s->outdev = g_strdup(value);
>   }
>   
> -static bool filter_redirector_get_vnet_hdr(Object *obj, Error **errp)
> -{
> -    MirrorState *s = FILTER_REDIRECTOR(obj);
> -
> -    return s->vnet_hdr;
> -}
> -
> -static void filter_redirector_set_vnet_hdr(Object *obj,
> -                                           bool value,
> -                                           Error **errp)
> -{
> -    MirrorState *s = FILTER_REDIRECTOR(obj);
> -
> -    s->vnet_hdr = value;
> -}
> -
>   static void filter_mirror_class_init(ObjectClass *oc, void *data)
>   {
>       NetFilterClass *nfc = NETFILTER_CLASS(oc);
>   
>       object_class_property_add_str(oc, "outdev", filter_mirror_get_outdev,
>                                     filter_mirror_set_outdev);
> -    object_class_property_add_bool(oc, "vnet_hdr_support",
> -                                   filter_mirror_get_vnet_hdr,
> -                                   filter_mirror_set_vnet_hdr);
>   
>       nfc->setup = filter_mirror_setup;
>       nfc->cleanup = filter_mirror_cleanup;
> @@ -393,29 +362,12 @@ static void filter_redirector_class_init(ObjectClass *oc, void *data)
>                                     filter_redirector_set_indev);
>       object_class_property_add_str(oc, "outdev", filter_redirector_get_outdev,
>                                     filter_redirector_set_outdev);
> -    object_class_property_add_bool(oc, "vnet_hdr_support",
> -                                   filter_redirector_get_vnet_hdr,
> -                                   filter_redirector_set_vnet_hdr);
>   
>       nfc->setup = filter_redirector_setup;
>       nfc->cleanup = filter_redirector_cleanup;
>       nfc->receive_iov = filter_redirector_receive_iov;
>   }
>   
> -static void filter_mirror_init(Object *obj)
> -{
> -    MirrorState *s = FILTER_MIRROR(obj);
> -
> -    s->vnet_hdr = false;
> -}
> -
> -static void filter_redirector_init(Object *obj)
> -{
> -    MirrorState *s = FILTER_REDIRECTOR(obj);
> -
> -    s->vnet_hdr = false;
> -}
> -
>   static void filter_mirror_fini(Object *obj)
>   {
>       MirrorState *s = FILTER_MIRROR(obj);
> @@ -435,7 +387,6 @@ static const TypeInfo filter_redirector_info = {
>       .name = TYPE_FILTER_REDIRECTOR,
>       .parent = TYPE_NETFILTER,
>       .class_init = filter_redirector_class_init,
> -    .instance_init = filter_redirector_init,
>       .instance_finalize = filter_redirector_fini,
>       .instance_size = sizeof(MirrorState),
>   };
> @@ -444,7 +395,6 @@ static const TypeInfo filter_mirror_info = {
>       .name = TYPE_FILTER_MIRROR,
>       .parent = TYPE_NETFILTER,
>       .class_init = filter_mirror_class_init,
> -    .instance_init = filter_mirror_init,
>       .instance_finalize = filter_mirror_fini,
>       .instance_size = sizeof(MirrorState),
>   };
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 5f375bbfa6..38c03812a7 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -4946,18 +4946,16 @@ SRST
>   
>           ``behind``: insert behind the specified filter (default).
>   
> -    ``-object filter-mirror,id=id,netdev=netdevid,outdev=chardevid,queue=all|rx|tx[,vnet_hdr_support][,position=head|tail|id=<id>][,insert=behind|before]``
> +    ``-object filter-mirror,id=id,netdev=netdevid,outdev=chardevid,queue=all|rx|tx[,position=head|tail|id=<id>][,insert=behind|before]``


I wonder if we break management layer. If yes, maybe it's better to keep 
the vnet_hdr_support here.

Thanks


>           filter-mirror on netdev netdevid,mirror net packet to
> -        chardevchardevid, if it has the vnet\_hdr\_support flag,
> -        filter-mirror will mirror packet with vnet\_hdr\_len.
> +        chardev chardevid, filter-mirror will mirror packet with vnet\_hdr\_len.
>   
> -    ``-object filter-redirector,id=id,netdev=netdevid,indev=chardevid,outdev=chardevid,queue=all|rx|tx[,vnet_hdr_support][,position=head|tail|id=<id>][,insert=behind|before]``
> +    ``-object filter-redirector,id=id,netdev=netdevid,indev=chardevid,outdev=chardevid,queue=all|rx|tx[,position=head|tail|id=<id>][,insert=behind|before]``
>           filter-redirector on netdev netdevid,redirect filter's net
>           packet to chardev chardevid,and redirect indev's packet to
> -        filter.if it has the vnet\_hdr\_support flag, filter-redirector
> -        will redirect packet with vnet\_hdr\_len. Create a
> -        filter-redirector we need to differ outdev id from indev id, id
> -        can not be the same. we can just use indev or outdev, but at
> +        filter. filter-redirector will redirect packet with vnet\_hdr\_len.
> +        Create a filter-redirector we need to differ outdev id from indev id,
> +        id can not be the same. we can just use indev or outdev, but at
>           least one of indev or outdev need to be specified.
>   
>       ``-object filter-rewriter,id=id,netdev=netdevid,queue=all|rx|tx,[vnet_hdr_support][,position=head|tail|id=<id>][,insert=behind|before]``

RE: [PATCH V4 1/3] net/filter: Remove vnet_hdr from filter-mirror and filter-redirector

[Zhang, Chen]

> -----Original Message-----
> From: Jason Wang <jasowang@redhat.com>
> Sent: Wednesday, October 27, 2021 12:46 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: qemu-dev <qemu-devel@nongnu.org>; Markus Armbruster
> <armbru@redhat.com>; Li Zhijian <lizhijian@cn.fujitsu.com>
> Subject: Re: [PATCH V4 1/3] net/filter: Remove vnet_hdr from filter-mirror
> and filter-redirector
> 
> 
> 在 2021/10/27 上午2:17, Zhang Chen 写道:
> > Make the vnet header a necessary part of filter transfer protocol.
> > So remove the module's vnet_hdr_support switch here.
> > It make other modules(like another filter-redirector,colo-compare...)
> > know how to parse net packet correctly. If local device is not the
> > virtio-net-pci, vnet_hdr_len will be 0.
> >
> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> > ---
> >   net/filter-mirror.c | 88 ++++++++++-----------------------------------
> >   qemu-options.hx     | 14 ++++----
> >   2 files changed, 25 insertions(+), 77 deletions(-)
> >
> > diff --git a/net/filter-mirror.c b/net/filter-mirror.c index
> > f20240cc9f..4f0e26cc92 100644
> > --- a/net/filter-mirror.c
> > +++ b/net/filter-mirror.c
> > @@ -39,7 +39,6 @@ struct MirrorState {
> >       CharBackend chr_in;
> >       CharBackend chr_out;
> >       SocketReadState rs;
> > -    bool vnet_hdr;
> >   };
> >
> >   static int filter_send(MirrorState *s, @@ -48,7 +47,7 @@ static int
> > filter_send(MirrorState *s,
> >   {
> >       NetFilterState *nf = NETFILTER(s);
> >       int ret = 0;
> > -    ssize_t size = 0;
> > +    ssize_t size = 0, vnet_hdr_len = 0;
> >       uint32_t len = 0;
> >       char *buf;
> >
> > @@ -63,21 +62,18 @@ static int filter_send(MirrorState *s,
> >           goto err;
> >       }
> >
> > -    if (s->vnet_hdr) {
> > -        /*
> > -         * If vnet_hdr = on, we send vnet header len to make other
> > -         * module(like colo-compare) know how to parse net
> > -         * packet correctly.
> > -         */
> > -        ssize_t vnet_hdr_len;
> > -
> > -        vnet_hdr_len = nf->netdev->vnet_hdr_len;
> > +    /*
> > +     * The vnet header is the necessary part of filter transfer protocol.
> > +     * It make other module(like colo-compare) know how to parse net
> > +     * packet correctly. If device is not the virtio-net-pci,
> > +     * vnet_hdr_len will be 0.
> > +     */
> > +    vnet_hdr_len = nf->netdev->vnet_hdr_len;
> >
> > -        len = htonl(vnet_hdr_len);
> > -        ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)&len,
> sizeof(len));
> > -        if (ret != sizeof(len)) {
> > -            goto err;
> > -        }
> > +    len = htonl(vnet_hdr_len);
> > +    ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)&len, sizeof(len));
> > +    if (ret != sizeof(len)) {
> > +        goto err;
> >       }
> >
> >       buf = g_malloc(size);
> > @@ -232,6 +228,12 @@ static void redirector_rs_finalize(SocketReadState
> *rs)
> >       MirrorState *s = container_of(rs, MirrorState, rs);
> >       NetFilterState *nf = NETFILTER(s);
> >
> > +    /* Check remote vnet_hdr */
> > +    if (rs->vnet_hdr_len != nf->netdev->vnet_hdr_len) {
> > +        error_report("filter redirector got a different packet vnet_hdr"
> > +        " from local, please check the -device configuration");
> > +    }
> > +
> >       redirector_to_filter(nf, rs->buf, rs->packet_len);
> >   }
> >
> > @@ -252,7 +254,7 @@ static void filter_redirector_setup(NetFilterState
> *nf, Error **errp)
> >           }
> >       }
> >
> > -    net_socket_rs_init(&s->rs, redirector_rs_finalize, s->vnet_hdr);
> > +    net_socket_rs_init(&s->rs, redirector_rs_finalize, true);
> >
> >       if (s->indev) {
> >           chr = qemu_chr_find(s->indev); @@ -323,20 +325,6 @@ static
> > void filter_mirror_set_outdev(Object *obj,
> >       }
> >   }
> >
> > -static bool filter_mirror_get_vnet_hdr(Object *obj, Error **errp) -{
> > -    MirrorState *s = FILTER_MIRROR(obj);
> > -
> > -    return s->vnet_hdr;
> > -}
> > -
> > -static void filter_mirror_set_vnet_hdr(Object *obj, bool value, Error
> > **errp) -{
> > -    MirrorState *s = FILTER_MIRROR(obj);
> > -
> > -    s->vnet_hdr = value;
> > -}
> > -
> >   static char *filter_redirector_get_outdev(Object *obj, Error **errp)
> >   {
> >       MirrorState *s = FILTER_REDIRECTOR(obj); @@ -354,31 +342,12 @@
> > static void filter_redirector_set_outdev(Object *obj,
> >       s->outdev = g_strdup(value);
> >   }
> >
> > -static bool filter_redirector_get_vnet_hdr(Object *obj, Error **errp)
> > -{
> > -    MirrorState *s = FILTER_REDIRECTOR(obj);
> > -
> > -    return s->vnet_hdr;
> > -}
> > -
> > -static void filter_redirector_set_vnet_hdr(Object *obj,
> > -                                           bool value,
> > -                                           Error **errp)
> > -{
> > -    MirrorState *s = FILTER_REDIRECTOR(obj);
> > -
> > -    s->vnet_hdr = value;
> > -}
> > -
> >   static void filter_mirror_class_init(ObjectClass *oc, void *data)
> >   {
> >       NetFilterClass *nfc = NETFILTER_CLASS(oc);
> >
> >       object_class_property_add_str(oc, "outdev", filter_mirror_get_outdev,
> >                                     filter_mirror_set_outdev);
> > -    object_class_property_add_bool(oc, "vnet_hdr_support",
> > -                                   filter_mirror_get_vnet_hdr,
> > -                                   filter_mirror_set_vnet_hdr);
> >
> >       nfc->setup = filter_mirror_setup;
> >       nfc->cleanup = filter_mirror_cleanup; @@ -393,29 +362,12 @@
> > static void filter_redirector_class_init(ObjectClass *oc, void *data)
> >                                     filter_redirector_set_indev);
> >       object_class_property_add_str(oc, "outdev",
> filter_redirector_get_outdev,
> >                                     filter_redirector_set_outdev);
> > -    object_class_property_add_bool(oc, "vnet_hdr_support",
> > -                                   filter_redirector_get_vnet_hdr,
> > -                                   filter_redirector_set_vnet_hdr);
> >
> >       nfc->setup = filter_redirector_setup;
> >       nfc->cleanup = filter_redirector_cleanup;
> >       nfc->receive_iov = filter_redirector_receive_iov;
> >   }
> >
> > -static void filter_mirror_init(Object *obj) -{
> > -    MirrorState *s = FILTER_MIRROR(obj);
> > -
> > -    s->vnet_hdr = false;
> > -}
> > -
> > -static void filter_redirector_init(Object *obj) -{
> > -    MirrorState *s = FILTER_REDIRECTOR(obj);
> > -
> > -    s->vnet_hdr = false;
> > -}
> > -
> >   static void filter_mirror_fini(Object *obj)
> >   {
> >       MirrorState *s = FILTER_MIRROR(obj); @@ -435,7 +387,6 @@ static
> > const TypeInfo filter_redirector_info = {
> >       .name = TYPE_FILTER_REDIRECTOR,
> >       .parent = TYPE_NETFILTER,
> >       .class_init = filter_redirector_class_init,
> > -    .instance_init = filter_redirector_init,
> >       .instance_finalize = filter_redirector_fini,
> >       .instance_size = sizeof(MirrorState),
> >   };
> > @@ -444,7 +395,6 @@ static const TypeInfo filter_mirror_info = {
> >       .name = TYPE_FILTER_MIRROR,
> >       .parent = TYPE_NETFILTER,
> >       .class_init = filter_mirror_class_init,
> > -    .instance_init = filter_mirror_init,
> >       .instance_finalize = filter_mirror_fini,
> >       .instance_size = sizeof(MirrorState),
> >   };
> > diff --git a/qemu-options.hx b/qemu-options.hx index
> > 5f375bbfa6..38c03812a7 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -4946,18 +4946,16 @@ SRST
> >
> >           ``behind``: insert behind the specified filter (default).
> >
> > -    ``-object filter-
> mirror,id=id,netdev=netdevid,outdev=chardevid,queue=all|rx|tx[,vnet_hdr
> _support][,position=head|tail|id=<id>][,insert=behind|before]``
> > +    ``-object
> > + filter-mirror,id=id,netdev=netdevid,outdev=chardevid,queue=all|rx|tx
> > + [,position=head|tail|id=<id>][,insert=behind|before]``
> 
> 
> I wonder if we break management layer. If yes, maybe it's better to keep the
> vnet_hdr_support here.

Yes and no,   With this series of patches, filters have ability to automatically
Configure the appropriate vnet_hdr_support flag according to the current environment.
And can report error when can't fixing the vnet_hdr(The user cannot fix it from the previous way ).
So I think no need for the user to configure this option, some relevant background knowledge required.

For the management layer, keep the vnet_hdr_support may be meaningless except for compatibility.
In this situation, Do you think we still need to keep the vnet_hdr_support for management layer?
Enable/disable it do the same things for filters.

Thanks
Chen

> 
> Thanks
> 
> 
> >           filter-mirror on netdev netdevid,mirror net packet to
> > -        chardevchardevid, if it has the vnet\_hdr\_support flag,
> > -        filter-mirror will mirror packet with vnet\_hdr\_len.
> > +        chardev chardevid, filter-mirror will mirror packet with
> vnet\_hdr\_len.
> >
> > -    ``-object filter-
> redirector,id=id,netdev=netdevid,indev=chardevid,outdev=chardevid,queu
> e=all|rx|tx[,vnet_hdr_support][,position=head|tail|id=<id>][,insert=behind
> |before]``
> > +    ``-object
> > + filter-redirector,id=id,netdev=netdevid,indev=chardevid,outdev=chard
> > + evid,queue=all|rx|tx[,position=head|tail|id=<id>][,insert=behind|bef
> > + ore]``
> >           filter-redirector on netdev netdevid,redirect filter's net
> >           packet to chardev chardevid,and redirect indev's packet to
> > -        filter.if it has the vnet\_hdr\_support flag, filter-redirector
> > -        will redirect packet with vnet\_hdr\_len. Create a
> > -        filter-redirector we need to differ outdev id from indev id, id
> > -        can not be the same. we can just use indev or outdev, but at
> > +        filter. filter-redirector will redirect packet with vnet\_hdr\_len.
> > +        Create a filter-redirector we need to differ outdev id from indev id,
> > +        id can not be the same. we can just use indev or outdev, but
> > + at
> >           least one of indev or outdev need to be specified.
> >
> >       ``-object
> > filter-rewriter,id=id,netdev=netdevid,queue=all|rx|tx,[vnet_hdr_suppor
> > t][,position=head|tail|id=<id>][,insert=behind|before]``

Re: [PATCH V4 1/3] net/filter: Remove vnet_hdr from filter-mirror and filter-redirector

[Jason Wang]

在 2021/10/27 下午2:19, Zhang, Chen 写道:
>> mirror,id=id,netdev=netdevid,outdev=chardevid,queue=all|rx|tx[,vnet_hdr
>> _support][,position=head|tail|id=<id>][,insert=behind|before]``
>>> +    ``-object
>>> + filter-mirror,id=id,netdev=netdevid,outdev=chardevid,queue=all|rx|tx
>>> + [,position=head|tail|id=<id>][,insert=behind|before]``
>> I wonder if we break management layer. If yes, maybe it's better to keep the
>> vnet_hdr_support here.
> Yes and no,   With this series of patches, filters have ability to automatically
> Configure the appropriate vnet_hdr_support flag according to the current environment.
> And can report error when can't fixing the vnet_hdr(The user cannot fix it from the previous way ).
> So I think no need for the user to configure this option, some relevant background knowledge required.
>
> For the management layer, keep the vnet_hdr_support may be meaningless except for compatibility.
> In this situation, Do you think we still need to keep the vnet_hdr_support for management layer?


So it depends on whether management layer like libvirt has already  
supported this. If yes, we may get errors using new qemu with old libvirt?

Thanks

> Enable/disable it do the same things for filters.
>
> Thanks
> Chen
>

RE: [PATCH V4 1/3] net/filter: Remove vnet_hdr from filter-mirror and filter-redirector

[Zhang, Chen]

> -----Original Message-----
> From: Jason Wang <jasowang@redhat.com>
> Sent: Wednesday, October 27, 2021 2:24 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: qemu-dev <qemu-devel@nongnu.org>; Markus Armbruster
> <armbru@redhat.com>; Li Zhijian <lizhijian@cn.fujitsu.com>
> Subject: Re: [PATCH V4 1/3] net/filter: Remove vnet_hdr from filter-mirror
> and filter-redirector
> 
> 
> 在 2021/10/27 下午2:19, Zhang, Chen 写道:
> >>
> mirror,id=id,netdev=netdevid,outdev=chardevid,queue=all|rx|tx[,vnet_h
> >> dr _support][,position=head|tail|id=<id>][,insert=behind|before]``
> >>> +    ``-object
> >>> + filter-mirror,id=id,netdev=netdevid,outdev=chardevid,queue=all|rx|
> >>> + tx [,position=head|tail|id=<id>][,insert=behind|before]``
> >> I wonder if we break management layer. If yes, maybe it's better to
> >> keep the vnet_hdr_support here.
> > Yes and no,   With this series of patches, filters have ability to automatically
> > Configure the appropriate vnet_hdr_support flag according to the current
> environment.
> > And can report error when can't fixing the vnet_hdr(The user cannot fix it
> from the previous way ).
> > So I think no need for the user to configure this option, some relevant
> background knowledge required.
> >
> > For the management layer, keep the vnet_hdr_support may be
> meaningless except for compatibility.
> > In this situation, Do you think we still need to keep the vnet_hdr_support
> for management layer?
> 
> 
> So it depends on whether management layer like libvirt has already
> supported this. If yes, we may get errors using new qemu with old libvirt?

As far as I know, Current management layer like upstream libvirt is no COLO official support yet.
And some real CSPs use libvirt passthrough qmp command to Qemu for manage COLO VM.
It is no harm to users to reduce some unnecessary parameters. But if you think compatibility is
more important, I will restore this parameter in next version.

Thanks
Chen




> 
> Thanks
> 
> > Enable/disable it do the same things for filters.
> >
> > Thanks
> > Chen
> >

Re: [PATCH V4 1/3] net/filter: Remove vnet_hdr from filter-mirror and filter-redirector

[Jason Wang]

On Wed, Oct 27, 2021 at 2:40 PM Zhang, Chen <chen.zhang@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Wednesday, October 27, 2021 2:24 PM
> > To: Zhang, Chen <chen.zhang@intel.com>
> > Cc: qemu-dev <qemu-devel@nongnu.org>; Markus Armbruster
> > <armbru@redhat.com>; Li Zhijian <lizhijian@cn.fujitsu.com>
> > Subject: Re: [PATCH V4 1/3] net/filter: Remove vnet_hdr from filter-mirror
> > and filter-redirector
> >
> >
> > 在 2021/10/27 下午2:19, Zhang, Chen 写道:
> > >>
> > mirror,id=id,netdev=netdevid,outdev=chardevid,queue=all|rx|tx[,vnet_h
> > >> dr _support][,position=head|tail|id=<id>][,insert=behind|before]``
> > >>> +    ``-object
> > >>> + filter-mirror,id=id,netdev=netdevid,outdev=chardevid,queue=all|rx|
> > >>> + tx [,position=head|tail|id=<id>][,insert=behind|before]``
> > >> I wonder if we break management layer. If yes, maybe it's better to
> > >> keep the vnet_hdr_support here.
> > > Yes and no,   With this series of patches, filters have ability to automatically
> > > Configure the appropriate vnet_hdr_support flag according to the current
> > environment.
> > > And can report error when can't fixing the vnet_hdr(The user cannot fix it
> > from the previous way ).
> > > So I think no need for the user to configure this option, some relevant
> > background knowledge required.
> > >
> > > For the management layer, keep the vnet_hdr_support may be
> > meaningless except for compatibility.
> > > In this situation, Do you think we still need to keep the vnet_hdr_support
> > for management layer?
> >
> >
> > So it depends on whether management layer like libvirt has already
> > supported this. If yes, we may get errors using new qemu with old libvirt?
>
> As far as I know, Current management layer like upstream libvirt is no COLO official support yet.
> And some real CSPs use libvirt passthrough qmp command to Qemu for manage COLO VM.

So the question still, it looks to me it requires the modification of
the layers on top of libvirt? If the answer is yes, we'd better keep
that compatibility.

> It is no harm to users to reduce some unnecessary parameters. But if you think compatibility is
> more important, I will restore this parameter in next version.

Thanks

>
> Thanks
> Chen
>
>
>
>
> >
> > Thanks
> >
> > > Enable/disable it do the same things for filters.
> > >
> > > Thanks
> > > Chen
> > >
>

RE: [PATCH V4 1/3] net/filter: Remove vnet_hdr from filter-mirror and filter-redirector

[Zhang, Chen]

> -----Original Message-----
> From: Jason Wang <jasowang@redhat.com>
> Sent: Wednesday, October 27, 2021 2:45 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: qemu-dev <qemu-devel@nongnu.org>; Markus Armbruster
> <armbru@redhat.com>; Li Zhijian <lizhijian@cn.fujitsu.com>
> Subject: Re: [PATCH V4 1/3] net/filter: Remove vnet_hdr from filter-mirror
> and filter-redirector
> 
> On Wed, Oct 27, 2021 at 2:40 PM Zhang, Chen <chen.zhang@intel.com>
> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Jason Wang <jasowang@redhat.com>
> > > Sent: Wednesday, October 27, 2021 2:24 PM
> > > To: Zhang, Chen <chen.zhang@intel.com>
> > > Cc: qemu-dev <qemu-devel@nongnu.org>; Markus Armbruster
> > > <armbru@redhat.com>; Li Zhijian <lizhijian@cn.fujitsu.com>
> > > Subject: Re: [PATCH V4 1/3] net/filter: Remove vnet_hdr from
> > > filter-mirror and filter-redirector
> > >
> > >
> > > 在 2021/10/27 下午2:19, Zhang, Chen 写道:
> > > >>
> > > mirror,id=id,netdev=netdevid,outdev=chardevid,queue=all|rx|tx[,vnet_
> > > h
> > > >> dr
> > > >> _support][,position=head|tail|id=<id>][,insert=behind|before]``
> > > >>> +    ``-object
> > > >>> + filter-mirror,id=id,netdev=netdevid,outdev=chardevid,queue=all
> > > >>> + |rx| tx [,position=head|tail|id=<id>][,insert=behind|before]``
> > > >> I wonder if we break management layer. If yes, maybe it's better
> > > >> to keep the vnet_hdr_support here.
> > > > Yes and no,   With this series of patches, filters have ability to
> automatically
> > > > Configure the appropriate vnet_hdr_support flag according to the
> > > > current
> > > environment.
> > > > And can report error when can't fixing the vnet_hdr(The user
> > > > cannot fix it
> > > from the previous way ).
> > > > So I think no need for the user to configure this option, some
> > > > relevant
> > > background knowledge required.
> > > >
> > > > For the management layer, keep the vnet_hdr_support may be
> > > meaningless except for compatibility.
> > > > In this situation, Do you think we still need to keep the
> > > > vnet_hdr_support
> > > for management layer?
> > >
> > >
> > > So it depends on whether management layer like libvirt has already
> > > supported this. If yes, we may get errors using new qemu with old libvirt?
> >
> > As far as I know, Current management layer like upstream libvirt is no COLO
> official support yet.
> > And some real CSPs use libvirt passthrough qmp command to Qemu for
> manage COLO VM.
> 
> So the question still, it looks to me it requires the modification of the layers
> on top of libvirt? If the answer is yes, we'd better keep that compatibility.
> 

Yes, I will keep the vnet_hdr_support and add some comments to update it in next version.

Thanks
Chen

> > It is no harm to users to reduce some unnecessary parameters. But if
> > you think compatibility is more important, I will restore this parameter in
> next version.
> 
> Thanks
> 
> >
> > Thanks
> > Chen
> >
> >
> >
> >
> > >
> > > Thanks
> > >
> > > > Enable/disable it do the same things for filters.
> > > >
> > > > Thanks
> > > > Chen
> > > >
> >

Re: [PATCH V4 1/3] net/filter: Remove vnet_hdr from filter-mirror and filter-redirector

[Markus Armbruster]

Jason Wang <jasowang@redhat.com> writes:

> On Wed, Oct 27, 2021 at 2:40 PM Zhang, Chen <chen.zhang@intel.com> wrote:

[...]

>> > >> I wonder if we break management layer. If yes, maybe it's better to
>> > >> keep the vnet_hdr_support here.
>> > >
>> > > Yes and no,   With this series of patches, filters have ability to automatically
>> > > Configure the appropriate vnet_hdr_support flag according to the current environment.
>> > > And can report error when can't fixing the vnet_hdr(The user cannot fix it from the previous way ).
>> > > So I think no need for the user to configure this option, some relevant background knowledge required.
>> > >
>> > > For the management layer, keep the vnet_hdr_support may be meaningless except for compatibility.
>> > > In this situation, Do you think we still need to keep the vnet_hdr_support for management layer?
>> >
>> >
>> > So it depends on whether management layer like libvirt has already
>> > supported this. If yes, we may get errors using new qemu with old libvirt?
>>
>> As far as I know, Current management layer like upstream libvirt is no COLO official support yet.
>> And some real CSPs use libvirt passthrough qmp command to Qemu for manage COLO VM.
>
> So the question still, it looks to me it requires the modification of
> the layers on top of libvirt? If the answer is yes, we'd better keep
> that compatibility.

When in doubt, maintain compatibility.

We may want to deprecate parameters that have become unnecessary.

[...]