diff mbox series

[V4,2/7] qapi/net.json: Add L4_Connection definition

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

Commit Message

chen.zhang@intel.com March 19, 2021, 3:55 a.m. UTC
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(+)

Comments

Markus Armbruster March 19, 2021, 3:48 p.m. UTC | #1
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' } }
> +
Markus Armbruster March 19, 2021, 3:53 p.m. UTC | #2
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' } }
chen.zhang@intel.com March 22, 2021, 10 a.m. UTC | #3
> -----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' } }
> > +
Markus Armbruster March 22, 2021, 12:31 p.m. UTC | #4
"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.
chen.zhang@intel.com March 23, 2021, 9:06 a.m. UTC | #5
> -----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
Markus Armbruster March 23, 2021, 9:54 a.m. UTC | #6
"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?
Dr. David Alan Gilbert March 23, 2021, 8:14 p.m. UTC | #7
* 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
chen.zhang@intel.com March 24, 2021, 12:59 a.m. UTC | #8
> -----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
Markus Armbruster March 24, 2021, 6:47 a.m. UTC | #9
"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 March 24, 2021, 6:51 a.m. UTC | #10
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}".
Markus Armbruster March 24, 2021, 6:56 a.m. UTC | #11
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.
chen.zhang@intel.com March 26, 2021, 2:27 a.m. UTC | #12
> -----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 mbox series

Patch

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' } }
+