RE: [PATCH V4 2/7] qapi/net.json: Add L4_Connection definition

["Zhang, Chen" <chen.zhang@intel.com>]

> -----Original Message-----
> From: Markus Armbruster <armbru@redhat.com>
> Sent: Wednesday, March 24, 2021 2:47 PM
> To: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Lukas Straub <lukasstraub2@web.de>; Li Zhijian
> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; qemu-
> dev <qemu-devel@nongnu.org>; Zhang, Chen <chen.zhang@intel.com>;
> Zhang Chen <zhangckid@gmail.com>
> Subject: Re: [PATCH V4 2/7] qapi/net.json: Add L4_Connection definition
> 
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
> 
> > * Markus Armbruster (armbru@redhat.com) wrote:
> >> "Zhang, Chen" <chen.zhang@intel.com> writes:
> >>
> >> >> -----Original Message-----
> >> >> From: Markus Armbruster <armbru@redhat.com>
> >> [...]
> >> >> Naming the argument type L4_Connection is misleading.
> >> >>
> >> >> Even naming the match arguments L4_Connection would be
> misleading.
> >> >> "Connection" has a specific meaning in networking.  There are TCP
> >> >> connections.  There is no such thing as an UDP connection.
> >> >>
> >> >> A TCP connection is uniquely identified by a pair of endpoints,
> >> >> i.e. by source address, source port, destination address, destination
> port.
> >> >> Same for other connection-oriented protocols.  The protocol is not
> >> >> part of the connection.  Thus, L4_Connection would be misleading
> >> >> even for the connection-oriented case.
> >> >>
> >> >> You need a named type for colo-passthrough-add's argument because
> >> >> you share it with colo-passthrough-del.  I'm not sure that's what
> >> >> we want (I'm going to write more on that in a moment).  If it is
> >> >> what we want, then please pick a another, descriptive name.
> >> >
> >> > What do you think the "L4BypassRule" or "NetworkRule" ?
> >>
> >> NetworkRule is too generic.
> >>
> >> What about ColoPassthroughRule?
> >
> > Which is a bit specific; there's not actually anything Colo specific
> > in there; can I suggest 'L4FlowSpec';
> 
> "A bit too specific" is mostly harmless, since we can rename types at any time
> (they are not visible in external interfaces).
> 
> This is *not* an objection to less specific names.  All I want is names that
> don't give me wrong ideas on the thing's purpose.  L4FlowSpec and
> IPFlowSpec (below) feel fine in that regard.
> 
> >                                     I think there should also beb a
> > separate type that represents an IP address+port, so that what you end
> > up with is:
> >
> >   IPFlowSpec
> >      ID
> >      Protocol
> >      Source
> >      Dest
> 
> I understand the motivation.  Three drawbacks, though.
> 
> One, it gets us another level of nesting on the wire, i.e. something like
> 
>     {"source": {"address": SRC-ADDR, "port": SRC-PORT},
>      "destination": {"address": DST-ADDR, "port": DST-PORT}}
> 
> instead of
> 
>     {"source-address": SRC-ADDR, "source-port": SRC-PORT,
>      "destination-address": DST-ADDR, "destination-port": DST-PORT}
> 
> QMP clients shouldn't care.
> 
> Two, we have many (address, port) pairs in the schema that don't use
> nesting.  Adding nesting sometimes makes QMP less consistent.
> 
> Three, human-friendly interface wrappers tend to dislike nesting.  This
> particular case seems okay; we end up with dotted keys like source.address
> instead of source-address.  In a case where we need just one (address, port),
> we'd get some-silly-name.address instead of just address, though.
> 
> I've occasionally felt a mild need for letting me say "this struct member
> should be unboxed on the wire", i.e. have its curlies peeled off.
> Never enough to justify the additional generator complexity, though.

The initial patch of this series used unboxed struct, Eric's comments is change it to boxed.
I think it's OK, for the unused field we can keep 0 for it. The n-tuple(src IP, dst IP, src port, dst port, protocol)
will be used in many place on Qemu network related code(like migrate, NBD....).  
For the name, I think Dave's comments is well, for the @InetSocketAddressBase we can remove it and change it to use IPFlowSpec.
Markus, what do you think about it?

Thanks
Chen