RE: [PULL V3 for 6.2 1/6] qapi/net: Add IPFlowSpec and QMP command for filter passthrough

[Zhang, Chen]

> -----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' }