Since the real user scenario does not need to monitor all traffic.
Add passthrough-filter-add and passthrough-filter-del to maintain
a network passthrough list in object with network packet processing
function. Add IPFlowSpec struct for all QMP commands.
Most the fields of IPFlowSpec are optional,except object-name.

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

diff --git a/net/net.c b/net/net.c
index 76bbb7c31b..00f2be7a58 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1195,6 +1195,16 @@ void qmp_netdev_del(const char *id, Error **errp)
     }
 }
 
+void qmp_passthrough_filter_add(IPFlowSpec *spec, Error **errp)
+{
+    /* TODO implement setup passthrough rule */
+}
+
+void qmp_passthrough_filter_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..bfe38faab5 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -7,6 +7,7 @@
 ##
 
 { 'include': 'common.json' }
+{ 'include': 'sockets.json' }
 
 ##
 # @set_link:
@@ -696,3 +697,80 @@
 ##
 { '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 a qemu object with network packet
+#               processing function, for example colo-compare, filtr-redirector
+#               filtr-mirror, etc. 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.
+#
+# @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' } }
+
+##
+# @passthrough-filter-add:
+#
+# Add passthrough entry IPFlowSpec to a qemu object with network packet
+# processing function, for example filtr-mirror, COLO-compare, etc.
+# The object-name is necessary. The protocol and source/destination IP and
+# source/destination ports are optional. if only inputs part of the
+# information, it will match all traffic.
+#
+# Returns: Nothing on success
+#
+# Since: 6.1
+#
+# Example:
+#
+# -> { "execute": "passthrough-filter-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': 'passthrough-filter-add', 'boxed': true,
+     'data': 'IPFlowSpec' }
+
+##
+# @passthrough-filter-del:
+#
+# Delete passthrough entry IPFlowSpec to a qemu object with network packet
+# processing function, for example filtr-mirror, COLO-compare, etc.
+# The object-name is necessary. The protocol and source/destination IP and
+# source/destination ports are optional. if only inputs part of the
+# information, only the exact same rule will be deleted.
+#
+# Returns: Nothing on success
+#
+# Since: 6.1
+#
+# Example:
+#
+# -> { "execute": "passthrough-filter-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': 'passthrough-filter-del', 'boxed': true,
+     'data': 'IPFlowSpec' }
-- 
2.25.1

I see Jason queued this while I was failing at keeping up with review.
I apologize for dropping the ball.

There still are a few unresolved issues I raised in prior review.  The
documentation is not ready, and there is no consensus on the design of
passthrough-filter-del.

If we merge this as is for 6.2, then I want my review to be addressed on
top.

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

> Since the real user scenario does not need to monitor all traffic.
> Add passthrough-filter-add and passthrough-filter-del to maintain
> a network passthrough list in object with network packet processing
> function. Add IPFlowSpec struct for all QMP commands.
> Most the fields of IPFlowSpec are optional,except object-name.
>
> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> ---

[...]

> diff --git a/qapi/net.json b/qapi/net.json
> index 7fab2e7cd8..bfe38faab5 100644
> --- a/qapi/net.json
> +++ b/qapi/net.json
> @@ -7,6 +7,7 @@
>  ##
>  
>  { 'include': 'common.json' }
> +{ 'include': 'sockets.json' }
>  
>  ##
>  # @set_link:
> @@ -696,3 +697,80 @@
>  ##
>  { '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.

In review of v8, I wrote:

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

to which you replied "OK."  Please apply the change.

> +#
> +# @object-name: The @object-name means a qemu object with network packet
> +#               processing function, for example colo-compare, filtr-redirector
> +#               filtr-mirror, etc. VM can running with multi network packet

s/qemu/QEMU/

s/filtr/filter/ two times here, and more below.

s/VM/The VM/

s/multi/multiple/

Also, limit doc comment line length to 70 characters or so.

> +#               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.

Again, this is rationale, which doesn't really belong here.

What does belong here, but isn't: meaning of @object-name, i.e. how it
is resolved to a "qemu object with network packet processing function",
whatever that is.

I'm *guessing* it's the QOM path of a QOM object that provides a certain
interface.  Correct?

If yes, which interface exactly?  Is it a QOM interface?

The comment could then look like

  # QOM path to a QOM object that implements the MUMBLE interface.

with the details corrected / fleshed out.

> +#
> +# @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' } }
> +
> +##
> +# @passthrough-filter-add:
> +#
> +# Add passthrough entry IPFlowSpec to a qemu object with network packet
> +# processing function, for example filtr-mirror, COLO-compare, etc.
> +# The object-name is necessary. The protocol and source/destination IP and
> +# source/destination ports are optional. if only inputs part of the

Start your sentences with a capital letter, please.

More importantly, the paragraph is confusing.  I suggested

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

> +# information, it will match all traffic.
> +#
> +# Returns: Nothing on success
> +#
> +# Since: 6.1
> +#
> +# Example:
> +#
> +# -> { "execute": "passthrough-filter-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': 'passthrough-filter-add', 'boxed': true,
> +     'data': 'IPFlowSpec' }
> +
> +##
> +# @passthrough-filter-del:
> +#
> +# Delete passthrough entry IPFlowSpec to a qemu object with network packet
> +# processing function, for example filtr-mirror, COLO-compare, etc.
> +# The object-name is necessary. The protocol and source/destination IP and
> +# source/destination ports are optional. if only inputs part of the

Likewise.  I suggested

   # Delete an entry from the COLO network passthrough list.

as first sentence.  The remainder needs to explain how the arguments are
used to select the entry to delete.  Something like

   # Deletes the entry with exactly this protocol, host addresses and
   # ports.
     
assuming that's what it actually does.

I reiterate my strong dislike for selecting the object to delete with a
pattern match.  The common way to refer to objects is by ID.

> +# information, only the exact same rule will be deleted.
> +#
> +# Returns: Nothing on success
> +#
> +# Since: 6.1
> +#
> +# Example:
> +#
> +# -> { "execute": "passthrough-filter-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': 'passthrough-filter-del', 'boxed': true,
> +     'data': 'IPFlowSpec' }

> -----Original Message-----
> From: Markus Armbruster <armbru@redhat.com>
> Sent: Saturday, August 7, 2021 7:32 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: Jason Wang <jasowang@redhat.com>; Eric Blake <eblake@redhat.com>;
> Dr. David Alan Gilbert <dgilbert@redhat.com>; qemu-dev <qemu-
> devel@nongnu.org>; Daniel P.Berrangé <berrange@redhat.com>; Gerd
> Hoffmann <kraxel@redhat.com>; Li Zhijian <lizhijian@cn.fujitsu.com>; Lukas
> Straub <lukasstraub2@web.de>
> Subject: Re: [PULL V3 for 6.2 1/6] qapi/net: Add IPFlowSpec and QMP
> command for filter passthrough
> 
> I see Jason queued this while I was failing at keeping up with review.
> I apologize for dropping the ball.
> 
> There still are a few unresolved issues I raised in prior review.  The
> documentation is not ready, and there is no consensus on the design of
> passthrough-filter-del.
> 
> If we merge this as is for 6.2, then I want my review to be addressed on top.

OK, please hold the ball and let me know if I missed something.
I will try to do this well.

> 
> Zhang Chen <chen.zhang@intel.com> writes:
> 
> > Since the real user scenario does not need to monitor all traffic.
> > Add passthrough-filter-add and passthrough-filter-del to maintain a
> > network passthrough list in object with network packet processing
> > function. Add IPFlowSpec struct for all QMP commands.
> > Most the fields of IPFlowSpec are optional,except object-name.
> >
> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> > ---
> 
> [...]
> 
> > diff --git a/qapi/net.json b/qapi/net.json index
> > 7fab2e7cd8..bfe38faab5 100644
> > --- a/qapi/net.json
> > +++ b/qapi/net.json
> > @@ -7,6 +7,7 @@
> >  ##
> >
> >  { 'include': 'common.json' }
> > +{ 'include': 'sockets.json' }
> >
> >  ##
> >  # @set_link:
> > @@ -696,3 +697,80 @@
> >  ##
> >  { '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.
> 
> In review of v8, I wrote:
> 
>     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).
> 
> to which you replied "OK."  Please apply the change.

Sorry, I missed it.
I thought that more comments would make it clearer and avoid
misunderstandings like Eric Blake comments why not enum in V7.
I will change it as your comments here.

> 
> > +#
> > +# @object-name: The @object-name means a qemu object with network
> packet
> > +#               processing function, for example colo-compare, filtr-redirector
> > +#               filtr-mirror, etc. VM can running with multi network packet
> 
> s/qemu/QEMU/
> 
> s/filtr/filter/ two times here, and more below.
> 
> s/VM/The VM/
> 
> s/multi/multiple/
> 
> Also, limit doc comment line length to 70 characters or so.
> 

OK, I will fix it.

> > +#               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.
> 
> Again, this is rationale, which doesn't really belong here.
> 
> What does belong here, but isn't: meaning of @object-name, i.e. how it is
> resolved to a "qemu object with network packet processing function",
> whatever that is.
> 
> I'm *guessing* it's the QOM path of a QOM object that provides a certain
> interface.  Correct?
> 
> If yes, which interface exactly?  Is it a QOM interface?
> 
> The comment could then look like
> 
>   # QOM path to a QOM object that implements the MUMBLE interface.
> 
> with the details corrected / fleshed out.

Yes, the QOM object need to maintain/apply their own passthrough list while working.
I will remove original comments and change it to:

#    QOM path to a QOM object that implements their own passthrough work in
#    the original data processing flow. What is exposed to the outside is an operable
#    passthrough list.


> 
> > +#
> > +# @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' } }
> > +
> > +##
> > +# @passthrough-filter-add:
> > +#
> > +# Add passthrough entry IPFlowSpec to a qemu object with network
> > +packet # processing function, for example filtr-mirror, COLO-compare, etc.
> > +# The object-name is necessary. The protocol and source/destination
> > +IP and # source/destination ports are optional. if only inputs part
> > +of the
> 
> Start your sentences with a capital letter, please.
> 
> More importantly, the paragraph is confusing.  I suggested
> 
>    # Add an entry to the COLO network passthrough list.
>    # Absent protocol, host addresses and ports match anything.

Current passthrough command is not bind with COLO.
How about this:

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

> 
> > +# information, it will match all traffic.
> > +#
> > +# Returns: Nothing on success
> > +#
> > +# Since: 6.1
> > +#
> > +# Example:
> > +#
> > +# -> { "execute": "passthrough-filter-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': 'passthrough-filter-add', 'boxed': true,
> > +     'data': 'IPFlowSpec' }
> > +
> > +##
> > +# @passthrough-filter-del:
> > +#
> > +# Delete passthrough entry IPFlowSpec to a qemu object with network
> > +packet # processing function, for example filtr-mirror, COLO-compare, etc.
> > +# The object-name is necessary. The protocol and source/destination
> > +IP and # source/destination ports are optional. if only inputs part
> > +of the
> 
> Likewise.  I suggested
> 
>    # Delete an entry from the COLO network passthrough list.
> 
> as first sentence.  The remainder needs to explain how the arguments are
> used to select the entry to delete.  Something like
> 
>    # Deletes the entry with exactly this protocol, host addresses and
>    # ports.
> 
> assuming that's what it actually does.
> 

You are right.

Change it to:
  # Delete an entry from the QOM object own network
  # passthrough list. Deletes the entry with exactly this 
  # protocol, host addresses and ports.

> I reiterate my strong dislike for selecting the object to delete with a pattern
> match.  The common way to refer to objects is by ID.

The QOM object is very like iptables:
iptables -A INPUT -s 127.0.0.1 -d 127.0.0.1 -j ACCEPT
iptables -D INPUT -s 127.0.0.1 -d 127.0.0.1 -j ACCEPT


> 
> > +# information, only the exact same rule will be deleted.
> > +#
> > +# Returns: Nothing on success
> > +#
> > +# Since: 6.1

Will change to 6.2 tag.

Finally, thank you very much for your suggestions.
I also want to get your reviewed-by on QAPI, but V9 to Pull V3 I lost your voice.
If current reply is OK for you, I will send the V10 for Qemu 6.2.

Thanks
Chen


> > +#
> > +# Example:
> > +#
> > +# -> { "execute": "passthrough-filter-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': 'passthrough-filter-del', 'boxed': true,
> > +     'data': 'IPFlowSpec' }

Markus Armbruster <armbru@redhat.com> writes:

> I see Jason queued this while I was failing at keeping up with review.
> I apologize for dropping the ball.
>
> There still are a few unresolved issues I raised in prior review.  The
> documentation is not ready, and there is no consensus on the design of
> passthrough-filter-del.
>
> If we merge this as is for 6.2, then I want my review to be addressed on
> top.

One more thing...

> Zhang Chen <chen.zhang@intel.com> writes:
>
>> Since the real user scenario does not need to monitor all traffic.
>> Add passthrough-filter-add and passthrough-filter-del to maintain
>> a network passthrough list in object with network packet processing
>> function. Add IPFlowSpec struct for all QMP commands.
>> Most the fields of IPFlowSpec are optional,except object-name.
>>
>> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
>> ---
>
> [...]
>
>> diff --git a/qapi/net.json b/qapi/net.json
>> index 7fab2e7cd8..bfe38faab5 100644
>> --- a/qapi/net.json
>> +++ b/qapi/net.json
>> @@ -7,6 +7,7 @@
>>  ##
>>  
>>  { 'include': 'common.json' }
>> +{ 'include': 'sockets.json' }
>>  
>>  ##
>>  # @set_link:
>> @@ -696,3 +697,80 @@
>>  ##
>>  { '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.
>
> In review of v8, I wrote:
>
>     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).
>
> to which you replied "OK."  Please apply the change.
>
>> +#
>> +# @object-name: The @object-name means a qemu object with network packet
>> +#               processing function, for example colo-compare, filtr-redirector
>> +#               filtr-mirror, etc. VM can running with multi network packet
>
> s/qemu/QEMU/
>
> s/filtr/filter/ two times here, and more below.
>
> s/VM/The VM/
>
> s/multi/multiple/
>
> Also, limit doc comment line length to 70 characters or so.
>
>> +#               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.
>
> Again, this is rationale, which doesn't really belong here.
>
> What does belong here, but isn't: meaning of @object-name, i.e. how it
> is resolved to a "qemu object with network packet processing function",
> whatever that is.
>
> I'm *guessing* it's the QOM path of a QOM object that provides a certain
> interface.  Correct?
>
> If yes, which interface exactly?  Is it a QOM interface?
>
> The comment could then look like
>
>   # QOM path to a QOM object that implements the MUMBLE interface.
>
> with the details corrected / fleshed out.
>
>> +#
>> +# @source: Source address and port.
>> +#
>> +# @destination: Destination address and port.
>> +#
>> +# Since: 6.1

6.2 now.  More of the same below.

[...]