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

[Markus Armbruster]

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

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

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