| Message ID | 20210319035508.113741-3-chen.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Bypass specific network traffic in COLO | expand |
Zhang Chen <chen.zhang@intel.com> writes: > Add L4_Connection struct for other QMP commands. > Except protocol field is necessary, other fields are optional. > > Signed-off-by: Zhang Chen <chen.zhang@intel.com> > --- > qapi/net.json | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/qapi/net.json b/qapi/net.json > index 498ea7aa72..cd4a8ed95e 100644 > --- a/qapi/net.json > +++ b/qapi/net.json > @@ -825,3 +825,29 @@ > { 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite', > 'icmp', 'igmp', 'ipv6' ] } > > +## > +# @L4_Connection: > +# > +# Layer 4 network connection. > +# > +# Just for IPv4. > +# > +# @protocol: Transport layer protocol like TCP/UDP... > +# > +# @id: For specific module with Qemu object ID, If there is no such part, > +# it means global rules. Clear as mud. See my review of PATCH 3. > +# > +# @src_ip: Source IP. > +# > +# @dst_ip: Destination IP. > +# > +# @src_port: Source port. > +# > +# @dst_port: Destination port. > +# > +# Since: 6.1 > +## > +{ 'struct': 'L4_Connection', > + 'data': { 'protocol': 'IP_PROTOCOL', '*id': 'str', '*src_ip': 'str', '*dst_ip': 'str', > + '*src_port': 'int', '*dst_port': 'int' } } > +
One more little thing... Zhang Chen <chen.zhang@intel.com> writes: > Add L4_Connection struct for other QMP commands. > Except protocol field is necessary, other fields are optional. > > Signed-off-by: Zhang Chen <chen.zhang@intel.com> > --- > qapi/net.json | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/qapi/net.json b/qapi/net.json > index 498ea7aa72..cd4a8ed95e 100644 > --- a/qapi/net.json > +++ b/qapi/net.json > @@ -825,3 +825,29 @@ > { 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite', > 'icmp', 'igmp', 'ipv6' ] } > > +## > +# @L4_Connection: > +# > +# Layer 4 network connection. > +# > +# Just for IPv4. > +# > +# @protocol: Transport layer protocol like TCP/UDP... > +# > +# @id: For specific module with Qemu object ID, If there is no such part, > +# it means global rules. > +# > +# @src_ip: Source IP. > +# > +# @dst_ip: Destination IP. > +# > +# @src_port: Source port. > +# > +# @dst_port: Destination port. > +# > +# Since: 6.1 > +## > +{ 'struct': 'L4_Connection', > + 'data': { 'protocol': 'IP_PROTOCOL', '*id': 'str', '*src_ip': 'str', '*dst_ip': 'str', > + '*src_port': 'int', '*dst_port': 'int' } } > + Please avoid the long line, e.g. like this: { 'struct': 'L4_Connection', 'data': { 'protocol': 'IP_PROTOCOL', '*id': 'str', '*src_ip': 'str', '*dst_ip': 'str', '*src_port': 'int', '*dst_port': 'int' } }
> -----Original Message----- > From: Markus Armbruster <armbru@redhat.com> > Sent: Friday, March 19, 2021 11:48 PM > To: Zhang, Chen <chen.zhang@intel.com> > Cc: Jason Wang <jasowang@redhat.com>; qemu-dev <qemu- > devel@nongnu.org>; Eric Blake <eblake@redhat.com>; Dr. David Alan > Gilbert <dgilbert@redhat.com>; Markus Armbruster <armbru@redhat.com>; > Li Zhijian <lizhijian@cn.fujitsu.com>; Lukas Straub <lukasstraub2@web.de>; > Zhang Chen <zhangckid@gmail.com> > Subject: Re: [PATCH V4 2/7] qapi/net.json: Add L4_Connection definition > > Zhang Chen <chen.zhang@intel.com> writes: > > > Add L4_Connection struct for other QMP commands. > > Except protocol field is necessary, other fields are optional. > > > > Signed-off-by: Zhang Chen <chen.zhang@intel.com> > > --- > > qapi/net.json | 26 ++++++++++++++++++++++++++ > > 1 file changed, 26 insertions(+) > > > > diff --git a/qapi/net.json b/qapi/net.json index > > 498ea7aa72..cd4a8ed95e 100644 > > --- a/qapi/net.json > > +++ b/qapi/net.json > > @@ -825,3 +825,29 @@ > > { 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite', > > 'icmp', 'igmp', 'ipv6' ] } > > > > +## > > +# @L4_Connection: > > +# > > +# Layer 4 network connection. > > +# > > +# Just for IPv4. > > +# > > +# @protocol: Transport layer protocol like TCP/UDP... > > +# > > +# @id: For specific module with Qemu object ID, If there is no such part, > > +# it means global rules. > > Clear as mud. Sorry, let me re-clear it. If I understand correctly, The ID shouldn't be here, but I found the 'boxed' flag just can add only one 'data' like this: +## +{ 'command': 'colo-passthrough-add', 'boxed': true, + 'data': 'L4_Connection' } I original want to this: +## +{ 'command': 'colo-passthrough-add', + 'data': { 'id': 'str', 'boxed': false, 'conn': 'L4_Connection', 'boxed': true } So, I add the @id as an optional argument here. rewrite the comments: +# @id: Assign the rule to Qemu network handle module object ID. Like colo-compare, net-filter. Please see the ID details in patch3 too. Thanks Chen > > See my review of PATCH 3. > > > +# > > +# @src_ip: Source IP. > > +# > > +# @dst_ip: Destination IP. > > +# > > +# @src_port: Source port. > > +# > > +# @dst_port: Destination port. > > +# > > +# Since: 6.1 > > +## > > +{ 'struct': 'L4_Connection', > > + 'data': { 'protocol': 'IP_PROTOCOL', '*id': 'str', '*src_ip': 'str', '*dst_ip': 'str', > > + '*src_port': 'int', '*dst_port': 'int' } } > > +
"Zhang, Chen" <chen.zhang@intel.com> writes: >> -----Original Message----- >> From: Markus Armbruster <armbru@redhat.com> >> Sent: Friday, March 19, 2021 11:48 PM >> To: Zhang, Chen <chen.zhang@intel.com> >> Cc: Jason Wang <jasowang@redhat.com>; qemu-dev <qemu- >> devel@nongnu.org>; Eric Blake <eblake@redhat.com>; Dr. David Alan >> Gilbert <dgilbert@redhat.com>; Markus Armbruster <armbru@redhat.com>; >> Li Zhijian <lizhijian@cn.fujitsu.com>; Lukas Straub <lukasstraub2@web.de>; >> Zhang Chen <zhangckid@gmail.com> >> Subject: Re: [PATCH V4 2/7] qapi/net.json: Add L4_Connection definition >> >> Zhang Chen <chen.zhang@intel.com> writes: >> >> > Add L4_Connection struct for other QMP commands. >> > Except protocol field is necessary, other fields are optional. >> > >> > Signed-off-by: Zhang Chen <chen.zhang@intel.com> >> > --- >> > qapi/net.json | 26 ++++++++++++++++++++++++++ >> > 1 file changed, 26 insertions(+) >> > >> > diff --git a/qapi/net.json b/qapi/net.json index >> > 498ea7aa72..cd4a8ed95e 100644 >> > --- a/qapi/net.json >> > +++ b/qapi/net.json >> > @@ -825,3 +825,29 @@ >> > { 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite', >> > 'icmp', 'igmp', 'ipv6' ] } >> > >> > +## >> > +# @L4_Connection: >> > +# >> > +# Layer 4 network connection. >> > +# >> > +# Just for IPv4. >> > +# >> > +# @protocol: Transport layer protocol like TCP/UDP... >> > +# >> > +# @id: For specific module with Qemu object ID, If there is no such part, >> > +# it means global rules. >> >> Clear as mud. > > Sorry, let me re-clear it. > If I understand correctly, The ID shouldn't be here, but I found the 'boxed' flag just can add only one 'data' like this: > +## > +{ 'command': 'colo-passthrough-add', 'boxed': true, > + 'data': 'L4_Connection' } > > I original want to this: > +## > +{ 'command': 'colo-passthrough-add', > + 'data': { 'id': 'str', 'boxed': false, 'conn': 'L4_Connection', 'boxed': true } > > So, I add the @id as an optional argument here. > > rewrite the comments: > +# @id: Assign the rule to Qemu network handle module object ID. Like colo-compare, net-filter. > > Please see the ID details in patch3 too. So, colo-passthrough-add takes an @id argument (to be tacked onto packets to help with further processing, I understand), and arguments to match packets. Naming the argument type L4_Connection is misleading. Even naming the match arguments L4_Connection would be misleading. "Connection" has a specific meaning in networking. There are TCP connections. There is no such thing as an UDP connection. A TCP connection is uniquely identified by a pair of endpoints, i.e. by source address, source port, destination address, destination port. Same for other connection-oriented protocols. The protocol is not part of the connection. Thus, L4_Connection would be misleading even for the connection-oriented case. You need a named type for colo-passthrough-add's argument because you share it with colo-passthrough-del. I'm not sure that's what we want (I'm going to write more on that in a moment). If it is what we want, then please pick a another, descriptive name.
> -----Original Message----- > From: Markus Armbruster <armbru@redhat.com> > Sent: Monday, March 22, 2021 8:31 PM > To: Zhang, Chen <chen.zhang@intel.com> > Cc: Lukas Straub <lukasstraub2@web.de>; Li Zhijian > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; qemu- > dev <qemu-devel@nongnu.org>; Dr. David Alan Gilbert > <dgilbert@redhat.com>; Zhang Chen <zhangckid@gmail.com> > Subject: Re: [PATCH V4 2/7] qapi/net.json: Add L4_Connection definition > > "Zhang, Chen" <chen.zhang@intel.com> writes: > > >> -----Original Message----- > >> From: Markus Armbruster <armbru@redhat.com> > >> Sent: Friday, March 19, 2021 11:48 PM > >> To: Zhang, Chen <chen.zhang@intel.com> > >> Cc: Jason Wang <jasowang@redhat.com>; qemu-dev <qemu- > >> devel@nongnu.org>; Eric Blake <eblake@redhat.com>; Dr. David Alan > >> Gilbert <dgilbert@redhat.com>; Markus Armbruster > <armbru@redhat.com>; > >> Li Zhijian <lizhijian@cn.fujitsu.com>; Lukas Straub > >> <lukasstraub2@web.de>; Zhang Chen <zhangckid@gmail.com> > >> Subject: Re: [PATCH V4 2/7] qapi/net.json: Add L4_Connection > >> definition > >> > >> Zhang Chen <chen.zhang@intel.com> writes: > >> > >> > Add L4_Connection struct for other QMP commands. > >> > Except protocol field is necessary, other fields are optional. > >> > > >> > Signed-off-by: Zhang Chen <chen.zhang@intel.com> > >> > --- > >> > qapi/net.json | 26 ++++++++++++++++++++++++++ > >> > 1 file changed, 26 insertions(+) > >> > > >> > diff --git a/qapi/net.json b/qapi/net.json index > >> > 498ea7aa72..cd4a8ed95e 100644 > >> > --- a/qapi/net.json > >> > +++ b/qapi/net.json > >> > @@ -825,3 +825,29 @@ > >> > { 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite', > >> > 'icmp', 'igmp', 'ipv6' ] } > >> > > >> > +## > >> > +# @L4_Connection: > >> > +# > >> > +# Layer 4 network connection. > >> > +# > >> > +# Just for IPv4. > >> > +# > >> > +# @protocol: Transport layer protocol like TCP/UDP... > >> > +# > >> > +# @id: For specific module with Qemu object ID, If there is no such part, > >> > +# it means global rules. > >> > >> Clear as mud. > > > > Sorry, let me re-clear it. > > If I understand correctly, The ID shouldn't be here, but I found the 'boxed' > flag just can add only one 'data' like this: > > +## > > +{ 'command': 'colo-passthrough-add', 'boxed': true, > > + 'data': 'L4_Connection' } > > > > I original want to this: > > +## > > +{ 'command': 'colo-passthrough-add', > > + 'data': { 'id': 'str', 'boxed': false, 'conn': 'L4_Connection', > > +'boxed': true } > > > > So, I add the @id as an optional argument here. > > > > rewrite the comments: > > +# @id: Assign the rule to Qemu network handle module object ID. Like > colo-compare, net-filter. > > > > Please see the ID details in patch3 too. > > So, colo-passthrough-add takes an @id argument (to be tacked onto packets > to help with further processing, I understand), and arguments to match > packets. Yes. > > Naming the argument type L4_Connection is misleading. > > Even naming the match arguments L4_Connection would be misleading. > "Connection" has a specific meaning in networking. There are TCP > connections. There is no such thing as an UDP connection. > > A TCP connection is uniquely identified by a pair of endpoints, i.e. by source > address, source port, destination address, destination port. > Same for other connection-oriented protocols. The protocol is not part of > the connection. Thus, L4_Connection would be misleading even for the > connection-oriented case. > > You need a named type for colo-passthrough-add's argument because you > share it with colo-passthrough-del. I'm not sure that's what we want (I'm > going to write more on that in a moment). If it is what we want, then please > pick a another, descriptive name. What do you think the "L4BypassRule" or "NetworkRule" ? Thanks Chen
"Zhang, Chen" <chen.zhang@intel.com> writes: >> -----Original Message----- >> From: Markus Armbruster <armbru@redhat.com> [...] >> Naming the argument type L4_Connection is misleading. >> >> Even naming the match arguments L4_Connection would be misleading. >> "Connection" has a specific meaning in networking. There are TCP >> connections. There is no such thing as an UDP connection. >> >> A TCP connection is uniquely identified by a pair of endpoints, i.e. by source >> address, source port, destination address, destination port. >> Same for other connection-oriented protocols. The protocol is not part of >> the connection. Thus, L4_Connection would be misleading even for the >> connection-oriented case. >> >> You need a named type for colo-passthrough-add's argument because you >> share it with colo-passthrough-del. I'm not sure that's what we want (I'm >> going to write more on that in a moment). If it is what we want, then please >> pick a another, descriptive name. > > What do you think the "L4BypassRule" or "NetworkRule" ? NetworkRule is too generic. What about ColoPassthroughRule?
* Markus Armbruster (armbru@redhat.com) wrote: > "Zhang, Chen" <chen.zhang@intel.com> writes: > > >> -----Original Message----- > >> From: Markus Armbruster <armbru@redhat.com> > [...] > >> Naming the argument type L4_Connection is misleading. > >> > >> Even naming the match arguments L4_Connection would be misleading. > >> "Connection" has a specific meaning in networking. There are TCP > >> connections. There is no such thing as an UDP connection. > >> > >> A TCP connection is uniquely identified by a pair of endpoints, i.e. by source > >> address, source port, destination address, destination port. > >> Same for other connection-oriented protocols. The protocol is not part of > >> the connection. Thus, L4_Connection would be misleading even for the > >> connection-oriented case. > >> > >> You need a named type for colo-passthrough-add's argument because you > >> share it with colo-passthrough-del. I'm not sure that's what we want (I'm > >> going to write more on that in a moment). If it is what we want, then please > >> pick a another, descriptive name. > > > > What do you think the "L4BypassRule" or "NetworkRule" ? > > NetworkRule is too generic. > > What about ColoPassthroughRule? Which is a bit specific; there's not actually anything Colo specific in there; can I suggest 'L4FlowSpec'; I think there should also be a separate type that represents an IP address+port, so that what you end up with is: IPFlowSpec ID Protocol Source Dest Dave
> -----Original Message----- > From: Markus Armbruster <armbru@redhat.com> > Sent: Tuesday, March 23, 2021 5:55 PM > To: Zhang, Chen <chen.zhang@intel.com> > Cc: Lukas Straub <lukasstraub2@web.de>; Li Zhijian > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; qemu- > dev <qemu-devel@nongnu.org>; Dr. David Alan Gilbert > <dgilbert@redhat.com>; Zhang Chen <zhangckid@gmail.com> > Subject: Re: [PATCH V4 2/7] qapi/net.json: Add L4_Connection definition > > "Zhang, Chen" <chen.zhang@intel.com> writes: > > >> -----Original Message----- > >> From: Markus Armbruster <armbru@redhat.com> > [...] > >> Naming the argument type L4_Connection is misleading. > >> > >> Even naming the match arguments L4_Connection would be misleading. > >> "Connection" has a specific meaning in networking. There are TCP > >> connections. There is no such thing as an UDP connection. > >> > >> A TCP connection is uniquely identified by a pair of endpoints, i.e. > >> by source address, source port, destination address, destination port. > >> Same for other connection-oriented protocols. The protocol is not > >> part of the connection. Thus, L4_Connection would be misleading even > >> for the connection-oriented case. > >> > >> You need a named type for colo-passthrough-add's argument because > you > >> share it with colo-passthrough-del. I'm not sure that's what we want > >> (I'm going to write more on that in a moment). If it is what we > >> want, then please pick a another, descriptive name. > > > > What do you think the "L4BypassRule" or "NetworkRule" ? > > NetworkRule is too generic. > > What about ColoPassthroughRule? It can be used by net filter modules(filter mirror,filter-dump....) in the future, that's not just for COLO. PassthroughRule is better? Thanks Chen
"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes: > * Markus Armbruster (armbru@redhat.com) wrote: >> "Zhang, Chen" <chen.zhang@intel.com> writes: >> >> >> -----Original Message----- >> >> From: Markus Armbruster <armbru@redhat.com> >> [...] >> >> Naming the argument type L4_Connection is misleading. >> >> >> >> Even naming the match arguments L4_Connection would be misleading. >> >> "Connection" has a specific meaning in networking. There are TCP >> >> connections. There is no such thing as an UDP connection. >> >> >> >> A TCP connection is uniquely identified by a pair of endpoints, i.e. by source >> >> address, source port, destination address, destination port. >> >> Same for other connection-oriented protocols. The protocol is not part of >> >> the connection. Thus, L4_Connection would be misleading even for the >> >> connection-oriented case. >> >> >> >> You need a named type for colo-passthrough-add's argument because you >> >> share it with colo-passthrough-del. I'm not sure that's what we want (I'm >> >> going to write more on that in a moment). If it is what we want, then please >> >> pick a another, descriptive name. >> > >> > What do you think the "L4BypassRule" or "NetworkRule" ? >> >> NetworkRule is too generic. >> >> What about ColoPassthroughRule? > > Which is a bit specific; there's not actually anything Colo specific in > there; can I suggest 'L4FlowSpec'; "A bit too specific" is mostly harmless, since we can rename types at any time (they are not visible in external interfaces). This is *not* an objection to less specific names. All I want is names that don't give me wrong ideas on the thing's purpose. L4FlowSpec and IPFlowSpec (below) feel fine in that regard. > I think there should also beb > a separate type that represents an IP address+port, so that what you end > up with is: > > IPFlowSpec > ID > Protocol > Source > Dest I understand the motivation. Three drawbacks, though. One, it gets us another level of nesting on the wire, i.e. something like {"source": {"address": SRC-ADDR, "port": SRC-PORT}, "destination": {"address": DST-ADDR, "port": DST-PORT}} instead of {"source-address": SRC-ADDR, "source-port": SRC-PORT, "destination-address": DST-ADDR, "destination-port": DST-PORT} QMP clients shouldn't care. Two, we have many (address, port) pairs in the schema that don't use nesting. Adding nesting sometimes makes QMP less consistent. Three, human-friendly interface wrappers tend to dislike nesting. This particular case seems okay; we end up with dotted keys like source.address instead of source-address. In a case where we need just one (address, port), we'd get some-silly-name.address instead of just address, though. I've occasionally felt a mild need for letting me say "this struct member should be unboxed on the wire", i.e. have its curlies peeled off. Never enough to justify the additional generator complexity, though.
Markus Armbruster <armbru@redhat.com> writes: > "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes: [...] >> I think there should also beb >> a separate type that represents an IP address+port, so that what you end >> up with is: >> >> IPFlowSpec >> ID >> Protocol >> Source >> Dest > > I understand the motivation. Three drawbacks, though. > > One, it gets us another level of nesting on the wire, i.e. something > like > > {"source": {"address": SRC-ADDR, "port": SRC-PORT}, > "destination": {"address": DST-ADDR, "port": DST-PORT}} > > instead of > > {"source-address": SRC-ADDR, "source-port": SRC-PORT, > "destination-address": DST-ADDR, "destination-port": DST-PORT} > > QMP clients shouldn't care. > > Two, we have many (address, port) pairs in the schema that don't use > nesting. Adding nesting sometimes makes QMP less consistent. > > Three, human-friendly interface wrappers tend to dislike nesting. This > particular case seems okay; we end up with dotted keys like > source.address instead of source-address. In a case where we need just > one (address, port), we'd get some-silly-name.address instead of just > address, though. > > I've occasionally felt a mild need for letting me say "this struct > member should be unboxed on the wire", i.e. have its curlies peeled off. > Never enough to justify the additional generator complexity, though. Just remembered we actually have ## # @InetSocketAddressBase: # # @host: host part of the address # @port: port part of the address ## { 'struct': 'InetSocketAddressBase', 'data': { 'host': 'str', 'port': 'str' } } It's from commit eb87203b64 "rbd: Reject -blockdev server.*.{numeric, to, ipv4, ipv6}".
Zhang Chen <chen.zhang@intel.com> writes: > Add L4_Connection struct for other QMP commands. > Except protocol field is necessary, other fields are optional. > > Signed-off-by: Zhang Chen <chen.zhang@intel.com> > --- > qapi/net.json | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/qapi/net.json b/qapi/net.json > index 498ea7aa72..cd4a8ed95e 100644 > --- a/qapi/net.json > +++ b/qapi/net.json > @@ -825,3 +825,29 @@ > { 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite', > 'icmp', 'igmp', 'ipv6' ] } > > +## > +# @L4_Connection: > +# > +# Layer 4 network connection. > +# > +# Just for IPv4. > +# > +# @protocol: Transport layer protocol like TCP/UDP... > +# > +# @id: For specific module with Qemu object ID, If there is no such part, > +# it means global rules. > +# > +# @src_ip: Source IP. > +# > +# @dst_ip: Destination IP. > +# > +# @src_port: Source port. > +# > +# @dst_port: Destination port. > +# > +# Since: 6.1 > +## > +{ 'struct': 'L4_Connection', > + 'data': { 'protocol': 'IP_PROTOCOL', '*id': 'str', '*src_ip': 'str', '*dst_ip': 'str', > + '*src_port': 'int', '*dst_port': 'int' } } > + Please use '-' instead of '_' in member names. Patches to enforce this rule just landed in master. We tend to avoid abbreviations like src and dst in QMP. Consider spelling them out, except when it makes things less consistent.
> -----Original Message----- > From: Markus Armbruster <armbru@redhat.com> > Sent: Wednesday, March 24, 2021 2:47 PM > To: Dr. David Alan Gilbert <dgilbert@redhat.com> > Cc: Lukas Straub <lukasstraub2@web.de>; Li Zhijian > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; qemu- > dev <qemu-devel@nongnu.org>; Zhang, Chen <chen.zhang@intel.com>; > Zhang Chen <zhangckid@gmail.com> > Subject: Re: [PATCH V4 2/7] qapi/net.json: Add L4_Connection definition > > "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes: > > > * Markus Armbruster (armbru@redhat.com) wrote: > >> "Zhang, Chen" <chen.zhang@intel.com> writes: > >> > >> >> -----Original Message----- > >> >> From: Markus Armbruster <armbru@redhat.com> > >> [...] > >> >> Naming the argument type L4_Connection is misleading. > >> >> > >> >> Even naming the match arguments L4_Connection would be > misleading. > >> >> "Connection" has a specific meaning in networking. There are TCP > >> >> connections. There is no such thing as an UDP connection. > >> >> > >> >> A TCP connection is uniquely identified by a pair of endpoints, > >> >> i.e. by source address, source port, destination address, destination > port. > >> >> Same for other connection-oriented protocols. The protocol is not > >> >> part of the connection. Thus, L4_Connection would be misleading > >> >> even for the connection-oriented case. > >> >> > >> >> You need a named type for colo-passthrough-add's argument because > >> >> you share it with colo-passthrough-del. I'm not sure that's what > >> >> we want (I'm going to write more on that in a moment). If it is > >> >> what we want, then please pick a another, descriptive name. > >> > > >> > What do you think the "L4BypassRule" or "NetworkRule" ? > >> > >> NetworkRule is too generic. > >> > >> What about ColoPassthroughRule? > > > > Which is a bit specific; there's not actually anything Colo specific > > in there; can I suggest 'L4FlowSpec'; > > "A bit too specific" is mostly harmless, since we can rename types at any time > (they are not visible in external interfaces). > > This is *not* an objection to less specific names. All I want is names that > don't give me wrong ideas on the thing's purpose. L4FlowSpec and > IPFlowSpec (below) feel fine in that regard. > > > I think there should also beb a > > separate type that represents an IP address+port, so that what you end > > up with is: > > > > IPFlowSpec > > ID > > Protocol > > Source > > Dest > > I understand the motivation. Three drawbacks, though. > > One, it gets us another level of nesting on the wire, i.e. something like > > {"source": {"address": SRC-ADDR, "port": SRC-PORT}, > "destination": {"address": DST-ADDR, "port": DST-PORT}} > > instead of > > {"source-address": SRC-ADDR, "source-port": SRC-PORT, > "destination-address": DST-ADDR, "destination-port": DST-PORT} > > QMP clients shouldn't care. > > Two, we have many (address, port) pairs in the schema that don't use > nesting. Adding nesting sometimes makes QMP less consistent. > > Three, human-friendly interface wrappers tend to dislike nesting. This > particular case seems okay; we end up with dotted keys like source.address > instead of source-address. In a case where we need just one (address, port), > we'd get some-silly-name.address instead of just address, though. > > I've occasionally felt a mild need for letting me say "this struct member > should be unboxed on the wire", i.e. have its curlies peeled off. > Never enough to justify the additional generator complexity, though. The initial patch of this series used unboxed struct, Eric's comments is change it to boxed. I think it's OK, for the unused field we can keep 0 for it. The n-tuple(src IP, dst IP, src port, dst port, protocol) will be used in many place on Qemu network related code(like migrate, NBD....). For the name, I think Dave's comments is well, for the @InetSocketAddressBase we can remove it and change it to use IPFlowSpec. Markus, what do you think about it? Thanks Chen
diff --git a/qapi/net.json b/qapi/net.json index 498ea7aa72..cd4a8ed95e 100644 --- a/qapi/net.json +++ b/qapi/net.json @@ -825,3 +825,29 @@ { 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite', 'icmp', 'igmp', 'ipv6' ] } +## +# @L4_Connection: +# +# Layer 4 network connection. +# +# Just for IPv4. +# +# @protocol: Transport layer protocol like TCP/UDP... +# +# @id: For specific module with Qemu object ID, If there is no such part, +# it means global rules. +# +# @src_ip: Source IP. +# +# @dst_ip: Destination IP. +# +# @src_port: Source port. +# +# @dst_port: Destination port. +# +# Since: 6.1 +## +{ 'struct': 'L4_Connection', + 'data': { 'protocol': 'IP_PROTOCOL', '*id': 'str', '*src_ip': 'str', '*dst_ip': 'str', + '*src_port': 'int', '*dst_port': 'int' } } +
Add L4_Connection struct for other QMP commands. Except protocol field is necessary, other fields are optional. Signed-off-by: Zhang Chen <chen.zhang@intel.com> --- qapi/net.json | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)