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

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, Mar 22, 2021 at 09:59:54AM +0000, Zhang, Chen wrote:
> 
> 
> > -----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?

I'm inclined to say that this should implement IPv6 from the start.

No modern software should be implementing network functionality
for IPv4 only anymore IMHO.   Especially when there are potential
implications for the design of public APIs like QMP, we should
expect dual-stack support from the start.

> 
> > 
> > > +#
> > > +# @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' ] }
> > > +
> > 
> > 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
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|