Re: [PATCH V7 1/6] qapi/net: Add IPFlowSpec and QMP command for COLO passthrough

[Markus Armbruster]

"Zhang, Chen" <chen.zhang@intel.com> writes:

>> -----Original Message-----
>> From: Eric Blake <eblake@redhat.com>
>> Sent: Friday, June 4, 2021 10:35 PM
>> To: Zhang, Chen <chen.zhang@intel.com>
>> Cc: Jason Wang <jasowang@redhat.com>; qemu-dev <qemu-
>> devel@nongnu.org>; Dr. David Alan Gilbert <dgilbert@redhat.com>; Markus
>> Armbruster <armbru@redhat.com>; Daniel P. Berrangé
>> <berrange@redhat.com>; Gerd Hoffmann <kraxel@redhat.com>; Li Zhijian
>> <lizhijian@cn.fujitsu.com>; Zhang Chen <zhangckid@gmail.com>; Lukas
>> Straub <lukasstraub2@web.de>
>> Subject: Re: [PATCH V7 1/6] qapi/net: Add IPFlowSpec and QMP command
>> for COLO passthrough
>> 
>> On Wed, May 26, 2021 at 10:54:19AM +0800, Zhang Chen wrote:
>> > Since the real user scenario does not need COLO to monitor all traffic.
>> > Add colo-passthrough-add and colo-passthrough-del to maintain a COLO
>> > network passthrough list. Add IPFlowSpec struct for all QMP commands.
>> > Except protocol field is necessary, other fields are optional.
>> 
>> That last sentence reads awkwardly, and I don't see a protocol field in the
>> patch below.
>
> Oh, We move the protocol field to optional by Lukas's comments in V6.
> I will remove this comments here.
>
>> 
>> >
>> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
>> > ---
>> >  net/net.c     | 10 ++++++++
>> >  qapi/net.json | 68
>> > +++++++++++++++++++++++++++++++++++++++++++++++++++
>> >  2 files changed, 78 insertions(+)
>> >
>> 
>> > +++ b/qapi/net.json
>> > @@ -7,6 +7,7 @@
>> >  ##
>> >
>> >  { 'include': 'common.json' }
>> > +{ 'include': 'sockets.json' }
>> >
>> >  ##
>> >  # @set_link:
>> > @@ -694,3 +695,70 @@
>> >  ##
>> >  { 'event': 'FAILOVER_NEGOTIATED',
>> >    'data': {'device-id': 'str'} }
>> > +
>> > +##
>> > +# @IPFlowSpec:
>> > +#
>> > +# IP flow specification.
>> > +#
>> > +# @protocol: Transport layer protocol like TCP/UDP...
>> 
>> Why is this open-coded as 'str' instead of an enum?
>
> The original code use enum, but we change it by Dave and Markus's comments.
> Please check the history:
> https://lists.gnu.org/archive/html/qemu-devel/2021-04/msg03919.html

It's a string to be passed to getprotobyname(3).  Please mention that in
the doc string.

It's not an enum, because we don't want to duplicate /etc/protocols in
the QAPI schema.

>> > +#
>> > +# @object-name: Point out the IPflow spec effective range of object,

I have no idea what that means :)

It might be what was called @id in v4.  There, you explained

    The @id means packet hander in Qemu. Because not all the guest network 
packet into the colo-compare module, the net-filters are same cases.
    There modules attach to NIC or chardev socket to work, VM maybe have multi 
modules running. So we use the ID to set the rule to the specific module. 

and I asked you to work it into the doc comment.

If you want help with working it into the doc comment, please explain
its intended use for dummies :)

>> > +#               If there is no such part, it means global spec.
>> > +#
>> > +# @source: Source address and port.
>> > +#
>> > +# @destination: Destination address and port.
>> > +#
>> > +# Since: 6.1
>> > +##
>> > +{ 'struct': 'IPFlowSpec',
>> > +  'data': { '*protocol': 'str', '*object-name': 'str',
>> > +    '*source': 'InetSocketAddressBase',
>> > +    '*destination': 'InetSocketAddressBase' } }
>> > +
>> > +##
>> > +# @colo-passthrough-add:
>> > +#
>> > +# Add passthrough entry according to user's needs in COLO-compare.
>> > +# Source IP/port and destination IP/port both optional, If user just
>> > +# input parts of infotmation, it will match all.
>> 
>> information
>> 
>> Grammar suggestion:
>> 
>> The source and destination IP/ports are both optional; if the user only 
>> inputs
>> part of the information, this will match all traffic.
>> 
>> except I'm not sure if my rewrite conveys the actual intent.
>
> Looks good to me, It should add the "protocol" to optional too.
> Sorry, I'm not a native speaker, I will fix it in next version.
>
>> 
>> > +#
>> > +# Returns: Nothing on success
>> > +#
>> > +# Since: 6.1
>> > +#
>> > +# Example:
>> > +#
>> > +# -> { "execute": "colo-passthrough-add",
>> > +#      "arguments": { "protocol": "tcp", "object-name": "object0",
>> > +#      "source": {"host": "192.168.1.1", "port": "1234"},
>> > +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
>> > +# <- { "return": {} }
>> > +#
>> > +##
>> > +{ 'command': 'colo-passthrough-add', 'boxed': true,
>> > +     'data': 'IPFlowSpec' }
>> > +
>> > +##
>> > +# @colo-passthrough-del:
>> > +#
>> > +# Delete passthrough entry according to user's needs in COLO-compare.
>> > +# Source IP/port and destination IP/port both optional, If user just
>> > +# input parts of infotmation, it will match all.
>> 
>> Same problems as above.
>
> OK.
>
> Thanks
> Chen
>
>> 
>> > +#
>> > +# Returns: Nothing on success
>> > +#
>> > +# Since: 6.1
>> > +#
>> > +# Example:
>> > +#
>> > +# -> { "execute": "colo-passthrough-del",
>> > +#      "arguments": { "protocol": "tcp", "object-name": "object0",
>> > +#      "source": {"host": "192.168.1.1", "port": "1234"},
>> > +#      "destination": {"host": "192.168.1.2", "port": "4321"} } }
>> > +# <- { "return": {} }
>> > +#
>> > +##
>> > +{ 'command': 'colo-passthrough-del', 'boxed': true,
>> > +     'data': 'IPFlowSpec' }
>> > --
>> > 2.25.1
>> >
>> 
>> --
>> Eric Blake, Principal Software Engineer
>> Red Hat, Inc.           +1-919-301-3266
>> Virtualization:  qemu.org | libvirt.org