[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PULL V3 for 6.2 1/6] qapi/net: Add IPFlowSpec and QMP command for f
From: |
Markus Armbruster |
Subject: |
Re: [PULL V3 for 6.2 1/6] qapi/net: Add IPFlowSpec and QMP command for filter passthrough |
Date: |
Sat, 07 Aug 2021 14:08:10 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) |
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.
[...]