* Markus Armbruster (arm...@redhat.com) wrote:
> "Zhang, Chen" <chen.zh...@intel.com> writes:
>
> >> -----Original Message-----
> >> From: Markus Armbruster <arm...@redhat.com>
> >> Sent: Thursday, April 15, 2021 11:15 PM
> >> To: Zhang, Chen <chen.zh...@intel.com>
> >> Cc: Dr. David Alan Gilbert <dgilb...@redhat.com>; Lukas Straub
> >> <lukasstra...@web.de>; Li Zhijian <lizhij...@cn.fujitsu.com>; Jason Wang
> >> <jasow...@redhat.com>; qemu-dev <qemu-devel@nongnu.org>; Zhang
> >> Chen <zhangc...@gmail.com>
> >> Subject: Re: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition
> >>
> >> "Zhang, Chen" <chen.zh...@intel.com> writes:
> >>
> >> >> -----Original Message-----
> >> >> From: Qemu-devel <qemu-devel-
> >> >> bounces+chen.zhang=intel....@nongnu.org> On Behalf Of Dr. David
> >> Alan
> >> >> Gilbert
> >> >> Sent: Wednesday, March 24, 2021 4:01 AM
> >> >> To: Zhang, Chen <chen.zh...@intel.com>
> >> >> Cc: Lukas Straub <lukasstra...@web.de>; Li Zhijian
> >> >> <lizhij...@cn.fujitsu.com>; Jason Wang <jasow...@redhat.com>; qemu-
> >> >> dev <qemu-devel@nongnu.org>; Markus Armbruster
> >> <arm...@redhat.com>;
> >> >> Zhang Chen <zhangc...@gmail.com>
> >> >> Subject: Re: [PATCH V4 1/7] qapi/net.json: Add IP_PROTOCOL definition
> >> >>
> >> >> * Zhang Chen (chen.zh...@intel.com) wrote:
> >> >> > Add IP_PROTOCOL as enum include TCP,UDP, ICMP... for other QMP
> >> >> commands.
> >> >> >
> >> >> > Signed-off-by: Zhang Chen <chen.zh...@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
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
