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

[Markus Armbruster <armbru@redhat.com>]

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