| Message ID | 20210719090051.3824672-2-chen.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PULL,V3,for,6.2,1/6] qapi/net: Add IPFlowSpec and QMP command for filter passthrough | expand |
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.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 > +## > +{ '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' }
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. [...]
> -----Original Message----- > From: Markus Armbruster <armbru@redhat.com> > Sent: Saturday, August 7, 2021 7:32 PM > To: Zhang, Chen <chen.zhang@intel.com> > Cc: Jason Wang <jasowang@redhat.com>; Eric Blake <eblake@redhat.com>; > Dr. David Alan Gilbert <dgilbert@redhat.com>; qemu-dev <qemu- > devel@nongnu.org>; Daniel P.Berrangé <berrange@redhat.com>; Gerd > Hoffmann <kraxel@redhat.com>; Li Zhijian <lizhijian@cn.fujitsu.com>; Lukas > Straub <lukasstraub2@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.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. 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' }
diff --git a/net/net.c b/net/net.c index 76bbb7c31b..00f2be7a58 100644 --- a/net/net.c +++ b/net/net.c @@ -1195,6 +1195,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..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. +# +# @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 +# 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. +# +# @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 +# 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 +# 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' }
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> --- net/net.c | 10 +++++++ qapi/net.json | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+)