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

[Zhang, Chen]

> -----Original Message-----
> From: Markus Armbruster <armbru@redhat.com>
> Sent: Friday, June 11, 2021 5:37 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: Eric Blake <eblake@redhat.com>; 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>;
> Dr. David Alan Gilbert <dgilbert@redhat.com>
> Subject: Re: [PATCH V7 1/6] qapi/net: Add IPFlowSpec and QMP command
> for COLO passthrough
> 
> "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.

OK, I will add this to commit log and qapi/net.json  in next version.

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

Yes.

> 
> >> > +#
> >> > +# @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.
> 

Yes, As we discussed In V4, already changed the "id" to "object-name".
https://lists.gnu.org/archive/html/qemu-devel/2021-04/msg01088.html 

And sorry I missed another mail thread to add the comments, I will add the 
explanation and quick update to V8 for this series.
Thanks for your help~

Thanks
Chen

> 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