RE: [RFC PATCH 08/12] qapi: Add FilterUbpfProperties and qemu-options

2022-06-20 Thread Zhang, Chen



> -Original Message-
> From: Markus Armbruster 
> Sent: Monday, June 20, 2022 3:45 PM
> To: Zhang, Chen 
> Cc: Jason Wang ; qemu-dev  de...@nongnu.org>; Paolo Bonzini ; Daniel
> P.Berrangé ; Eduardo Habkost
> ; Eric Blake ; Peter Maydell
> ; Thomas Huth ; Laurent
> Vivier ; Yuri Benditovich
> ; Andrew Melnychenko
> 
> Subject: Re: [RFC PATCH 08/12] qapi: Add FilterUbpfProperties and qemu-
> options
> 
> Zhang Chen  writes:
> 
> > Add filter-ubpf related QOM and qemu-options.
> >
> > Signed-off-by: Zhang Chen 
> > ---
> >  qapi/qom.json   | 18 ++
> >  qemu-options.hx |  6 ++
> >  2 files changed, 24 insertions(+)
> >
> > diff --git a/qapi/qom.json b/qapi/qom.json index
> > 6a653c6636..820a5218e8 100644
> > --- a/qapi/qom.json
> > +++ b/qapi/qom.json
> > @@ -444,6 +444,22 @@
> >'base': 'NetfilterProperties',
> >'data': { '*vnet_hdr_support': 'bool' } }
> >
> > +##
> > +# @FilterUbpfProperties:
> > +#
> > +# Properties for filter-ubpf objects.
> > +#
> > +# @ip-mode: if true, IP packet handle mode is enabled(default: true).
> 
> Space between "enabled" and "(default: true)", please.
> 
> I'm not sure about the name @ip-mode.  A mode tends to be an enum.  A
> boolean tends to be a flag, like @disable-packed-handle-mode.  Note that I
> reverted the sense there, to make the default false.

Thanks for your review Markus~

Makes sense. Current mode just include ip-mode and raw-mode, it looks
Need change to a enum mode for it, maybe will add other mode in the future.

> 
> > +#
> > +# @ubpf-handler: The filename where the userspace ebpf packets
> handler.
> > +#
> > +# Since: 7.1
> > +##
> > +{ 'struct': 'FilterUbpfProperties',
> > +  'base': 'NetfilterProperties',
> > +  'data': { '*ip-mode': 'bool',
> > +'*ubpf-handler': 'str' } }
> > +
> >  ##
> >  # @InputBarrierProperties:
> >  #
> > @@ -845,6 +861,7 @@
> >  'filter-redirector',
> >  'filter-replay',
> >  'filter-rewriter',
> > +'filter-ubpf',
> >  'input-barrier',
> >  { 'name': 'input-linux',
> >'if': 'CONFIG_LINUX' },
> > @@ -911,6 +928,7 @@
> >'filter-redirector':  'FilterRedirectorProperties',
> >'filter-replay':  'NetfilterProperties',
> >'filter-rewriter':'FilterRewriterProperties',
> > +  'filter-ubpf':'FilterUbpfProperties',
> >'input-barrier':  'InputBarrierProperties',
> >'input-linux':{ 'type': 'InputLinuxProperties',
> >'if': 'CONFIG_LINUX' }, diff
> > --git a/qemu-options.hx b/qemu-options.hx index 60cf188da4..3dfb858867
> > 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -5080,6 +5080,12 @@ SRST
> >  stored. The file format is libpcap, so it can be analyzed with
> >  tools such as tcpdump or Wireshark.
> >
> > +``-object filter-ubpf,id=id,netdev=dev,ubpf-handler=filename[,ip-
> mode][,position=head|tail|id=][,insert=behind|before]``
> > +filter-ubpf is the userspace ebpf network traffic handler on netdev
> dev
> > +from the userspace ebpf handler file specified by filename.
> 
> I believe the conventional capitalization is eBPF.

Yes, will fix it.

> 
> > +If disable ip_mode, the loaded ebpf program will handle raw
> 
> Markup: ``ip_mode``.

OK.

> 
> > +network packet.
> 
> Suggest something like "If ``ip_mode`` is off, the eBPF program is fed raw
> network packets" (hope I'm not misinterpreting things).

OK. I will address your comments in next version.

Thanks
Chen

> 
> > +
> >  ``-object colo-
> compare,id=id,primary_in=chardevid,secondary_in=chardevid,outdev=chard
> evid,iothread=id[,vnet_hdr_support][,notify_dev=id][,compare_timeout=@
> var{ms}][,expired_scan_cycle=@var{ms}][,max_queue_size=@var{size}]``
> >  Colo-compare gets packet from primary\_in chardevid and
> >  secondary\_in, then compare whether the payload of primary
> > packet




Re: [RFC PATCH 08/12] qapi: Add FilterUbpfProperties and qemu-options

2022-06-20 Thread Markus Armbruster
Zhang Chen  writes:

> Add filter-ubpf related QOM and qemu-options.
>
> Signed-off-by: Zhang Chen 
> ---
>  qapi/qom.json   | 18 ++
>  qemu-options.hx |  6 ++
>  2 files changed, 24 insertions(+)
>
> diff --git a/qapi/qom.json b/qapi/qom.json
> index 6a653c6636..820a5218e8 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -444,6 +444,22 @@
>'base': 'NetfilterProperties',
>'data': { '*vnet_hdr_support': 'bool' } }
>  
> +##
> +# @FilterUbpfProperties:
> +#
> +# Properties for filter-ubpf objects.
> +#
> +# @ip-mode: if true, IP packet handle mode is enabled(default: true).

Space between "enabled" and "(default: true)", please.

I'm not sure about the name @ip-mode.  A mode tends to be an enum.  A
boolean tends to be a flag, like @disable-packed-handle-mode.  Note that
I reverted the sense there, to make the default false.

> +#
> +# @ubpf-handler: The filename where the userspace ebpf packets handler.
> +#
> +# Since: 7.1
> +##
> +{ 'struct': 'FilterUbpfProperties',
> +  'base': 'NetfilterProperties',
> +  'data': { '*ip-mode': 'bool',
> +'*ubpf-handler': 'str' } }
> +
>  ##
>  # @InputBarrierProperties:
>  #
> @@ -845,6 +861,7 @@
>  'filter-redirector',
>  'filter-replay',
>  'filter-rewriter',
> +'filter-ubpf',
>  'input-barrier',
>  { 'name': 'input-linux',
>'if': 'CONFIG_LINUX' },
> @@ -911,6 +928,7 @@
>'filter-redirector':  'FilterRedirectorProperties',
>'filter-replay':  'NetfilterProperties',
>'filter-rewriter':'FilterRewriterProperties',
> +  'filter-ubpf':'FilterUbpfProperties',
>'input-barrier':  'InputBarrierProperties',
>'input-linux':{ 'type': 'InputLinuxProperties',
>'if': 'CONFIG_LINUX' },
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 60cf188da4..3dfb858867 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -5080,6 +5080,12 @@ SRST
>  stored. The file format is libpcap, so it can be analyzed with
>  tools such as tcpdump or Wireshark.
>  
> +``-object 
> filter-ubpf,id=id,netdev=dev,ubpf-handler=filename[,ip-mode][,position=head|tail|id=][,insert=behind|before]``
> +filter-ubpf is the userspace ebpf network traffic handler on netdev 
> dev
> +from the userspace ebpf handler file specified by filename.

I believe the conventional capitalization is eBPF.

> +If disable ip_mode, the loaded ebpf program will handle raw

Markup: ``ip_mode``.

> +network packet.

Suggest something like "If ``ip_mode`` is off, the eBPF program is fed
raw network packets" (hope I'm not misinterpreting things).

> +
>  ``-object 
> colo-compare,id=id,primary_in=chardevid,secondary_in=chardevid,outdev=chardevid,iothread=id[,vnet_hdr_support][,notify_dev=id][,compare_timeout=@var{ms}][,expired_scan_cycle=@var{ms}][,max_queue_size=@var{size}]``
>  Colo-compare gets packet from primary\_in chardevid and
>  secondary\_in, then compare whether the payload of primary packet