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

[Markus Armbruster]

"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.