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

From: Markus Armbruster <armbru@redhat.com>
To: "Zhang, Chen" <chen.zhang@intel.com>
Cc: "Lukas Straub" <lukasstraub2@web.de>,
	"Daniel P. Berrangé" <berrange@redhat.com>,
	"Li Zhijian" <lizhijian@cn.fujitsu.com>,
	"Jason Wang" <jasowang@redhat.com>,
	"Markus Armbruster" <armbru@redhat.com>,
	qemu-dev <qemu-devel@nongnu.org>,
	"Gerd Hoffmann" <kraxel@redhat.com>,
	"Zhang Chen" <zhangckid@gmail.com>,
	"Eric Blake" <eblake@redhat.com>,
	"Dr. David Alan Gilbert" <dgilbert@redhat.com>
Subject: Re: [PATCH V7 1/6] qapi/net: Add IPFlowSpec and QMP command for COLO passthrough
Date: Fri, 11 Jun 2021 11:36:51 +0200	[thread overview]
Message-ID: <87mtrwspxo.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <9a7a70304c304d2f99e5ee952eddf49f@intel.com> (Chen Zhang's message of "Fri, 4 Jun 2021 15:57:27 +0000")

"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