Re: [PATCH for 7.0 V10 1/6] qapi/net: Add IPFlowSpec and QMP command for filter passthrough

[Markus Armbruster]

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.

This is too much "how", and not enough "why".

Less "how": drop everything after "Add IPFlowSpec ..." sentence.

More "why": I guess you tried with "Since the real user scenario does
not need to monitor all traffic."  I don't understand.  Possible because
it's not a sentence :)  What problem of interest are you trying to solve
with this patch?  How do the QMP commands added in this patch help solve
it?  Keep it brief.

> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> ---
>  net/net.c     | 10 +++++++
>  qapi/net.json | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 83 insertions(+)
>
> diff --git a/net/net.c b/net/net.c
> index f0d14dbfc1..5d0d5914fb 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -1215,6 +1215,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..5194aedcf5 100644
> --- a/qapi/net.json
> +++ b/qapi/net.json
> @@ -7,6 +7,7 @@
>  ##
>  
>  { 'include': 'common.json' }
> +{ 'include': 'sockets.json' }
>  
>  ##
>  # @set_link:
> @@ -696,3 +697,75 @@
>  ##
>  { 'event': 'FAILOVER_NEGOTIATED',
>    'data': {'device-id': 'str'} }
> +
> +##
> +# @IPFlowSpec:
> +#
> +# IP flow specification.
> +#
> +# @protocol: Transport layer protocol like TCP/UDP, etc. This will be
> +#            passed to getprotobyname(3).
> +#
> +# @object-name: The @object-name means a QEMU object with network
> +#               packet processing function, for example colo-compare,
> +#               filter-redirector, filter-mirror, etc. QOM path to
> +#               a QOM object that implements their own passthrough
> +#               work in the original data processing flow. What is
> +#               exposed to the outside world is an operable
> +#               passthrough list.

I don't like @object-name, because it's a *path*, not a name.  We use
@qom-path elsewhere.

The documentation still makes no sense to me.  I suggested something
like

    # @qom-path: An object that implements the MUMBLE interface.

This assumes the thing the valid objects have in common is a QOM
interface (whose name I don't know, so I used MUMBLE).  If this is
incorrect, then replace "that implements..." by what they do have in
common.

Giving these kinds of objects a name will make the remainder of the
documentation *much* easier to write.  I can't, because I really have no
idea what this is all about, so I'll use MUMBLE as a placeholder name.

> +#
> +# @source: Source address and port.
> +#
> +# @destination: Destination address and port.
> +#
> +# Since: 7.0
> +##
> +{ 'struct': 'IPFlowSpec',
> +  'data': { '*protocol': 'str', 'object-name': 'str',
> +    '*source': 'InetSocketAddressBase',
> +    '*destination': 'InetSocketAddressBase' } }
> +
> +##
> +# @passthrough-filter-add:
> +#
> +# Add an entry to the QOM object own network passthrough list.

"to a MUMBLE object's network passthrough list"

> +# Absent protocol, host addresses and ports match anything.
> +#
> +# Returns: Nothing on success
> +#
> +# Since: 7.0
> +#
> +# 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 an entry from the QOM object own network passthrough list.

"from a MUMBLE object's 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.  Have you
considered this?

> +#
> +# Returns: Nothing on success
> +#
> +# Since: 7.0
> +#
> +# 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' }