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

From: Markus Armbruster <armbru@redhat.com>
To: "Zhang, Chen" <chen.zhang@intel.com>
Cc: Lukas Straub <lukasstraub2@web.de>,
	Li Zhijian <lizhijian@cn.fujitsu.com>,
	Jason Wang <jasowang@redhat.com>,
	"Dr. David Alan Gilbert" <dgilbert@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
Date: Thu, 15 Apr 2021 17:14:57 +0200	[thread overview]
Message-ID: <87czuvbmvi.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <5a9d9778f4784b4b96fa9e0831635c8d@intel.com> (Chen Zhang's message of "Thu, 15 Apr 2021 10:51:13 +0000")

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