diff mbox series

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

Message ID 20210319035508.113741-4-chen.zhang@intel.com
State New
Headers show
Series Bypass specific network traffic in COLO | expand

Commit Message

Zhang, Chen March 19, 2021, 3:55 a.m. UTC
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(+)

Comments

Markus Armbruster March 19, 2021, 4:03 p.m. UTC | #1
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 */

?

> +}
> +
> +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 :)

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

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?

The protocol is mandatory, all others are optional.  What does it mean
to omit an optional one?  Match all?

I have no idea what @id is supposed to mean.  Please explain intended
use.

I'm ignoring colo-passthrough-del for now, because I feel need to
understand -add first.
Zhang, Chen March 22, 2021, 9:59 a.m. UTC | #2
> -----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.

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

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

Thanks
Chen

> 
> I'm ignoring colo-passthrough-del for now, because I feel need to
> understand -add first.
Markus Armbruster March 22, 2021, 12:16 p.m. UTC | #3
"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.
Markus Armbruster March 22, 2021, 12:36 p.m. UTC | #4
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 */
> +}
> +
> +void qmp_colo_passthrough_del(L4_Connection *conn, Error **errp)
> +{
> +    /* Delete passthrough connection */
> +}
> +
>  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.
> +#
> +# 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' }
> +

Now let's look at colo-passthrough-del.  I figure it is for deleting the
kind of things colo-passthrough-add adds.

What exactly is deleted?  The thing created with the exact same
arguments?

This would be unusual.  Commonly, FOO-add and FOO-del both take a string
ID argument.  The FOO created by FOO-add remembers its ID, and FOO-del
deletes by ID.
Zhang, Chen March 23, 2021, 9:06 a.m. UTC | #5
> -----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.
Zhang, Chen March 23, 2021, 9:19 a.m. UTC | #6
> -----Original Message-----
> From: Markus Armbruster <armbru@redhat.com>
> Sent: Monday, March 22, 2021 8:36 PM
> 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 */ }
> > +
> > +void qmp_colo_passthrough_del(L4_Connection *conn, Error **errp) {
> > +    /* Delete passthrough connection */ }
> > +
> >  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.
> > +#
> > +# 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' }
> > +
> 
> Now let's look at colo-passthrough-del.  I figure it is for deleting the kind of
> things colo-passthrough-add adds.
> 

Yes.

> What exactly is deleted?  The thing created with the exact same arguments?
> 

Delete the rule from the module's private bypass list.
When user input a rule, the colo-passthrough-del will find the specific module by the object ID,
Then delete the rule.

> This would be unusual.  Commonly, FOO-add and FOO-del both take a string
> ID argument.  The FOO created by FOO-add remembers its ID, and FOO-del
> deletes by ID.

The ID not for rules itself, it just logged the modules(ID tagged) affected by the rule.

Thanks
Chen
Markus Armbruster March 23, 2021, 9:58 a.m. UTC | #7
"Zhang, Chen" <chen.zhang@intel.com> writes:

>> -----Original Message-----
>> From: Markus Armbruster <armbru@redhat.com>
[...]
>> Now let's look at colo-passthrough-del.  I figure it is for deleting the kind of
>> things colo-passthrough-add adds.
>> 
>
> Yes.
>
>> What exactly is deleted?  The thing created with the exact same arguments?
>> 
>
> Delete the rule from the module's private bypass list.
> When user input a rule, the colo-passthrough-del will find the specific module by the object ID,
> Then delete the rule.
>
>> This would be unusual.  Commonly, FOO-add and FOO-del both take a string
>> ID argument.  The FOO created by FOO-add remembers its ID, and FOO-del
>> deletes by ID.
>
> The ID not for rules itself, it just logged the modules(ID tagged) affected by the rule.

I'm not sure I understand.

If you're pointing out that existing colo-passthrough-del parameter @id
is not suitable for use as unique rule ID: you can always add another
parameter that is suitable.
Zhang, Chen March 30, 2021, 3:38 a.m. UTC | #8
> -----Original Message-----
> From: Qemu-devel <qemu-devel-
> bounces+chen.zhang=intel.com@nongnu.org> On Behalf Of Markus
> Armbruster
> Sent: Tuesday, March 23, 2021 5:58 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>
> [...]
> >> Now let's look at colo-passthrough-del.  I figure it is for deleting
> >> the kind of things colo-passthrough-add adds.
> >>
> >
> > Yes.
> >
> >> What exactly is deleted?  The thing created with the exact same
> arguments?
> >>
> >
> > Delete the rule from the module's private bypass list.
> > When user input a rule, the colo-passthrough-del will find the
> > specific module by the object ID, Then delete the rule.
> >
> >> This would be unusual.  Commonly, FOO-add and FOO-del both take a
> >> string ID argument.  The FOO created by FOO-add remembers its ID, and
> >> FOO-del deletes by ID.
> >
> > The ID not for rules itself, it just logged the modules(ID tagged) affected by
> the rule.
> 
> I'm not sure I understand.
> 
> If you're pointing out that existing colo-passthrough-del parameter @id is not
> suitable for use as unique rule ID: you can always add another parameter
> that is suitable.

Sorry to missed this mail.

For example:
The VM running with filter-mirror(object id==0), filter-redirector(object id==1) and colo-compare(object id==2),
We use  colo-passthrough-add/del to add/del a rule with a ID, if the ID==2, the rule just affect to colo-compare.
The filter-mirror and filter-redirector feel nothing after the add/del.

Thanks
Chen

>
Markus Armbruster April 6, 2021, 8:01 a.m. UTC | #9
"Zhang, Chen" <chen.zhang@intel.com> writes:

>> -----Original Message-----
>> From: Qemu-devel <qemu-devel-
>> bounces+chen.zhang=intel.com@nongnu.org> On Behalf Of Markus
>> Armbruster
>> Sent: Tuesday, March 23, 2021 5:58 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>
>> [...]
>> >> Now let's look at colo-passthrough-del.  I figure it is for deleting
>> >> the kind of things colo-passthrough-add adds.
>> >>
>> >
>> > Yes.
>> >
>> >> What exactly is deleted?  The thing created with the exact same
>> arguments?
>> >>
>> >
>> > Delete the rule from the module's private bypass list.
>> > When user input a rule, the colo-passthrough-del will find the
>> > specific module by the object ID, Then delete the rule.
>> >
>> >> This would be unusual.  Commonly, FOO-add and FOO-del both take a
>> >> string ID argument.  The FOO created by FOO-add remembers its ID, and
>> >> FOO-del deletes by ID.
>> >
>> > The ID not for rules itself, it just logged the modules(ID tagged) affected by
>> the rule.
>> 
>> I'm not sure I understand.
>> 
>> If you're pointing out that existing colo-passthrough-del parameter @id is not
>> suitable for use as unique rule ID: you can always add another parameter
>> that is suitable.
>
> Sorry to missed this mail.
>
> For example:
> The VM running with filter-mirror(object id==0), filter-redirector(object id==1) and colo-compare(object id==2),
> We use  colo-passthrough-add/del to add/del a rule with a ID, if the ID==2, the rule just affect to colo-compare.
> The filter-mirror and filter-redirector feel nothing after the add/del.

I think you're trying to explain existing parameter @id.  The point I
was trying to make is unrelated to this parameter, except by name
collision.

My point is: our existing "delete" operations select the object to be
deleted by some unique name that is assigned by the "add" operation.
The unique name is a property of the object.  The property name is
often, but not always "id".

Examples:

    device_add argument "id" sets the device's unique name.
    device_del argument "id" selects the device to delete by its name.

    blockdev-add argument "node-name" sets the block backend device's
    unique name.
    blockdev-del argument "node-name" selects the block backend device
    to delete by its name.

Is there any particular reason why deletion of your kind of object can't
work the same way?
Zhang, Chen April 8, 2021, 3:24 a.m. UTC | #10
> -----Original Message-----
> From: Markus Armbruster <armbru@redhat.com>
> Sent: Tuesday, April 6, 2021 4:01 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: Qemu-devel <qemu-devel-
> >> bounces+chen.zhang=intel.com@nongnu.org> On Behalf Of Markus
> >> Armbruster
> >> Sent: Tuesday, March 23, 2021 5:58 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>
> >> [...]
> >> >> Now let's look at colo-passthrough-del.  I figure it is for
> >> >> deleting the kind of things colo-passthrough-add adds.
> >> >>
> >> >
> >> > Yes.
> >> >
> >> >> What exactly is deleted?  The thing created with the exact same
> >> arguments?
> >> >>
> >> >
> >> > Delete the rule from the module's private bypass list.
> >> > When user input a rule, the colo-passthrough-del will find the
> >> > specific module by the object ID, Then delete the rule.
> >> >
> >> >> This would be unusual.  Commonly, FOO-add and FOO-del both take a
> >> >> string ID argument.  The FOO created by FOO-add remembers its ID,
> >> >> and FOO-del deletes by ID.
> >> >
> >> > The ID not for rules itself, it just logged the modules(ID tagged)
> >> > affected by
> >> the rule.
> >>
> >> I'm not sure I understand.
> >>
> >> If you're pointing out that existing colo-passthrough-del parameter
> >> @id is not suitable for use as unique rule ID: you can always add
> >> another parameter that is suitable.
> >
> > Sorry to missed this mail.
> >
> > For example:
> > The VM running with filter-mirror(object id==0),
> > filter-redirector(object id==1) and colo-compare(object id==2), We use
> colo-passthrough-add/del to add/del a rule with a ID, if the ID==2, the rule
> just affect to colo-compare.
> > The filter-mirror and filter-redirector feel nothing after the add/del.
> 
> I think you're trying to explain existing parameter @id.  The point I was trying
> to make is unrelated to this parameter, except by name collision.
> 
> My point is: our existing "delete" operations select the object to be deleted
> by some unique name that is assigned by the "add" operation.
> The unique name is a property of the object.  The property name is often,
> but not always "id".
> 
> Examples:
> 
>     device_add argument "id" sets the device's unique name.
>     device_del argument "id" selects the device to delete by its name.
> 
>     blockdev-add argument "node-name" sets the block backend device's
>     unique name.
>     blockdev-del argument "node-name" selects the block backend device
>     to delete by its name.
> 
> Is there any particular reason why deletion of your kind of object can't work
> the same way?

Current command can work in this way, It seems that name "ID" can be misunderstood.
The id=object0 is OK here. I will change the "id" to "object-name".
Thank you for clear the comments.

Thanks   
Chen
diff mbox series

Patch

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 */
+}
+
+void qmp_colo_passthrough_del(L4_Connection *conn, Error **errp)
+{
+    /* Delete passthrough connection */
+}
+
 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.
+#
+# 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' }
+