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

[Markus Armbruster <armbru@redhat.com>]

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.

[...]