| Message ID | 20210319035508.113741-2-chen.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Bypass specific network traffic in COLO | expand |
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? > +# > +# @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.
> -----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? > > > +# > > +# @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
"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
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
* 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. Dave > + > -- > 2.25.1 >
> -----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. Please review the V5 patches, Thanks. Thanks Chen > Dave > > > + > > -- > > 2.25.1 > > > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK >
"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 >>
> -----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? Thanks Chen > > > Please review the V5 patches, Thanks. > > > > Thanks > > Chen > > > >> Dave > >> > >> > + > >> > -- > >> > 2.25.1 > >> > > >> -- > >> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > >>
"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.
* 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. Dave
> -----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
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' ] } +
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(+)