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