diff mbox series

[V8,1/6] qapi/net: Add IPFlowSpec and QMP command for COLO passthrough

Message ID 20210615113740.2278015-2-chen.zhang@intel.com
State New
Headers show
Series Passthrough specific network traffic in COLO | expand

Commit Message

Zhang, Chen June 15, 2021, 11:37 a.m. UTC 
Since the real user scenario does not need COLO to monitor all traffic.
Add colo-passthrough-add and colo-passthrough-del to maintain
a COLO network passthrough list. Add IPFlowSpec struct for all QMP commands.
All the fields of IPFlowSpec are optional.

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

Comments

Markus Armbruster June 15, 2021, 2:43 p.m. UTC | #1 
Zhang Chen <chen.zhang@intel.com> writes:

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

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

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

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

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


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

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

Jason or David, perhaps?

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

Actually, all arguments are optional.

Suggest:

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

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

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

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

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

Can it delete more than one such entry?

Suggest:

   # Delete an entry from the COLO network passthrough list.

and then explain how the command arguments select entries.

> +#
> +# Returns: Nothing on success
> +#
> +# Since: 6.1
> +#
> +# Example:
> +#
> +# -> { "execute": "colo-passthrough-del",
> +#      "arguments": { "protocol": "tcp", "object-name": "object0",
> +#      "source": {"host": "192.168.1.1", "port": "1234"},
> +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
> +# <- { "return": {} }
> +#
> +##
> +{ 'command': 'colo-passthrough-del', 'boxed': true,
> +     'data': 'IPFlowSpec' }
Lukas Straub June 15, 2021, 3:01 p.m. UTC | #2 
On Tue, 15 Jun 2021 19:37:35 +0800
Zhang Chen <chen.zhang@intel.com> wrote:

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

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

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



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

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

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

Thanks
Chen


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

OK.

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

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

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

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

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


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

Yes, more than one list.

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

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

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

Yes.

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

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

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

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

Thanks
Chen

> 
> > +#
> > +# Returns: Nothing on success
> > +#
> > +# Since: 6.1
> > +#
> > +# Example:
> > +#
> > +# -> { "execute": "colo-passthrough-del",
> > +#      "arguments": { "protocol": "tcp", "object-name": "object0",
> > +#      "source": {"host": "192.168.1.1", "port": "1234"},
> > +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
> > +# <- { "return": {} }
> > +#
> > +##
> > +{ 'command': 'colo-passthrough-del', 'boxed': true,
> > +     'data': 'IPFlowSpec' }
Markus Armbruster June 16, 2021, 6:04 a.m. UTC | #5 
"Zhang, Chen" <chen.zhang@intel.com> writes:

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

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

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

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

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

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

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

Got it now.

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

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

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

1. Select first entry that matches.

2. Select all entries that match.

3. If exactly one entry matches, select it.

The second design choice is behavior when nothing gets selected:

a. Silently do nothing

b. Error

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

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

Yes.

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

Yes.

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

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

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

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


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

Please see the explanation below.

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

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

Thanks
Chen


> 
> >> Suggest:
> >>
> >>    # Delete an entry from the COLO network passthrough list.
> >>
> >> and then explain how the command arguments select entries.
> >
> > Search the object's passthrough list, if find same entry,  delete it.
> >
> > Thanks
> > Chen
> >
> >>
> >> > +#
> >> > +# Returns: Nothing on success
> >> > +#
> >> > +# Since: 6.1
> >> > +#
> >> > +# Example:
> >> > +#
> >> > +# -> { "execute": "colo-passthrough-del",
> >> > +#      "arguments": { "protocol": "tcp", "object-name": "object0",
> >> > +#      "source": {"host": "192.168.1.1", "port": "1234"},
> >> > +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
> >> > +# <- { "return": {} }
> >> > +#
> >> > +##
> >> > +{ 'command': 'colo-passthrough-del', 'boxed': true,
> >> > +     'data': 'IPFlowSpec' }
Markus Armbruster June 16, 2021, 1:26 p.m. UTC | #7 
"Zhang, Chen" <chen.zhang@intel.com> writes:

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

Got it.

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

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

Okay.

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

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

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

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

I'm not sure I understand.

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

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

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

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

Yes, I think so.

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

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


Thanks
Chen
Markus Armbruster June 17, 2021, 11:03 a.m. UTC | #9 
You recently started using

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

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

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

[...]

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

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

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

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

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

Dave

> > +#
> > +# @source: Source address and port.
> > +#
> > +# @destination: Destination address and port.
> > +#
> > +# Since: 6.1
> > +##
> > +{ 'struct': 'IPFlowSpec',
> > +  'data': { '*protocol': 'str', '*object-name': 'str',
> > +    '*source': 'InetSocketAddressBase',
> > +    '*destination': 'InetSocketAddressBase' } }
> > +
> > +##
> > +# @colo-passthrough-add:
> > +#
> > +# Add passthrough entry IPFlowSpec to the COLO-compare instance.
> > +# The protocol and source/destination IP/ports are optional. if the user
> > +# only inputs part of the information, this will match all traffic.
> 
> Actually, all arguments are optional.
> 
> Suggest:
> 
>    # Add an entry to the COLO network passthrough list.
>    # Absent protocol, host addresses and ports match anything.
> 
> If there is more than one such list, then "to a COLO network passthrough
> list" instead.
> 
> Still missing then: meaning of absent @object-name.  Does it select the
> COLO network passthrough list, perhaps?
> 
> > +#
> > +# Returns: Nothing on success
> > +#
> > +# Since: 6.1
> > +#
> > +# Example:
> > +#
> > +# -> { "execute": "colo-passthrough-add",
> > +#      "arguments": { "protocol": "tcp", "object-name": "object0",
> > +#      "source": {"host": "192.168.1.1", "port": "1234"},
> > +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
> > +# <- { "return": {} }
> > +#
> > +##
> > +{ 'command': 'colo-passthrough-add', 'boxed': true,
> > +     'data': 'IPFlowSpec' }
> > +
> > +##
> > +# @colo-passthrough-del:
> > +#
> > +# Delete passthrough entry IPFlowSpec to the COLO-compare instance.
> > +# The protocol and source/destination IP/ports are optional. if the user
> > +# only inputs part of the information, this will match all traffic.
> 
> I suspect this command doesn't actually match traffic, it matches
> entries added with colo-passthrough-add.
> 
> Can it delete more than one such entry?
> 
> Suggest:
> 
>    # Delete an entry from the COLO network passthrough list.
> 
> and then explain how the command arguments select entries.
> 
> > +#
> > +# Returns: Nothing on success
> > +#
> > +# Since: 6.1
> > +#
> > +# Example:
> > +#
> > +# -> { "execute": "colo-passthrough-del",
> > +#      "arguments": { "protocol": "tcp", "object-name": "object0",
> > +#      "source": {"host": "192.168.1.1", "port": "1234"},
> > +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
> > +# <- { "return": {} }
> > +#
> > +##
> > +{ 'command': 'colo-passthrough-del', 'boxed': true,
> > +     'data': 'IPFlowSpec' }
Zhang, Chen June 22, 2021, 5:58 a.m. UTC | #11 
On 6/17/21 7:03 PM, Markus Armbruster wrote:
> You recently started using
>
>      Content-Type: text/plain; charset="utf-8"
>      Content-Transfer-Encoding: base64
>
> Please consider anorhter Content-Transfer-Encoding instead.
> quoted-printable should do.
>
> "Zhang, Chen" <chen.zhang@intel.com> writes:
>
> [...]
>
>> No, net-filter is an independent module, although colo must use net-filter to build colo-proxy.
>> I think we can change the name when enable net-filter support passthrough list.
> Changing names of stable interfaces is always awkward.  So, either make
> this an unstable interface, or use names that are likely to work for the
> foreseeable evolution of the interface.
>
> What about passthrough-filter-add and -del?  Jason?


Hi Markus and Dave,

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

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


Thanks

Chen


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

Yes, you are right.


Thanks

Chen


> Dave
>
>>> +#
>>> +# @source: Source address and port.
>>> +#
>>> +# @destination: Destination address and port.
>>> +#
>>> +# Since: 6.1
>>> +##
>>> +{ 'struct': 'IPFlowSpec',
>>> +  'data': { '*protocol': 'str', '*object-name': 'str',
>>> +    '*source': 'InetSocketAddressBase',
>>> +    '*destination': 'InetSocketAddressBase' } }
>>> +
>>> +##
>>> +# @colo-passthrough-add:
>>> +#
>>> +# Add passthrough entry IPFlowSpec to the COLO-compare instance.
>>> +# The protocol and source/destination IP/ports are optional. if the user
>>> +# only inputs part of the information, this will match all traffic.
>> Actually, all arguments are optional.
>>
>> Suggest:
>>
>>     # Add an entry to the COLO network passthrough list.
>>     # Absent protocol, host addresses and ports match anything.
>>
>> If there is more than one such list, then "to a COLO network passthrough
>> list" instead.
>>
>> Still missing then: meaning of absent @object-name.  Does it select the
>> COLO network passthrough list, perhaps?
>>
>>> +#
>>> +# Returns: Nothing on success
>>> +#
>>> +# Since: 6.1
>>> +#
>>> +# Example:
>>> +#
>>> +# -> { "execute": "colo-passthrough-add",
>>> +#      "arguments": { "protocol": "tcp", "object-name": "object0",
>>> +#      "source": {"host": "192.168.1.1", "port": "1234"},
>>> +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
>>> +# <- { "return": {} }
>>> +#
>>> +##
>>> +{ 'command': 'colo-passthrough-add', 'boxed': true,
>>> +     'data': 'IPFlowSpec' }
>>> +
>>> +##
>>> +# @colo-passthrough-del:
>>> +#
>>> +# Delete passthrough entry IPFlowSpec to the COLO-compare instance.
>>> +# The protocol and source/destination IP/ports are optional. if the user
>>> +# only inputs part of the information, this will match all traffic.
>> I suspect this command doesn't actually match traffic, it matches
>> entries added with colo-passthrough-add.
>>
>> Can it delete more than one such entry?
>>
>> Suggest:
>>
>>     # Delete an entry from the COLO network passthrough list.
>>
>> and then explain how the command arguments select entries.
>>
>>> +#
>>> +# Returns: Nothing on success
>>> +#
>>> +# Since: 6.1
>>> +#
>>> +# Example:
>>> +#
>>> +# -> { "execute": "colo-passthrough-del",
>>> +#      "arguments": { "protocol": "tcp", "object-name": "object0",
>>> +#      "source": {"host": "192.168.1.1", "port": "1234"},
>>> +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
>>> +# <- { "return": {} }
>>> +#
>>> +##
>>> +{ 'command': 'colo-passthrough-del', 'boxed': true,
>>> +     'data': 'IPFlowSpec' }
Jason Wang June 22, 2021, 7:04 a.m. UTC | #13 
在 2021/6/22 下午2:01, chen.zhang@intel.com 写道:
>
> On 6/21/21 7:30 PM, Dr. David Alan Gilbert wrote:
>> * Markus Armbruster (armbru@redhat.com) wrote:
>>> Zhang Chen <chen.zhang@intel.com> writes:
>>>
>>>> Since the real user scenario does not need COLO to monitor all 
>>>> traffic.
>>>> Add colo-passthrough-add and colo-passthrough-del to maintain
>>>> a COLO network passthrough list. Add IPFlowSpec struct for all QMP 
>>>> commands.
>>>> All the fields of IPFlowSpec are optional.
>>>>
>>>> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
>>>> ---
>>> The QAPI schema looks good to me, but the interface documentation is
>>> still not quite clear enough.  To make progress, I'm going to make
>>> concrete suggestions wherever I can despite being quite clueless about
>>> the subject matter.  Risks me writing something that's clearer, but
>>> wrong.  Keep that in mind, please.
>>>
>>>>   net/net.c     | 10 +++++++
>>>>   qapi/net.json | 74 
>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>   2 files changed, 84 insertions(+)
>>>>
>>>> diff --git a/net/net.c b/net/net.c
>>>> index 76bbb7c31b..f913e97983 100644
>>>> --- a/net/net.c
>>>> +++ b/net/net.c
>>>> @@ -1195,6 +1195,16 @@ void qmp_netdev_del(const char *id, Error 
>>>> **errp)
>>>>       }
>>>>   }
>>>>   +void qmp_colo_passthrough_add(IPFlowSpec *spec, Error **errp)
>>>> +{
>>>> +    /* TODO implement setup passthrough rule */
>>>> +}
>>>> +
>>>> +void qmp_colo_passthrough_del(IPFlowSpec *spec, Error **errp)
>>>> +{
>>>> +    /* TODO implement delete passthrough rule */
>>>> +}
>>>> +
>>>>   static void netfilter_print_info(Monitor *mon, NetFilterState *nf)
>>>>   {
>>>>       char *str;
>>>> diff --git a/qapi/net.json b/qapi/net.json
>>>> index 7fab2e7cd8..91f2e1495a 100644
>>>> --- a/qapi/net.json
>>>> +++ b/qapi/net.json
>>>> @@ -7,6 +7,7 @@
>>>>   ##
>>>>     { 'include': 'common.json' }
>>>> +{ 'include': 'sockets.json' }
>>>>     ##
>>>>   # @set_link:
>>>> @@ -696,3 +697,76 @@
>>>>   ##
>>>>   { 'event': 'FAILOVER_NEGOTIATED',
>>>>     'data': {'device-id': 'str'} }
>>>> +
>>>> +##
>>>> +# @IPFlowSpec:
>>>> +#
>>>> +# IP flow specification.
>>>> +#
>>>> +# @protocol: Transport layer protocol like TCP/UDP, etc. The 
>>>> protocol is the
>>>> +#            string instead of enum, because it can be passed to 
>>>> getprotobyname(3)
>>>> +#            and avoid duplication with /etc/protocols.
>>> The rationale is good, but it doesn't really belong into the interface
>>> documentation.  Suggest:
>>>
>>>     # @protocol: Transport layer protocol like TCP/UDP, etc. This 
>>> will be
>>>     #            passed to getprotobyname(3).
>>>
>>>
>>>> +#
>>>> +# @object-name: The @object-name means packet handler in Qemu.


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


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


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


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


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

Thanks


>
>
> Thanks
>
> Chen
>
>
>> Dave
>>
>>>> +#
>>>> +# @source: Source address and port.
>>>> +#
>>>> +# @destination: Destination address and port.
>>>> +#
>>>> +# Since: 6.1
>>>> +##
>>>> +{ 'struct': 'IPFlowSpec',
>>>> +  'data': { '*protocol': 'str', '*object-name': 'str',
>>>> +    '*source': 'InetSocketAddressBase',
>>>> +    '*destination': 'InetSocketAddressBase' } }
>>>> +
>>>> +##
>>>> +# @colo-passthrough-add:
>>>> +#
>>>> +# Add passthrough entry IPFlowSpec to the COLO-compare instance.
>>>> +# The protocol and source/destination IP/ports are optional. if 
>>>> the user
>>>> +# only inputs part of the information, this will match all traffic.
>>> Actually, all arguments are optional.
>>>
>>> Suggest:
>>>
>>>     # Add an entry to the COLO network passthrough list.
>>>     # Absent protocol, host addresses and ports match anything.
>>>
>>> If there is more than one such list, then "to a COLO network 
>>> passthrough
>>> list" instead.
>>>
>>> Still missing then: meaning of absent @object-name.  Does it select the
>>> COLO network passthrough list, perhaps?
>>>
>>>> +#
>>>> +# Returns: Nothing on success
>>>> +#
>>>> +# Since: 6.1
>>>> +#
>>>> +# Example:
>>>> +#
>>>> +# -> { "execute": "colo-passthrough-add",
>>>> +#      "arguments": { "protocol": "tcp", "object-name": "object0",
>>>> +#      "source": {"host": "192.168.1.1", "port": "1234"},
>>>> +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
>>>> +# <- { "return": {} }
>>>> +#
>>>> +##
>>>> +{ 'command': 'colo-passthrough-add', 'boxed': true,
>>>> +     'data': 'IPFlowSpec' }
>>>> +
>>>> +##
>>>> +# @colo-passthrough-del:
>>>> +#
>>>> +# Delete passthrough entry IPFlowSpec to the COLO-compare instance.
>>>> +# The protocol and source/destination IP/ports are optional. if 
>>>> the user
>>>> +# only inputs part of the information, this will match all traffic.
>>> I suspect this command doesn't actually match traffic, it matches
>>> entries added with colo-passthrough-add.
>>>
>>> Can it delete more than one such entry?
>>>
>>> Suggest:
>>>
>>>     # Delete an entry from the COLO network passthrough list.
>>>
>>> and then explain how the command arguments select entries.
>>>
>>>> +#
>>>> +# Returns: Nothing on success
>>>> +#
>>>> +# Since: 6.1
>>>> +#
>>>> +# Example:
>>>> +#
>>>> +# -> { "execute": "colo-passthrough-del",
>>>> +#      "arguments": { "protocol": "tcp", "object-name": "object0",
>>>> +#      "source": {"host": "192.168.1.1", "port": "1234"},
>>>> +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
>>>> +# <- { "return": {} }
>>>> +#
>>>> +##
>>>> +{ 'command': 'colo-passthrough-del', 'boxed': true,
>>>> +     'data': 'IPFlowSpec' }
>
Jason Wang June 22, 2021, 7:05 a.m. UTC | #14 
在 2021/6/15 下午7:37, Zhang Chen 写道:
> Since the real user scenario does not need COLO to monitor all traffic.
> Add colo-passthrough-add and colo-passthrough-del to maintain
> a COLO network passthrough list. Add IPFlowSpec struct for all QMP commands.
> All the fields of IPFlowSpec are optional.
>
> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> ---
>   net/net.c     | 10 +++++++


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

Thanks


>   qapi/net.json | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 84 insertions(+)
>
> diff --git a/net/net.c b/net/net.c
> index 76bbb7c31b..f913e97983 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1195,6 +1195,16 @@ void qmp_netdev_del(const char *id, Error **errp)
>       }
>   }
>   
> +void qmp_colo_passthrough_add(IPFlowSpec *spec, Error **errp)
> +{
> +    /* TODO implement setup passthrough rule */
> +}
> +
> +void qmp_colo_passthrough_del(IPFlowSpec *spec, Error **errp)
> +{
> +    /* TODO implement delete passthrough rule */
> +}
> +
>   static void netfilter_print_info(Monitor *mon, NetFilterState *nf)
>   {
>       char *str;
> diff --git a/qapi/net.json b/qapi/net.json
> index 7fab2e7cd8..91f2e1495a 100644
> --- a/qapi/net.json
> +++ b/qapi/net.json
> @@ -7,6 +7,7 @@
>   ##
>   
>   { 'include': 'common.json' }
> +{ 'include': 'sockets.json' }
>   
>   ##
>   # @set_link:
> @@ -696,3 +697,76 @@
>   ##
>   { 'event': 'FAILOVER_NEGOTIATED',
>     'data': {'device-id': 'str'} }
> +
> +##
> +# @IPFlowSpec:
> +#
> +# IP flow specification.
> +#
> +# @protocol: Transport layer protocol like TCP/UDP, etc. The protocol is the
> +#            string instead of enum, because it can be passed to getprotobyname(3)
> +#            and avoid duplication with /etc/protocols.
> +#
> +# @object-name: The @object-name means packet handler in Qemu. Because not
> +#               all the network packet must pass the colo-compare module,
> +#               the net-filters are same situation. There modules attach to
> +#               netdev or chardev to work, VM can run multiple modules
> +#               at the same time. So it needs the object-name to set
> +#               the effective module.
> +#
> +# @source: Source address and port.
> +#
> +# @destination: Destination address and port.
> +#
> +# Since: 6.1
> +##
> +{ 'struct': 'IPFlowSpec',
> +  'data': { '*protocol': 'str', '*object-name': 'str',
> +    '*source': 'InetSocketAddressBase',
> +    '*destination': 'InetSocketAddressBase' } }
> +
> +##
> +# @colo-passthrough-add:
> +#
> +# Add passthrough entry IPFlowSpec to the COLO-compare instance.
> +# The protocol and source/destination IP/ports are optional. if the user
> +# only inputs part of the information, this will match all traffic.
> +#
> +# Returns: Nothing on success
> +#
> +# Since: 6.1
> +#
> +# Example:
> +#
> +# -> { "execute": "colo-passthrough-add",
> +#      "arguments": { "protocol": "tcp", "object-name": "object0",
> +#      "source": {"host": "192.168.1.1", "port": "1234"},
> +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
> +# <- { "return": {} }
> +#
> +##
> +{ 'command': 'colo-passthrough-add', 'boxed': true,
> +     'data': 'IPFlowSpec' }
> +
> +##
> +# @colo-passthrough-del:
> +#
> +# Delete passthrough entry IPFlowSpec to the COLO-compare instance.
> +# The protocol and source/destination IP/ports are optional. if the user
> +# only inputs part of the information, this will match all traffic.
> +#
> +# Returns: Nothing on success
> +#
> +# Since: 6.1
> +#
> +# Example:
> +#
> +# -> { "execute": "colo-passthrough-del",
> +#      "arguments": { "protocol": "tcp", "object-name": "object0",
> +#      "source": {"host": "192.168.1.1", "port": "1234"},
> +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
> +# <- { "return": {} }
> +#
> +##
> +{ 'command': 'colo-passthrough-del', 'boxed': true,
> +     'data': 'IPFlowSpec' }
Zhang, Chen June 22, 2021, 7:38 a.m. UTC | #15 
On 6/22/21 3:04 PM, Jason Wang wrote:
> 在 2021/6/22 下午2:01, chen.zhang@intel.com 写道:
>> On 6/21/21 7:30 PM, Dr. David Alan Gilbert wrote:
>>> * Markus Armbruster (armbru@redhat.com) wrote:
>>>> Zhang Chen <chen.zhang@intel.com> writes:
>>>>
>>>>> Since the real user scenario does not need COLO to monitor all
>>>>> traffic.
>>>>> Add colo-passthrough-add and colo-passthrough-del to maintain
>>>>> a COLO network passthrough list. Add IPFlowSpec struct for all QMP
>>>>> commands.
>>>>> All the fields of IPFlowSpec are optional.
>>>>>
>>>>> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
>>>>> ---
>>>> The QAPI schema looks good to me, but the interface documentation is
>>>> still not quite clear enough.  To make progress, I'm going to make
>>>> concrete suggestions wherever I can despite being quite clueless about
>>>> the subject matter.  Risks me writing something that's clearer, but
>>>> wrong.  Keep that in mind, please.
>>>>
>>>>>    net/net.c     | 10 +++++++
>>>>>    qapi/net.json | 74
>>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>    2 files changed, 84 insertions(+)
>>>>>
>>>>> diff --git a/net/net.c b/net/net.c
>>>>> index 76bbb7c31b..f913e97983 100644
>>>>> --- a/net/net.c
>>>>> +++ b/net/net.c
>>>>> @@ -1195,6 +1195,16 @@ void qmp_netdev_del(const char *id, Error
>>>>> **errp)
>>>>>        }
>>>>>    }
>>>>>    +void qmp_colo_passthrough_add(IPFlowSpec *spec, Error **errp)
>>>>> +{
>>>>> +    /* TODO implement setup passthrough rule */
>>>>> +}
>>>>> +
>>>>> +void qmp_colo_passthrough_del(IPFlowSpec *spec, Error **errp)
>>>>> +{
>>>>> +    /* TODO implement delete passthrough rule */
>>>>> +}
>>>>> +
>>>>>    static void netfilter_print_info(Monitor *mon, NetFilterState *nf)
>>>>>    {
>>>>>        char *str;
>>>>> diff --git a/qapi/net.json b/qapi/net.json
>>>>> index 7fab2e7cd8..91f2e1495a 100644
>>>>> --- a/qapi/net.json
>>>>> +++ b/qapi/net.json
>>>>> @@ -7,6 +7,7 @@
>>>>>    ##
>>>>>      { 'include': 'common.json' }
>>>>> +{ 'include': 'sockets.json' }
>>>>>      ##
>>>>>    # @set_link:
>>>>> @@ -696,3 +697,76 @@
>>>>>    ##
>>>>>    { 'event': 'FAILOVER_NEGOTIATED',
>>>>>      'data': {'device-id': 'str'} }
>>>>> +
>>>>> +##
>>>>> +# @IPFlowSpec:
>>>>> +#
>>>>> +# IP flow specification.
>>>>> +#
>>>>> +# @protocol: Transport layer protocol like TCP/UDP, etc. The
>>>>> protocol is the
>>>>> +#            string instead of enum, because it can be passed to
>>>>> getprotobyname(3)
>>>>> +#            and avoid duplication with /etc/protocols.
>>>> The rationale is good, but it doesn't really belong into the interface
>>>> documentation.  Suggest:
>>>>
>>>>      # @protocol: Transport layer protocol like TCP/UDP, etc. This
>>>> will be
>>>>      #            passed to getprotobyname(3).
>>>>
>>>>
>>>>> +#
>>>>> +# @object-name: The @object-name means packet handler in Qemu.
>
> I think we need clarify "packet handler" here. It does not look like a
> Qemu terminology.


OK.

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

I will add it in the next version.


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

OK. Change to:

VM can running with multi network packet processing function objects.

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

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


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

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

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

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


Thanks

Chen


>
> Thanks
>
>
>>
>> Thanks
>>
>> Chen
>>
>>
>>> Dave
>>>
>>>>> +#
>>>>> +# @source: Source address and port.
>>>>> +#
>>>>> +# @destination: Destination address and port.
>>>>> +#
>>>>> +# Since: 6.1
>>>>> +##
>>>>> +{ 'struct': 'IPFlowSpec',
>>>>> +  'data': { '*protocol': 'str', '*object-name': 'str',
>>>>> +    '*source': 'InetSocketAddressBase',
>>>>> +    '*destination': 'InetSocketAddressBase' } }
>>>>> +
>>>>> +##
>>>>> +# @colo-passthrough-add:
>>>>> +#
>>>>> +# Add passthrough entry IPFlowSpec to the COLO-compare instance.
>>>>> +# The protocol and source/destination IP/ports are optional. if
>>>>> the user
>>>>> +# only inputs part of the information, this will match all traffic.
>>>> Actually, all arguments are optional.
>>>>
>>>> Suggest:
>>>>
>>>>      # Add an entry to the COLO network passthrough list.
>>>>      # Absent protocol, host addresses and ports match anything.
>>>>
>>>> If there is more than one such list, then "to a COLO network
>>>> passthrough
>>>> list" instead.
>>>>
>>>> Still missing then: meaning of absent @object-name.  Does it select the
>>>> COLO network passthrough list, perhaps?
>>>>
>>>>> +#
>>>>> +# Returns: Nothing on success
>>>>> +#
>>>>> +# Since: 6.1
>>>>> +#
>>>>> +# Example:
>>>>> +#
>>>>> +# -> { "execute": "colo-passthrough-add",
>>>>> +#      "arguments": { "protocol": "tcp", "object-name": "object0",
>>>>> +#      "source": {"host": "192.168.1.1", "port": "1234"},
>>>>> +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
>>>>> +# <- { "return": {} }
>>>>> +#
>>>>> +##
>>>>> +{ 'command': 'colo-passthrough-add', 'boxed': true,
>>>>> +     'data': 'IPFlowSpec' }
>>>>> +
>>>>> +##
>>>>> +# @colo-passthrough-del:
>>>>> +#
>>>>> +# Delete passthrough entry IPFlowSpec to the COLO-compare instance.
>>>>> +# The protocol and source/destination IP/ports are optional. if
>>>>> the user
>>>>> +# only inputs part of the information, this will match all traffic.
>>>> I suspect this command doesn't actually match traffic, it matches
>>>> entries added with colo-passthrough-add.
>>>>
>>>> Can it delete more than one such entry?
>>>>
>>>> Suggest:
>>>>
>>>>      # Delete an entry from the COLO network passthrough list.
>>>>
>>>> and then explain how the command arguments select entries.
>>>>
>>>>> +#
>>>>> +# Returns: Nothing on success
>>>>> +#
>>>>> +# Since: 6.1
>>>>> +#
>>>>> +# Example:
>>>>> +#
>>>>> +# -> { "execute": "colo-passthrough-del",
>>>>> +#      "arguments": { "protocol": "tcp", "object-name": "object0",
>>>>> +#      "source": {"host": "192.168.1.1", "port": "1234"},
>>>>> +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
>>>>> +# <- { "return": {} }
>>>>> +#
>>>>> +##
>>>>> +{ 'command': 'colo-passthrough-del', 'boxed': true,
>>>>> +     'data': 'IPFlowSpec' }
>
Zhang, Chen June 22, 2021, 7:41 a.m. UTC | #16 
On 6/22/21 3:05 PM, Jason Wang wrote:
> 在 2021/6/15 下午7:37, Zhang Chen 写道:
>> Since the real user scenario does not need COLO to monitor all traffic.
>> Add colo-passthrough-add and colo-passthrough-del to maintain
>> a COLO network passthrough list. Add IPFlowSpec struct for all QMP commands.
>> All the fields of IPFlowSpec are optional.
>>
>> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
>> ---
>>    net/net.c     | 10 +++++++
>
> Let's avoid to have colo stuffs in the general net codes.


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


Thanks

Chen


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


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


Thanks

Chen


>
>>
>>>
>>>>>>> Because not
>>>>>>> +#               all the network packet must pass the colo-compare
>>>>>>> module,
>>>>>>> +#               the net-filters are same situation. There modules
>>>>>>> attach to
>>>>>>> +#               netdev or chardev to work, VM can run multiple
>>>>>>> modules
>>>>>>> +#               at the same time.
>>> This sentence needs some tweak since I fail to understand it's meaning.
>>>
>> OK. Change to:
>>
>> VM can running with multi network packet processing function objects.
>>
>> They can control different network data paths from netdev or chardev.
>>
>> So it needs the object-name to set the effective module.
>>
>>
>>>>>>> So it needs the object-name to set
>>>>>>> +#               the effective module.
>>>>>> I still don't understand this, and I'm too ignorant of COLO and
>>>>>> networking to suggest improvements.
>>>>>>
>>>>>> Jason or David, perhaps?
>>>>> I'll leave Jason to check on the object behaviour (and I see the
>>>>> rest of
>>>>> the thread); but at a high level, COLO is deciding whether to do VM
>>>>> synchronisation on whether the network behaviour of the two VMs get
>>>>> out
>>>>> of sync (e.g. due to randomness in the flow of requests); if you don't
>>>>> sync then when you fail-over, you'll get TCP errors/inconsistencies in
>>>>> the stream view from the secondary; but this patch series is saying
>>>>> you don't care if some TCP connections fail like that, you only care
>>>>> about maybe the main sockets the application is providing.
>>>> Yes, you are right.
>>> I wonder if it's the best to introduce colo specific command here.
>>> Instead, can we introduce commands to set and get netfilter properties?
>> This series has not added pass-through support for other netfilters,
>>
>> Can we change the qmp command to passthrough-filter-add/del as
>> markus's comments in this series,
>
> It looks more general, so I think it should work.
>
> Thanks
>
>
>> And enable filters pass-through capability and add properties in next
>> series. Step by step.
>>
>>
>> Thanks
>>
>> Chen
>>
>>
>>> Thanks
>>>
>>>
>>>> Thanks
>>>>
>>>> Chen
>>>>
>>>>
>>>>> Dave
>>>>>
>>>>>>> +#
>>>>>>> +# @source: Source address and port.
>>>>>>> +#
>>>>>>> +# @destination: Destination address and port.
>>>>>>> +#
>>>>>>> +# Since: 6.1
>>>>>>> +##
>>>>>>> +{ 'struct': 'IPFlowSpec',
>>>>>>> +  'data': { '*protocol': 'str', '*object-name': 'str',
>>>>>>> +    '*source': 'InetSocketAddressBase',
>>>>>>> +    '*destination': 'InetSocketAddressBase' } }
>>>>>>> +
>>>>>>> +##
>>>>>>> +# @colo-passthrough-add:
>>>>>>> +#
>>>>>>> +# Add passthrough entry IPFlowSpec to the COLO-compare instance.
>>>>>>> +# The protocol and source/destination IP/ports are optional. if
>>>>>>> the user
>>>>>>> +# only inputs part of the information, this will match all traffic.
>>>>>> Actually, all arguments are optional.
>>>>>>
>>>>>> Suggest:
>>>>>>
>>>>>>       # Add an entry to the COLO network passthrough list.
>>>>>>       # Absent protocol, host addresses and ports match anything.
>>>>>>
>>>>>> If there is more than one such list, then "to a COLO network
>>>>>> passthrough
>>>>>> list" instead.
>>>>>>
>>>>>> Still missing then: meaning of absent @object-name.  Does it
>>>>>> select the
>>>>>> COLO network passthrough list, perhaps?
>>>>>>
>>>>>>> +#
>>>>>>> +# Returns: Nothing on success
>>>>>>> +#
>>>>>>> +# Since: 6.1
>>>>>>> +#
>>>>>>> +# Example:
>>>>>>> +#
>>>>>>> +# -> { "execute": "colo-passthrough-add",
>>>>>>> +#      "arguments": { "protocol": "tcp", "object-name": "object0",
>>>>>>> +#      "source": {"host": "192.168.1.1", "port": "1234"},
>>>>>>> +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
>>>>>>> +# <- { "return": {} }
>>>>>>> +#
>>>>>>> +##
>>>>>>> +{ 'command': 'colo-passthrough-add', 'boxed': true,
>>>>>>> +     'data': 'IPFlowSpec' }
>>>>>>> +
>>>>>>> +##
>>>>>>> +# @colo-passthrough-del:
>>>>>>> +#
>>>>>>> +# Delete passthrough entry IPFlowSpec to the COLO-compare instance.
>>>>>>> +# The protocol and source/destination IP/ports are optional. if
>>>>>>> the user
>>>>>>> +# only inputs part of the information, this will match all traffic.
>>>>>> I suspect this command doesn't actually match traffic, it matches
>>>>>> entries added with colo-passthrough-add.
>>>>>>
>>>>>> Can it delete more than one such entry?
>>>>>>
>>>>>> Suggest:
>>>>>>
>>>>>>       # Delete an entry from the COLO network passthrough list.
>>>>>>
>>>>>> and then explain how the command arguments select entries.
>>>>>>
>>>>>>> +#
>>>>>>> +# Returns: Nothing on success
>>>>>>> +#
>>>>>>> +# Since: 6.1
>>>>>>> +#
>>>>>>> +# Example:
>>>>>>> +#
>>>>>>> +# -> { "execute": "colo-passthrough-del",
>>>>>>> +#      "arguments": { "protocol": "tcp", "object-name": "object0",
>>>>>>> +#      "source": {"host": "192.168.1.1", "port": "1234"},
>>>>>>> +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
>>>>>>> +# <- { "return": {} }
>>>>>>> +#
>>>>>>> +##
>>>>>>> +{ 'command': 'colo-passthrough-del', 'boxed': true,
>>>>>>> +     'data': 'IPFlowSpec' }
Jason Wang June 22, 2021, 8:03 a.m. UTC | #18 
在 2021/6/22 下午3:38, chen.zhang@intel.com 写道:
>
> On 6/22/21 3:04 PM, Jason Wang wrote:
>> 在 2021/6/22 下午2:01, chen.zhang@intel.com 写道:
>>> On 6/21/21 7:30 PM, Dr. David Alan Gilbert wrote:
>>>> * Markus Armbruster (armbru@redhat.com) wrote:
>>>>> Zhang Chen <chen.zhang@intel.com> writes:
>>>>>
>>>>>> Since the real user scenario does not need COLO to monitor all
>>>>>> traffic.
>>>>>> Add colo-passthrough-add and colo-passthrough-del to maintain
>>>>>> a COLO network passthrough list. Add IPFlowSpec struct for all QMP
>>>>>> commands.
>>>>>> All the fields of IPFlowSpec are optional.
>>>>>>
>>>>>> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
>>>>>> ---
>>>>> The QAPI schema looks good to me, but the interface documentation is
>>>>> still not quite clear enough.  To make progress, I'm going to make
>>>>> concrete suggestions wherever I can despite being quite clueless 
>>>>> about
>>>>> the subject matter.  Risks me writing something that's clearer, but
>>>>> wrong.  Keep that in mind, please.
>>>>>
>>>>>>    net/net.c     | 10 +++++++
>>>>>>    qapi/net.json | 74
>>>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>    2 files changed, 84 insertions(+)
>>>>>>
>>>>>> diff --git a/net/net.c b/net/net.c
>>>>>> index 76bbb7c31b..f913e97983 100644
>>>>>> --- a/net/net.c
>>>>>> +++ b/net/net.c
>>>>>> @@ -1195,6 +1195,16 @@ void qmp_netdev_del(const char *id, Error
>>>>>> **errp)
>>>>>>        }
>>>>>>    }
>>>>>>    +void qmp_colo_passthrough_add(IPFlowSpec *spec, Error **errp)
>>>>>> +{
>>>>>> +    /* TODO implement setup passthrough rule */
>>>>>> +}
>>>>>> +
>>>>>> +void qmp_colo_passthrough_del(IPFlowSpec *spec, Error **errp)
>>>>>> +{
>>>>>> +    /* TODO implement delete passthrough rule */
>>>>>> +}
>>>>>> +
>>>>>>    static void netfilter_print_info(Monitor *mon, NetFilterState 
>>>>>> *nf)
>>>>>>    {
>>>>>>        char *str;
>>>>>> diff --git a/qapi/net.json b/qapi/net.json
>>>>>> index 7fab2e7cd8..91f2e1495a 100644
>>>>>> --- a/qapi/net.json
>>>>>> +++ b/qapi/net.json
>>>>>> @@ -7,6 +7,7 @@
>>>>>>    ##
>>>>>>      { 'include': 'common.json' }
>>>>>> +{ 'include': 'sockets.json' }
>>>>>>      ##
>>>>>>    # @set_link:
>>>>>> @@ -696,3 +697,76 @@
>>>>>>    ##
>>>>>>    { 'event': 'FAILOVER_NEGOTIATED',
>>>>>>      'data': {'device-id': 'str'} }
>>>>>> +
>>>>>> +##
>>>>>> +# @IPFlowSpec:
>>>>>> +#
>>>>>> +# IP flow specification.
>>>>>> +#
>>>>>> +# @protocol: Transport layer protocol like TCP/UDP, etc. The
>>>>>> protocol is the
>>>>>> +#            string instead of enum, because it can be passed to
>>>>>> getprotobyname(3)
>>>>>> +#            and avoid duplication with /etc/protocols.
>>>>> The rationale is good, but it doesn't really belong into the 
>>>>> interface
>>>>> documentation.  Suggest:
>>>>>
>>>>>      # @protocol: Transport layer protocol like TCP/UDP, etc. This
>>>>> will be
>>>>>      #            passed to getprotobyname(3).
>>>>>
>>>>>
>>>>>> +#
>>>>>> +# @object-name: The @object-name means packet handler in Qemu.
>>
>> I think we need clarify "packet handler" here. It does not look like a
>> Qemu terminology.
>
>
> OK.
>
> The @object-name means a qemu object with network packet processing 
> function, for example colo-compare, filtr-mirror, etc.
>
> I will add it in the next version.


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


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


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

Thanks


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

Patch

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