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

[Zhang, Chen]

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