Re: [Qemu-devel] [PATCH 0/9] For QEMU 2.5: Add a net filter and a netbuffer plugin based on the filter

[Yang Hongyang]

Thank you, and sorry to Daniel that I forgot to CC you...

On 07/25/2015 01:06 PM, zhanghailiang wrote:
[...]
> >                  +--------------+       +-------------+
> >    +----------+  |    filter    |       |frontend(NIC)|
> >    |      peer+-->              |       |             |
> >    | network  <--+backend       <-------+ peer        |
> >    | backend  |  |         peer +------->             |
> >    +----------+  +--------------+       +-------------+
> >
> > Usage:
> >    -netdev tap,id=bn0  # you can use whatever backend as needed
> >    -netdev filter,id=f0,backend=bn0
> >    -netdev filter-<plugin>,id=p0,filter=f0
> >    -device e1000,netdev=f0
>
> Have you considered Daniel's suggestion ? Using the bellow style:

Yes, but by dig into the implementation, I think the current way is better,
here is the reason:

1. The flexibility to easily dynamically add/remove/change filters on the fly.
   This is what Daniel was worrying about (please correct me if I didn't
   get your point) I think I addressed his main concern on this series.
   And you can specify any param to the filter plugin under existing netdev
   design.

2. Reuse as many existing code as we can. think about doing a standalone object,
   add/remove filters will duplicate the existing netdev_add/netdev_del code.
   the filter plugin need to implement at lease 3 functions, init/cleanup and
   receive_filter, this is also duplicate the existing netdev design, we already
   have the architecture to init/cleanup/receive_filter, why not use it?

3. A filter is a backend in my design, so it is absolutely reasonable to
   implement it as a netdev and it is easy to implement it as a netdev, if
   you implement it as a standalone object, how do you integrate it to the
   backend? A filter plugin might be able to be a standalone object, but just
   as I said on #2, as long as we can archive our goal under the existing
   design, why duplicate it?

4. Current implementation don't affact the existing code too much, it is a
   bolt-on feature.

Overall, we reuse the existing design, implemented a flexible and extensible
net filter feature, so I think the current way is better.


>    -netfilter id=f0,plugin=dump
>    -netdev tap,id=bn0,filter=f0
>    -device e1000,netdev=bn0
>
> Considering the filter as a new 'netdev' seems to be unreasonable,
> Whenever we add a new plugin, we have to add a new member to
> 'NetClientOptions', there will be lots of 'filter' objects in NetClientOptions
> area.

Why can't we extend this struct? I don't see any problem with this. Doing the
other way is just to extend another struct.

 Besides when we want to describe a net device with several filter plugin
> for VM,
> it will become like:
>     -netdev tap,id=bn0
>     -netdev filter,id=f0,backend=bn0
>     -netdev filter-<plugin-0>,id=p0,filter=f0
>     -netdev filter-<plugin-1>,id=p1,filter=f1
>     ... ...
>     -device e1000,netdev=f0
> Which is a little verbose for 'netdev' option.

It's just the name diffrence, using netfilter will be
-netfilter ... -netfilter ...

using plugin=xxx will make us hard to extend the plugin params under existing
netdev design thus will needs lots of extra effort to archive our goal, but we
already have a simple way, do we? and do note that Daniel's concern was based
on my initial RFC patch, which has a usage about "plugin=xxx", this series
is totally different.

> We'd better come to an agreement on the command line style for net filter :)

This is my opinion, Daniel, what do you think?

> Cc: Daniel P. Berrange <address@hidden>
>
> Thanks,
> zhanghailiang
>
> > NOTE:
> >    You can attach multiple plugins to the filter, dynamically add/remove
> > filter and filter-<plugin>.
> >
> > The netbuffer plugin:
> > Usage:
> >    -netdev tap,id=bn0  # you can use whatever backend as needed
> >    -netdev filter,id=f0,backend=bn0
> >    -netdev filter-buffer,id=p0,filter=f0
> >    -device e1000,netdev=f0
> >
> > Will supply a public API to release the buffer. But there's no
> > callers currently.
> > To test this feature, it's quite simple, just use
> > netdev_add filter-buffer,id=p0,filter=f0
> > to buffer packets,
> > netdev_del p0
> > will release packets.
> >
> > You can also implement whatever plugin you needed based on this filter.
> >
> > Yang Hongyang (9):
> >    netdev: Add a net filter
> >    virtio-net: add filter support
> >    filter: remove plugins when remove filter
> >    filter: remove filter before remove network backend
> >    filter: add netbuffer plugin
> >    introduce qemu_find_net_clients_by_model
> >    net/queue: export qemu_net_queue_append
> >    move out net queue structs define
> >    add a public api to release buffer
> >
> >   hw/net/virtio-net.c  |  17 ++-
> >   include/net/filter.h |  21 ++++
> >   include/net/net.h    |   5 +
> >   include/net/queue.h  |  26 ++++
> >   net/Makefile.objs    |   2 +
> >   net/clients.h        |   6 +
> >   net/filter-buffer.c  | 185 ++++++++++++++++++++++++++++
> >   net/filter.c         | 331 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >   net/net.c            |  51 +++++++-
> >   net/queue.c          |  31 +----
> >   qapi-schema.json     |  40 ++++++-
> >   11 files changed, 679 insertions(+), 36 deletions(-)
> >   create mode 100644 include/net/filter.h
> >   create mode 100644 net/filter-buffer.c
> >   create mode 100644 net/filter.c
>
>
> .

--
Thanks,
Yang.