RE: [PATCH V4 3/7] qapi/net: Add new QMP command for COLO passthrough

[Zhang, Chen]

> -----Original Message-----
> From: Markus Armbruster <armbru@redhat.com>
> Sent: Monday, March 22, 2021 8:16 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: Lukas Straub <lukasstraub2@web.de>; Li Zhijian
> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; qemu-
> dev <qemu-devel@nongnu.org>; Dr. David Alan Gilbert
> <dgilbert@redhat.com>; Zhang Chen <zhangckid@gmail.com>
> Subject: Re: [PATCH V4 3/7] qapi/net: Add new QMP command for COLO
> passthrough
> 
> "Zhang, Chen" <chen.zhang@intel.com> writes:
> 
> >> -----Original Message-----
> >> From: Markus Armbruster <armbru@redhat.com>
> >> Sent: Saturday, March 20, 2021 12:03 AM
> >> To: Zhang, Chen <chen.zhang@intel.com>
> >> Cc: Jason Wang <jasowang@redhat.com>; qemu-dev <qemu-
> >> devel@nongnu.org>; Eric Blake <eblake@redhat.com>; Dr. David Alan
> >> Gilbert <dgilbert@redhat.com>; Li Zhijian <lizhijian@cn.fujitsu.com>;
> >> Lukas Straub <lukasstraub2@web.de>; Zhang Chen
> <zhangckid@gmail.com>
> >> Subject: Re: [PATCH V4 3/7] qapi/net: Add new QMP command for COLO
> >> passthrough
> >>
> >> Zhang Chen <chen.zhang@intel.com> writes:
> >>
> >> > 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.
> >> >
> >> > Signed-off-by: Zhang Chen <chen.zhang@intel.com>
> >> > ---
> >> >  net/net.c     | 10 ++++++++++
> >> >  qapi/net.json | 40 ++++++++++++++++++++++++++++++++++++++++
> >> >  2 files changed, 50 insertions(+)
> >> >
> >> > diff --git a/net/net.c b/net/net.c
> >> > index 725a4e1450..7c7cefe0e0 100644
> >> > --- a/net/net.c
> >> > +++ b/net/net.c
> >> > @@ -1199,6 +1199,16 @@ void qmp_netdev_del(const char *id, Error
> >> **errp)
> >> >      }
> >> >  }
> >> >
> >> > +void qmp_colo_passthrough_add(L4_Connection *conn, Error **errp)
> {
> >> > +    /* Setup passthrough connection */
> >>
> >> Do you mean to say
> >>
> >>        /* TODO implement */
> >>
> >> ?
> >
> > Yes, I will input real code here in 7/7 patch.
> 
> Use a TODO comment then.
> 
> >>
> >> > +}
> >> > +
> >> > +void qmp_colo_passthrough_del(L4_Connection *conn, Error **errp)
> {
> >> > +    /* Delete passthrough connection */ }
> >>
> >> Likewise.
> >>
> >> > +
> >> >  static void netfilter_print_info(Monitor *mon, NetFilterState *nf)  {
> >> >      char *str;
> >> > diff --git a/qapi/net.json b/qapi/net.json index
> >> > cd4a8ed95e..ec7d3b1128 100644
> >> > --- a/qapi/net.json
> >> > +++ b/qapi/net.json
> >> > @@ -851,3 +851,43 @@
> >> >    'data': { 'protocol': 'IP_PROTOCOL', '*id': 'str', '*src_ip': 'str', 
> >> > '*dst_ip':
> 'str',
> >> >      '*src_port': 'int', '*dst_port': 'int' } }
> >> >
> >> > +##
> >> > +# @colo-passthrough-add:
> >> > +#
> >> > +# Add passthrough entry according to customer's needs in COLO-
> compare.
> >>
> >> QEMU doesn't have customers, it has users :)
> >
> > Thanks note.
> >
> >>
> >> > +#
> >> > +# Returns: Nothing on success
> >> > +#
> >> > +# Since: 6.1
> >> > +#
> >> > +# Example:
> >> > +#
> >> > +# -> { "execute": "colo-passthrough-add",
> >> > +#      "arguments": { "protocol": "tcp", "id": "object0", "src_ip":
> "192.168.1.1",
> >> > +#      "dst_ip": "192.168.1.2", "src_port": 1234, "dst_port": 4321 } }
> >> > +# <- { "return": {} }
> >> > +#
> >> > +##
> >> > +{ 'command': 'colo-passthrough-add', 'boxed': true,
> >> > +     'data': 'L4_Connection' }
> >> > +
> >> > +##
> >> > +# @colo-passthrough-del:
> >> > +#
> >> > +# Delete passthrough entry according to customer's needs in COLO-
> compare.
> >> > +#
> >> > +# Returns: Nothing on success
> >> > +#
> >> > +# Since: 6.1
> >> > +#
> >> > +# Example:
> >> > +#
> >> > +# -> { "execute": "colo-passthrough-del",
> >> > +#      "arguments": { "protocol": "tcp", "id": "object0", "src_ip":
> "192.168.1.1",
> >> > +#      "dst_ip": "192.168.1.2", "src_port": 1234, "dst_port": 4321 } }
> >> > +# <- { "return": {} }
> >> > +#
> >> > +##
> >> > +{ 'command': 'colo-passthrough-del', 'boxed': true,
> >> > +     'data': 'L4_Connection' }
> >> > +
> >>
> >> To make sense of this, I have to refer back to PATCH 1 and 2:
> >>
> >>    { 'enum': 'IP_PROTOCOL', 'data': [ 'tcp', 'udp', 'dccp', 'sctp', 
> >> 'udplite',
> >>        'icmp', 'igmp', 'ipv6' ] }
> >>
> >>    { 'struct': 'L4_Connection',
> >>      'data': { 'protocol': 'IP_PROTOCOL', '*id': 'str', '*src_ip': 'str', 
> >> '*dst_ip':
> 'str',
> >>        '*src_port': 'int', '*dst_port': 'int' } }
> >>
> >> Please squash the three patches together.
> >
> > OK.
> >
> >>
> >> I figure colo-passthrough-add adds some kind of packet matching
> >> thingy that can match packets by source IP, source port, destination
> >> IP, destination port, and protocol.  Correct?
> >
> > Yes, you are right.
> >
> >>
> >> The protocol is mandatory, all others are optional.  What does it
> >> mean to omit an optional one?  Match all?
> >
> > Yes, match all. The idea from Jason Wang, for example:
> > User just set the protocol/source IP(tcp/192.168.1.1) , others empty.
> > The rule will bypass all the TCP packet from the source IP.
> 
> Work this into the doc comment, please.

OK.

> 
> >> I have no idea what @id is supposed to mean.  Please explain intended
> use.
> >
> > 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.
> 
> I'm not sure I understand, but then I'm a QEMU networking ignoramus :)
> 
> Work it into the doc comment.

Sure, I will add more comments in qapi/net.json next version.

Thanks
Chen

> 
> > Thanks
> > Chen
> >
> >>
> >> I'm ignoring colo-passthrough-del for now, because I feel need to
> >> understand -add first.