> -----Original Message-----
> From: Markus Armbruster <arm...@redhat.com>
> Sent: Saturday, August 7, 2021 7:32 PM
> To: Zhang, Chen <chen.zh...@intel.com>
> Cc: Jason Wang <jasow...@redhat.com>; Eric Blake <ebl...@redhat.com>;
> Dr. David Alan Gilbert <dgilb...@redhat.com>; qemu-dev <qemu-
> de...@nongnu.org>; Daniel P.Berrangé <berra...@redhat.com>; Gerd
> Hoffmann <kra...@redhat.com>; Li Zhijian <lizhij...@cn.fujitsu.com>; Lukas
> Straub <lukasstra...@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.zh...@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.zh...@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' }
