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.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.
> +#
> +# @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' }
