diff mbox series

[V4,1/7] qapi/net.json: Add IP_PROTOCOL definition

Message ID 20210319035508.113741-2-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 IP_PROTOCOL as enum include TCP,UDP, ICMP... for other QMP commands.

Signed-off-by: Zhang Chen <chen.zhang@intel.com>
---
 qapi/net.json | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

Comments

Markus Armbruster March 19, 2021, 3:46 p.m. UTC | #1
Zhang Chen <chen.zhang@intel.com> writes:

> Add IP_PROTOCOL as enum include TCP,UDP, ICMP... for other QMP commands.
>
> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> ---
>  qapi/net.json | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>
> diff --git a/qapi/net.json b/qapi/net.json
> index 87361ebd9a..498ea7aa72 100644
> --- a/qapi/net.json
> +++ b/qapi/net.json
> @@ -794,3 +794,34 @@
>  #
>  ##
>  { 'command': 'query-netdev', 'returns': ['NetdevInfo'] }
> +
> +##
> +# @IP_PROTOCOL:
> +#
> +# Transport layer protocol.
> +#
> +# Just for IPv4.

Really?

> +#
> +# @tcp: Transmission Control Protocol.
> +#
> +# @udp: User Datagram Protocol.
> +#
> +# @dccp: Datagram Congestion Control Protocol.
> +#
> +# @sctp: Stream Control Transmission Protocol.
> +#
> +# @udplite: Lightweight User Datagram Protocol.
> +#
> +# @icmp: Internet Control Message Protocol.
> +#
> +# @igmp: Internet Group Management Protocol.
> +#
> +# @ipv6: IPv6 Encapsulation.
> +#
> +# TODO: Need to add more transport layer protocol.
> +#
> +# Since: 6.1
> +##
> +{ 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite',
> +    'icmp', 'igmp', 'ipv6' ] }
> +

docs/devel/qapi-code-gen.txt: "type definitions should always use
CamelCase".

Make this something like 'enum': 'IpProtocol', please.
chen.zhang@intel.com March 22, 2021, 9:59 a.m. UTC | #2
> -----Original Message-----
> From: Markus Armbruster <armbru@redhat.com>
> Sent: Friday, March 19, 2021 11:47 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>; Li Zhijian <lizhijian@cn.fujitsu.com>; Lukas
> Straub <lukasstraub2@web.de>; Zhang Chen <zhangckid@gmail.com>
> Subject: Re: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition
> 
> Zhang Chen <chen.zhang@intel.com> writes:
> 
> > Add IP_PROTOCOL as enum include TCP,UDP, ICMP... for other QMP
> commands.
> >
> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> > ---
> >  qapi/net.json | 31 +++++++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >
> > diff --git a/qapi/net.json b/qapi/net.json index
> > 87361ebd9a..498ea7aa72 100644
> > --- a/qapi/net.json
> > +++ b/qapi/net.json
> > @@ -794,3 +794,34 @@
> >  #
> >  ##
> >  { 'command': 'query-netdev', 'returns': ['NetdevInfo'] }
> > +
> > +##
> > +# @IP_PROTOCOL:
> > +#
> > +# Transport layer protocol.
> > +#
> > +# Just for IPv4.
> 
> Really?

Current tcp/udp/icmp field from IPv4 header definition,
I think maybe we need add more to support IPv6.
So, looks change to #TODO support IPv6 part is better?

> 
> > +#
> > +# @tcp: Transmission Control Protocol.
> > +#
> > +# @udp: User Datagram Protocol.
> > +#
> > +# @dccp: Datagram Congestion Control Protocol.
> > +#
> > +# @sctp: Stream Control Transmission Protocol.
> > +#
> > +# @udplite: Lightweight User Datagram Protocol.
> > +#
> > +# @icmp: Internet Control Message Protocol.
> > +#
> > +# @igmp: Internet Group Management Protocol.
> > +#
> > +# @ipv6: IPv6 Encapsulation.
> > +#
> > +# TODO: Need to add more transport layer protocol.
> > +#
> > +# Since: 6.1
> > +##
> > +{ 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite',
> > +    'icmp', 'igmp', 'ipv6' ] }
> > +
> 
> docs/devel/qapi-code-gen.txt: "type definitions should always use
> CamelCase".
> 
> Make this something like 'enum': 'IpProtocol', please.

OK, I will fix it in next version.

Thanks
Chen
Markus Armbruster March 22, 2021, 12:12 p.m. UTC | #3
"Zhang, Chen" <chen.zhang@intel.com> writes:

>> -----Original Message-----
>> From: Markus Armbruster <armbru@redhat.com>
>> Sent: Friday, March 19, 2021 11:47 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>; Li Zhijian <lizhijian@cn.fujitsu.com>; Lukas
>> Straub <lukasstraub2@web.de>; Zhang Chen <zhangckid@gmail.com>
>> Subject: Re: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition
>> 
>> Zhang Chen <chen.zhang@intel.com> writes:
>> 
>> > Add IP_PROTOCOL as enum include TCP,UDP, ICMP... for other QMP
>> commands.
>> >
>> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
>> > ---
>> >  qapi/net.json | 31 +++++++++++++++++++++++++++++++
>> >  1 file changed, 31 insertions(+)
>> >
>> > diff --git a/qapi/net.json b/qapi/net.json index
>> > 87361ebd9a..498ea7aa72 100644
>> > --- a/qapi/net.json
>> > +++ b/qapi/net.json
>> > @@ -794,3 +794,34 @@
>> >  #
>> >  ##
>> >  { 'command': 'query-netdev', 'returns': ['NetdevInfo'] }
>> > +
>> > +##
>> > +# @IP_PROTOCOL:
>> > +#
>> > +# Transport layer protocol.
>> > +#
>> > +# Just for IPv4.
>> 
>> Really?
>
> Current tcp/udp/icmp field from IPv4 header definition,
> I think maybe we need add more to support IPv6.
> So, looks change to #TODO support IPv6 part is better?

IPv4 and IPv6 share internet protocol numbers.  IPv4 has it in header
field "protocol", IPv6 in "next header".

Canonical registry:
https://www.iana.org/assignments/protocol-numbers/protocol-numbers.xhtml

>> > +#
>> > +# @tcp: Transmission Control Protocol.
>> > +#
>> > +# @udp: User Datagram Protocol.
>> > +#
>> > +# @dccp: Datagram Congestion Control Protocol.
>> > +#
>> > +# @sctp: Stream Control Transmission Protocol.
>> > +#
>> > +# @udplite: Lightweight User Datagram Protocol.
>> > +#
>> > +# @icmp: Internet Control Message Protocol.
>> > +#
>> > +# @igmp: Internet Group Management Protocol.
>> > +#
>> > +# @ipv6: IPv6 Encapsulation.
>> > +#
>> > +# TODO: Need to add more transport layer protocol.

If there's a need *now*, we should add them now.  If the may be a need
in the future, then this isn't a TODO.  Perhaps

      # Additional protocols may be added as needed.

How did you pick the ones to add now?

What if a user wants to use a protocol number not in this enum?  If that
makes no sense (say because use requires code in QEMU), fine.  If it
does make sense, we need to talk.  You tell me :)

>> > +#
>> > +# Since: 6.1
>> > +##
>> > +{ 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite',
>> > +    'icmp', 'igmp', 'ipv6' ] }
>> > +
>> 
>> docs/devel/qapi-code-gen.txt: "type definitions should always use
>> CamelCase".
>> 
>> Make this something like 'enum': 'IpProtocol', please.
>
> OK, I will fix it in next version.
>
> Thanks
> Chen
Daniel P. Berrangé March 22, 2021, 12:43 p.m. UTC | #4
On Mon, Mar 22, 2021 at 09:59:54AM +0000, Zhang, Chen wrote:
> 
> 
> > -----Original Message-----
> > From: Markus Armbruster <armbru@redhat.com>
> > Sent: Friday, March 19, 2021 11:47 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>; Li Zhijian <lizhijian@cn.fujitsu.com>; Lukas
> > Straub <lukasstraub2@web.de>; Zhang Chen <zhangckid@gmail.com>
> > Subject: Re: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition
> > 
> > Zhang Chen <chen.zhang@intel.com> writes:
> > 
> > > Add IP_PROTOCOL as enum include TCP,UDP, ICMP... for other QMP
> > commands.
> > >
> > > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> > > ---
> > >  qapi/net.json | 31 +++++++++++++++++++++++++++++++
> > >  1 file changed, 31 insertions(+)
> > >
> > > diff --git a/qapi/net.json b/qapi/net.json index
> > > 87361ebd9a..498ea7aa72 100644
> > > --- a/qapi/net.json
> > > +++ b/qapi/net.json
> > > @@ -794,3 +794,34 @@
> > >  #
> > >  ##
> > >  { 'command': 'query-netdev', 'returns': ['NetdevInfo'] }
> > > +
> > > +##
> > > +# @IP_PROTOCOL:
> > > +#
> > > +# Transport layer protocol.
> > > +#
> > > +# Just for IPv4.
> > 
> > Really?
> 
> Current tcp/udp/icmp field from IPv4 header definition,
> I think maybe we need add more to support IPv6.
> So, looks change to #TODO support IPv6 part is better?

I'm inclined to say that this should implement IPv6 from the start.

No modern software should be implementing network functionality
for IPv4 only anymore IMHO.   Especially when there are potential
implications for the design of public APIs like QMP, we should
expect dual-stack support from the start.

> 
> > 
> > > +#
> > > +# @tcp: Transmission Control Protocol.
> > > +#
> > > +# @udp: User Datagram Protocol.
> > > +#
> > > +# @dccp: Datagram Congestion Control Protocol.
> > > +#
> > > +# @sctp: Stream Control Transmission Protocol.
> > > +#
> > > +# @udplite: Lightweight User Datagram Protocol.
> > > +#
> > > +# @icmp: Internet Control Message Protocol.
> > > +#
> > > +# @igmp: Internet Group Management Protocol.
> > > +#
> > > +# @ipv6: IPv6 Encapsulation.
> > > +#
> > > +# TODO: Need to add more transport layer protocol.
> > > +#
> > > +# Since: 6.1
> > > +##
> > > +{ 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite',
> > > +    'icmp', 'igmp', 'ipv6' ] }
> > > +
> > 
> > docs/devel/qapi-code-gen.txt: "type definitions should always use
> > CamelCase".
> > 
> > Make this something like 'enum': 'IpProtocol', please.
> 
> OK, I will fix it in next version.
> 
> Thanks
> Chen
> 
> 

Regards,
Daniel
Dr. David Alan Gilbert March 23, 2021, 8:01 p.m. UTC | #5
* Zhang Chen (chen.zhang@intel.com) wrote:
> Add IP_PROTOCOL as enum include TCP,UDP, ICMP... for other QMP commands.
> 
> Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> ---
>  qapi/net.json | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/qapi/net.json b/qapi/net.json
> index 87361ebd9a..498ea7aa72 100644
> --- a/qapi/net.json
> +++ b/qapi/net.json
> @@ -794,3 +794,34 @@
>  #
>  ##
>  { 'command': 'query-netdev', 'returns': ['NetdevInfo'] }
> +
> +##
> +# @IP_PROTOCOL:
> +#
> +# Transport layer protocol.
> +#
> +# Just for IPv4.
> +#
> +# @tcp: Transmission Control Protocol.
> +#
> +# @udp: User Datagram Protocol.
> +#
> +# @dccp: Datagram Congestion Control Protocol.
> +#
> +# @sctp: Stream Control Transmission Protocol.
> +#
> +# @udplite: Lightweight User Datagram Protocol.
> +#
> +# @icmp: Internet Control Message Protocol.
> +#
> +# @igmp: Internet Group Management Protocol.
> +#
> +# @ipv6: IPv6 Encapsulation.
> +#
> +# TODO: Need to add more transport layer protocol.
> +#
> +# Since: 6.1
> +##
> +{ 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite',
> +    'icmp', 'igmp', 'ipv6' ] }

Isn't the right thing to do here to use a string for protocol and then
pass it to getprotobyname;  that way your list is never out of date, and
you never have to translate between the order of this enum and the
integer assignment set in stone.

Dave

> +
> -- 
> 2.25.1
>
chen.zhang@intel.com April 15, 2021, 10:51 a.m. UTC | #6
> -----Original Message-----
> From: Qemu-devel <qemu-devel-
> bounces+chen.zhang=intel.com@nongnu.org> On Behalf Of Dr. David Alan
> Gilbert
> Sent: Wednesday, March 24, 2021 4:01 AM
> 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>; Markus Armbruster
> <armbru@redhat.com>; Zhang Chen <zhangckid@gmail.com>
> Subject: Re: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition
> 
> * Zhang Chen (chen.zhang@intel.com) wrote:
> > Add IP_PROTOCOL as enum include TCP,UDP, ICMP... for other QMP
> commands.
> >
> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> > ---
> >  qapi/net.json | 31 +++++++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >
> > diff --git a/qapi/net.json b/qapi/net.json index
> > 87361ebd9a..498ea7aa72 100644
> > --- a/qapi/net.json
> > +++ b/qapi/net.json
> > @@ -794,3 +794,34 @@
> >  #
> >  ##
> >  { 'command': 'query-netdev', 'returns': ['NetdevInfo'] }
> > +
> > +##
> > +# @IP_PROTOCOL:
> > +#
> > +# Transport layer protocol.
> > +#
> > +# Just for IPv4.
> > +#
> > +# @tcp: Transmission Control Protocol.
> > +#
> > +# @udp: User Datagram Protocol.
> > +#
> > +# @dccp: Datagram Congestion Control Protocol.
> > +#
> > +# @sctp: Stream Control Transmission Protocol.
> > +#
> > +# @udplite: Lightweight User Datagram Protocol.
> > +#
> > +# @icmp: Internet Control Message Protocol.
> > +#
> > +# @igmp: Internet Group Management Protocol.
> > +#
> > +# @ipv6: IPv6 Encapsulation.
> > +#
> > +# TODO: Need to add more transport layer protocol.
> > +#
> > +# Since: 6.1
> > +##
> > +{ 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite',
> > +    'icmp', 'igmp', 'ipv6' ] }
> 
> Isn't the right thing to do here to use a string for protocol and then pass it to
> getprotobyname;  that way your list is never out of date, and you never have
> to translate between the order of this enum and the integer assignment set
> in stone.
> 

Hi Dave,

Considering that most of the scenes in Qemu don't call the getprotobyname, looks keep the string are more readable.
Please review the V5 patches, Thanks.

Thanks
Chen

> Dave
> 
> > +
> > --
> > 2.25.1
> >
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
Markus Armbruster April 15, 2021, 3:14 p.m. UTC | #7
"Zhang, Chen" <chen.zhang@intel.com> writes:

>> -----Original Message-----
>> From: Qemu-devel <qemu-devel-
>> bounces+chen.zhang=intel.com@nongnu.org> On Behalf Of Dr. David Alan
>> Gilbert
>> Sent: Wednesday, March 24, 2021 4:01 AM
>> 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>; Markus Armbruster
>> <armbru@redhat.com>; Zhang Chen <zhangckid@gmail.com>
>> Subject: Re: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition
>> 
>> * Zhang Chen (chen.zhang@intel.com) wrote:
>> > Add IP_PROTOCOL as enum include TCP,UDP, ICMP... for other QMP
>> commands.
>> >
>> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
>> > ---
>> >  qapi/net.json | 31 +++++++++++++++++++++++++++++++
>> >  1 file changed, 31 insertions(+)
>> >
>> > diff --git a/qapi/net.json b/qapi/net.json index
>> > 87361ebd9a..498ea7aa72 100644
>> > --- a/qapi/net.json
>> > +++ b/qapi/net.json
>> > @@ -794,3 +794,34 @@
>> >  #
>> >  ##
>> >  { 'command': 'query-netdev', 'returns': ['NetdevInfo'] }
>> > +
>> > +##
>> > +# @IP_PROTOCOL:
>> > +#
>> > +# Transport layer protocol.
>> > +#
>> > +# Just for IPv4.
>> > +#
>> > +# @tcp: Transmission Control Protocol.
>> > +#
>> > +# @udp: User Datagram Protocol.
>> > +#
>> > +# @dccp: Datagram Congestion Control Protocol.
>> > +#
>> > +# @sctp: Stream Control Transmission Protocol.
>> > +#
>> > +# @udplite: Lightweight User Datagram Protocol.
>> > +#
>> > +# @icmp: Internet Control Message Protocol.
>> > +#
>> > +# @igmp: Internet Group Management Protocol.
>> > +#
>> > +# @ipv6: IPv6 Encapsulation.
>> > +#
>> > +# TODO: Need to add more transport layer protocol.
>> > +#
>> > +# Since: 6.1
>> > +##
>> > +{ 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite',
>> > +    'icmp', 'igmp', 'ipv6' ] }
>> 
>> Isn't the right thing to do here to use a string for protocol and then pass it to
>> getprotobyname;  that way your list is never out of date, and you never have
>> to translate between the order of this enum and the integer assignment set
>> in stone.
>> 
>
> Hi Dave,
>
> Considering that most of the scenes in Qemu don't call the getprotobyname, looks keep the string are more readable.

Unless I'm missing something,

(1) this enum is only used for matching packets by source IP, source
port, destination IP, destination port, and protocol, and

(2) the packet matching works just fine for *any* protocol.

By using an enum for the protocol, you're limiting the matcher to
whatever protocols we bother to include in the enum for no particular
reason.  Feels wrong to me.

> Please review the V5 patches, Thanks.
>
> Thanks
> Chen
>
>> Dave
>> 
>> > +
>> > --
>> > 2.25.1
>> >
>> --
>> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>>
chen.zhang@intel.com April 16, 2021, 6:03 a.m. UTC | #8
> -----Original Message-----
> From: Markus Armbruster <armbru@redhat.com>
> Sent: Thursday, April 15, 2021 11:15 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>; Lukas Straub
> <lukasstraub2@web.de>; Li Zhijian <lizhijian@cn.fujitsu.com>; Jason Wang
> <jasowang@redhat.com>; qemu-dev <qemu-devel@nongnu.org>; Zhang
> Chen <zhangckid@gmail.com>
> Subject: Re: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition
> 
> "Zhang, Chen" <chen.zhang@intel.com> writes:
> 
> >> -----Original Message-----
> >> From: Qemu-devel <qemu-devel-
> >> bounces+chen.zhang=intel.com@nongnu.org> On Behalf Of Dr. David
> Alan
> >> Gilbert
> >> Sent: Wednesday, March 24, 2021 4:01 AM
> >> 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>; Markus Armbruster
> <armbru@redhat.com>;
> >> Zhang Chen <zhangckid@gmail.com>
> >> Subject: Re: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition
> >>
> >> * Zhang Chen (chen.zhang@intel.com) wrote:
> >> > Add IP_PROTOCOL as enum include TCP,UDP, ICMP... for other QMP
> >> commands.
> >> >
> >> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> >> > ---
> >> >  qapi/net.json | 31 +++++++++++++++++++++++++++++++
> >> >  1 file changed, 31 insertions(+)
> >> >
> >> > diff --git a/qapi/net.json b/qapi/net.json index
> >> > 87361ebd9a..498ea7aa72 100644
> >> > --- a/qapi/net.json
> >> > +++ b/qapi/net.json
> >> > @@ -794,3 +794,34 @@
> >> >  #
> >> >  ##
> >> >  { 'command': 'query-netdev', 'returns': ['NetdevInfo'] }
> >> > +
> >> > +##
> >> > +# @IP_PROTOCOL:
> >> > +#
> >> > +# Transport layer protocol.
> >> > +#
> >> > +# Just for IPv4.
> >> > +#
> >> > +# @tcp: Transmission Control Protocol.
> >> > +#
> >> > +# @udp: User Datagram Protocol.
> >> > +#
> >> > +# @dccp: Datagram Congestion Control Protocol.
> >> > +#
> >> > +# @sctp: Stream Control Transmission Protocol.
> >> > +#
> >> > +# @udplite: Lightweight User Datagram Protocol.
> >> > +#
> >> > +# @icmp: Internet Control Message Protocol.
> >> > +#
> >> > +# @igmp: Internet Group Management Protocol.
> >> > +#
> >> > +# @ipv6: IPv6 Encapsulation.
> >> > +#
> >> > +# TODO: Need to add more transport layer protocol.
> >> > +#
> >> > +# Since: 6.1
> >> > +##
> >> > +{ 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite',
> >> > +    'icmp', 'igmp', 'ipv6' ] }
> >>
> >> Isn't the right thing to do here to use a string for protocol and
> >> then pass it to getprotobyname;  that way your list is never out of
> >> date, and you never have to translate between the order of this enum
> >> and the integer assignment set in stone.
> >>
> >
> > Hi Dave,
> >
> > Considering that most of the scenes in Qemu don't call the
> getprotobyname, looks keep the string are more readable.
> 
> Unless I'm missing something,
> 
> (1) this enum is only used for matching packets by source IP, source port,
> destination IP, destination port, and protocol, and
> 
> (2) the packet matching works just fine for *any* protocol.
> 
> By using an enum for the protocol, you're limiting the matcher to whatever
> protocols we bother to include in the enum for no particular reason.  Feels
> wrong to me.

Should we remove the IP_PROTOCOL enum here? Make user input string protocol name for this field?
Or any other detailed comments here?

Thanks
Chen

> 
> > Please review the V5 patches, Thanks.
> >
> > Thanks
> > Chen
> >
> >> Dave
> >>
> >> > +
> >> > --
> >> > 2.25.1
> >> >
> >> --
> >> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >>
Markus Armbruster April 16, 2021, 9:22 a.m. UTC | #9
"Zhang, Chen" <chen.zhang@intel.com> writes:

>> -----Original Message-----
>> From: Markus Armbruster <armbru@redhat.com>
>> Sent: Thursday, April 15, 2021 11:15 PM
>> To: Zhang, Chen <chen.zhang@intel.com>
>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>; Lukas Straub
>> <lukasstraub2@web.de>; Li Zhijian <lizhijian@cn.fujitsu.com>; Jason Wang
>> <jasowang@redhat.com>; qemu-dev <qemu-devel@nongnu.org>; Zhang
>> Chen <zhangckid@gmail.com>
>> Subject: Re: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition
>> 
>> "Zhang, Chen" <chen.zhang@intel.com> writes:
>> 
>> >> -----Original Message-----
>> >> From: Qemu-devel <qemu-devel-
>> >> bounces+chen.zhang=intel.com@nongnu.org> On Behalf Of Dr. David
>> Alan
>> >> Gilbert
>> >> Sent: Wednesday, March 24, 2021 4:01 AM
>> >> 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>; Markus Armbruster
>> <armbru@redhat.com>;
>> >> Zhang Chen <zhangckid@gmail.com>
>> >> Subject: Re: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition
>> >>
>> >> * Zhang Chen (chen.zhang@intel.com) wrote:
>> >> > Add IP_PROTOCOL as enum include TCP,UDP, ICMP... for other QMP
>> >> commands.
>> >> >
>> >> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
>> >> > ---
>> >> >  qapi/net.json | 31 +++++++++++++++++++++++++++++++
>> >> >  1 file changed, 31 insertions(+)
>> >> >
>> >> > diff --git a/qapi/net.json b/qapi/net.json index
>> >> > 87361ebd9a..498ea7aa72 100644
>> >> > --- a/qapi/net.json
>> >> > +++ b/qapi/net.json
>> >> > @@ -794,3 +794,34 @@
>> >> >  #
>> >> >  ##
>> >> >  { 'command': 'query-netdev', 'returns': ['NetdevInfo'] }
>> >> > +
>> >> > +##
>> >> > +# @IP_PROTOCOL:
>> >> > +#
>> >> > +# Transport layer protocol.
>> >> > +#
>> >> > +# Just for IPv4.
>> >> > +#
>> >> > +# @tcp: Transmission Control Protocol.
>> >> > +#
>> >> > +# @udp: User Datagram Protocol.
>> >> > +#
>> >> > +# @dccp: Datagram Congestion Control Protocol.
>> >> > +#
>> >> > +# @sctp: Stream Control Transmission Protocol.
>> >> > +#
>> >> > +# @udplite: Lightweight User Datagram Protocol.
>> >> > +#
>> >> > +# @icmp: Internet Control Message Protocol.
>> >> > +#
>> >> > +# @igmp: Internet Group Management Protocol.
>> >> > +#
>> >> > +# @ipv6: IPv6 Encapsulation.
>> >> > +#
>> >> > +# TODO: Need to add more transport layer protocol.
>> >> > +#
>> >> > +# Since: 6.1
>> >> > +##
>> >> > +{ 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite',
>> >> > +    'icmp', 'igmp', 'ipv6' ] }
>> >>
>> >> Isn't the right thing to do here to use a string for protocol and
>> >> then pass it to getprotobyname;  that way your list is never out of
>> >> date, and you never have to translate between the order of this enum
>> >> and the integer assignment set in stone.
>> >>
>> >
>> > Hi Dave,
>> >
>> > Considering that most of the scenes in Qemu don't call the
>> getprotobyname, looks keep the string are more readable.
>> 
>> Unless I'm missing something,
>> 
>> (1) this enum is only used for matching packets by source IP, source port,
>> destination IP, destination port, and protocol, and
>> 
>> (2) the packet matching works just fine for *any* protocol.
>> 
>> By using an enum for the protocol, you're limiting the matcher to whatever
>> protocols we bother to include in the enum for no particular reason.  Feels
>> wrong to me.
>
> Should we remove the IP_PROTOCOL enum here? Make user input string protocol name for this field?
> Or any other detailed comments here?

I believe that's Dave's point.  I.e.:

    { 'struct': 'L4_Connection',
      'data': { 'protocol': 'str', ... }

If we ever need to specify protocols by number in addition to name, we
can compatibly evolve the 'str' into an alternation of 'str' and
'uint8'.  Unlikely.
Dr. David Alan Gilbert April 20, 2021, 11:05 a.m. UTC | #10
* Markus Armbruster (armbru@redhat.com) wrote:
> "Zhang, Chen" <chen.zhang@intel.com> writes:
> 
> >> -----Original Message-----
> >> From: Markus Armbruster <armbru@redhat.com>
> >> Sent: Thursday, April 15, 2021 11:15 PM
> >> To: Zhang, Chen <chen.zhang@intel.com>
> >> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>; Lukas Straub
> >> <lukasstraub2@web.de>; Li Zhijian <lizhijian@cn.fujitsu.com>; Jason Wang
> >> <jasowang@redhat.com>; qemu-dev <qemu-devel@nongnu.org>; Zhang
> >> Chen <zhangckid@gmail.com>
> >> Subject: Re: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition
> >> 
> >> "Zhang, Chen" <chen.zhang@intel.com> writes:
> >> 
> >> >> -----Original Message-----
> >> >> From: Qemu-devel <qemu-devel-
> >> >> bounces+chen.zhang=intel.com@nongnu.org> On Behalf Of Dr. David
> >> Alan
> >> >> Gilbert
> >> >> Sent: Wednesday, March 24, 2021 4:01 AM
> >> >> 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>; Markus Armbruster
> >> <armbru@redhat.com>;
> >> >> Zhang Chen <zhangckid@gmail.com>
> >> >> Subject: Re: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition
> >> >>
> >> >> * Zhang Chen (chen.zhang@intel.com) wrote:
> >> >> > Add IP_PROTOCOL as enum include TCP,UDP, ICMP... for other QMP
> >> >> commands.
> >> >> >
> >> >> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> >> >> > ---
> >> >> >  qapi/net.json | 31 +++++++++++++++++++++++++++++++
> >> >> >  1 file changed, 31 insertions(+)
> >> >> >
> >> >> > diff --git a/qapi/net.json b/qapi/net.json index
> >> >> > 87361ebd9a..498ea7aa72 100644
> >> >> > --- a/qapi/net.json
> >> >> > +++ b/qapi/net.json
> >> >> > @@ -794,3 +794,34 @@
> >> >> >  #
> >> >> >  ##
> >> >> >  { 'command': 'query-netdev', 'returns': ['NetdevInfo'] }
> >> >> > +
> >> >> > +##
> >> >> > +# @IP_PROTOCOL:
> >> >> > +#
> >> >> > +# Transport layer protocol.
> >> >> > +#
> >> >> > +# Just for IPv4.
> >> >> > +#
> >> >> > +# @tcp: Transmission Control Protocol.
> >> >> > +#
> >> >> > +# @udp: User Datagram Protocol.
> >> >> > +#
> >> >> > +# @dccp: Datagram Congestion Control Protocol.
> >> >> > +#
> >> >> > +# @sctp: Stream Control Transmission Protocol.
> >> >> > +#
> >> >> > +# @udplite: Lightweight User Datagram Protocol.
> >> >> > +#
> >> >> > +# @icmp: Internet Control Message Protocol.
> >> >> > +#
> >> >> > +# @igmp: Internet Group Management Protocol.
> >> >> > +#
> >> >> > +# @ipv6: IPv6 Encapsulation.
> >> >> > +#
> >> >> > +# TODO: Need to add more transport layer protocol.
> >> >> > +#
> >> >> > +# Since: 6.1
> >> >> > +##
> >> >> > +{ 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite',
> >> >> > +    'icmp', 'igmp', 'ipv6' ] }
> >> >>
> >> >> Isn't the right thing to do here to use a string for protocol and
> >> >> then pass it to getprotobyname;  that way your list is never out of
> >> >> date, and you never have to translate between the order of this enum
> >> >> and the integer assignment set in stone.
> >> >>
> >> >
> >> > Hi Dave,
> >> >
> >> > Considering that most of the scenes in Qemu don't call the
> >> getprotobyname, looks keep the string are more readable.
> >> 
> >> Unless I'm missing something,
> >> 
> >> (1) this enum is only used for matching packets by source IP, source port,
> >> destination IP, destination port, and protocol, and
> >> 
> >> (2) the packet matching works just fine for *any* protocol.
> >> 
> >> By using an enum for the protocol, you're limiting the matcher to whatever
> >> protocols we bother to include in the enum for no particular reason.  Feels
> >> wrong to me.
> >
> > Should we remove the IP_PROTOCOL enum here? Make user input string protocol name for this field?
> > Or any other detailed comments here?
> 
> I believe that's Dave's point.  I.e.:
> 
>     { 'struct': 'L4_Connection',
>       'data': { 'protocol': 'str', ... }
> 
> If we ever need to specify protocols by number in addition to name, we
> can compatibly evolve the 'str' into an alternation of 'str' and
> 'uint8'.  Unlikely.

Right; just using a string and sending it via getprotobyname() actually
seems easier than using the enum and having all the conversions.

Dave
chen.zhang@intel.com April 20, 2021, 3:20 p.m. UTC | #11
> -----Original Message-----
> From: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Sent: Tuesday, April 20, 2021 7:06 PM
> To: Markus Armbruster <armbru@redhat.com>
> Cc: Zhang, Chen <chen.zhang@intel.com>; Lukas Straub
> <lukasstraub2@web.de>; Li Zhijian <lizhijian@cn.fujitsu.com>; Jason Wang
> <jasowang@redhat.com>; qemu-dev <qemu-devel@nongnu.org>; Zhang
> Chen <zhangckid@gmail.com>
> Subject: Re: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition
> 
> * Markus Armbruster (armbru@redhat.com) wrote:
> > "Zhang, Chen" <chen.zhang@intel.com> writes:
> >
> > >> -----Original Message-----
> > >> From: Markus Armbruster <armbru@redhat.com>
> > >> Sent: Thursday, April 15, 2021 11:15 PM
> > >> To: Zhang, Chen <chen.zhang@intel.com>
> > >> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>; Lukas Straub
> > >> <lukasstraub2@web.de>; Li Zhijian <lizhijian@cn.fujitsu.com>; Jason
> > >> Wang <jasowang@redhat.com>; qemu-dev <qemu-
> devel@nongnu.org>; Zhang
> > >> Chen <zhangckid@gmail.com>
> > >> Subject: Re: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL
> > >> definition
> > >>
> > >> "Zhang, Chen" <chen.zhang@intel.com> writes:
> > >>
> > >> >> -----Original Message-----
> > >> >> From: Qemu-devel <qemu-devel-
> > >> >> bounces+chen.zhang=intel.com@nongnu.org> On Behalf Of Dr.
> David
> > >> Alan
> > >> >> Gilbert
> > >> >> Sent: Wednesday, March 24, 2021 4:01 AM
> > >> >> 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>; Markus Armbruster
> > >> <armbru@redhat.com>;
> > >> >> Zhang Chen <zhangckid@gmail.com>
> > >> >> Subject: Re: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL
> > >> >> definition
> > >> >>
> > >> >> * Zhang Chen (chen.zhang@intel.com) wrote:
> > >> >> > Add IP_PROTOCOL as enum include TCP,UDP, ICMP... for other
> QMP
> > >> >> commands.
> > >> >> >
> > >> >> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> > >> >> > ---
> > >> >> >  qapi/net.json | 31 +++++++++++++++++++++++++++++++
> > >> >> >  1 file changed, 31 insertions(+)
> > >> >> >
> > >> >> > diff --git a/qapi/net.json b/qapi/net.json index
> > >> >> > 87361ebd9a..498ea7aa72 100644
> > >> >> > --- a/qapi/net.json
> > >> >> > +++ b/qapi/net.json
> > >> >> > @@ -794,3 +794,34 @@
> > >> >> >  #
> > >> >> >  ##
> > >> >> >  { 'command': 'query-netdev', 'returns': ['NetdevInfo'] }
> > >> >> > +
> > >> >> > +##
> > >> >> > +# @IP_PROTOCOL:
> > >> >> > +#
> > >> >> > +# Transport layer protocol.
> > >> >> > +#
> > >> >> > +# Just for IPv4.
> > >> >> > +#
> > >> >> > +# @tcp: Transmission Control Protocol.
> > >> >> > +#
> > >> >> > +# @udp: User Datagram Protocol.
> > >> >> > +#
> > >> >> > +# @dccp: Datagram Congestion Control Protocol.
> > >> >> > +#
> > >> >> > +# @sctp: Stream Control Transmission Protocol.
> > >> >> > +#
> > >> >> > +# @udplite: Lightweight User Datagram Protocol.
> > >> >> > +#
> > >> >> > +# @icmp: Internet Control Message Protocol.
> > >> >> > +#
> > >> >> > +# @igmp: Internet Group Management Protocol.
> > >> >> > +#
> > >> >> > +# @ipv6: IPv6 Encapsulation.
> > >> >> > +#
> > >> >> > +# TODO: Need to add more transport layer protocol.
> > >> >> > +#
> > >> >> > +# Since: 6.1
> > >> >> > +##
> > >> >> > +{ 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite',
> > >> >> > +    'icmp', 'igmp', 'ipv6' ] }
> > >> >>
> > >> >> Isn't the right thing to do here to use a string for protocol
> > >> >> and then pass it to getprotobyname;  that way your list is never
> > >> >> out of date, and you never have to translate between the order
> > >> >> of this enum and the integer assignment set in stone.
> > >> >>
> > >> >
> > >> > Hi Dave,
> > >> >
> > >> > Considering that most of the scenes in Qemu don't call the
> > >> getprotobyname, looks keep the string are more readable.
> > >>
> > >> Unless I'm missing something,
> > >>
> > >> (1) this enum is only used for matching packets by source IP,
> > >> source port, destination IP, destination port, and protocol, and
> > >>
> > >> (2) the packet matching works just fine for *any* protocol.
> > >>
> > >> By using an enum for the protocol, you're limiting the matcher to
> > >> whatever protocols we bother to include in the enum for no
> > >> particular reason.  Feels wrong to me.
> > >
> > > Should we remove the IP_PROTOCOL enum here? Make user input string
> protocol name for this field?
> > > Or any other detailed comments here?
> >
> > I believe that's Dave's point.  I.e.:
> >
> >     { 'struct': 'L4_Connection',
> >       'data': { 'protocol': 'str', ... }
> >
> > If we ever need to specify protocols by number in addition to name, we
> > can compatibly evolve the 'str' into an alternation of 'str' and
> > 'uint8'.  Unlikely.
> 
> Right; just using a string and sending it via getprotobyname() actually seems
> easier than using the enum and having all the conversions.

OK, Thanks for clear it.  I already fixed it.
Please review the V6 of this series.

Thanks
Chen

> 
> Dave
> 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff mbox series

Patch

diff --git a/qapi/net.json b/qapi/net.json
index 87361ebd9a..498ea7aa72 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -794,3 +794,34 @@ 
 #
 ##
 { 'command': 'query-netdev', 'returns': ['NetdevInfo'] }
+
+##
+# @IP_PROTOCOL:
+#
+# Transport layer protocol.
+#
+# Just for IPv4.
+#
+# @tcp: Transmission Control Protocol.
+#
+# @udp: User Datagram Protocol.
+#
+# @dccp: Datagram Congestion Control Protocol.
+#
+# @sctp: Stream Control Transmission Protocol.
+#
+# @udplite: Lightweight User Datagram Protocol.
+#
+# @icmp: Internet Control Message Protocol.
+#
+# @igmp: Internet Group Management Protocol.
+#
+# @ipv6: IPv6 Encapsulation.
+#
+# TODO: Need to add more transport layer protocol.
+#
+# Since: 6.1
+##
+{ 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 'udplite',
+    'icmp', 'igmp', 'ipv6' ] }
+