qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH RFC v2 3/5] net/filter: Introduce a helper to ad


From: Hailiang Zhang
Subject: Re: [Qemu-devel] [PATCH RFC v2 3/5] net/filter: Introduce a helper to add a filter to the netdev
Date: Mon, 1 Feb 2016 20:21:31 +0800
User-agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1

Hi Jason,

On 2016/2/1 17:42, Jason Wang wrote:


On 02/01/2016 05:22 PM, Hailiang Zhang wrote:
On 2016/2/1 17:04, Jason Wang wrote:


On 02/01/2016 03:56 PM, Hailiang Zhang wrote:
On 2016/2/1 15:46, Jason Wang wrote:


On 02/01/2016 02:13 PM, Hailiang Zhang wrote:
On 2016/2/1 11:14, Jason Wang wrote:


On 01/27/2016 04:29 PM, zhanghailiang wrote:
We add a new helper function netdev_add_filter(), this function
can help adding a filter object to a netdev.
Besides, we add a is_default member for struct NetFilterState
to indicate whether the filter is default or not.

Signed-off-by: zhanghailiang <address@hidden>
---
v2:
     -Re-implement netdev_add_filter() by re-using object_create()
      (Jason's suggestion)
---
     include/net/filter.h |  7 +++++
     net/filter.c         | 80
++++++++++++++++++++++++++++++++++++++++++++++++++++
     2 files changed, 87 insertions(+)



[...]

+
+    optarg =
g_strdup_printf("qom-type=%s,id=%s,netdev=%s,status=%s",
+            filter_type, id, netdev_id, is_default ? "disable" :
"enable"

Instead of this, I wonder maybe it's better to:

- store the default filter property into a pointer to string

Do you mean, pass a string parameter which stores the filter property
instead of
assemble it in this helper ?

Yes. E.g just a global string which could be changed by any subsystem.
E.g colo may change it to
"filter-buffer,interval=0,status=disable". But
filter ids need to be generated automatically.


Got it. Then we don't need the global default_netfilter_type[] in
patch 5,

Yes.

Just use this global string instead ?

Right.



- colo code may change the pointer to "filter-buffer,status=disable"


Then, there's no need for lots of codes above:
- no need a "is_default" parameter in netdev_add_filter which
does not
scale consider we may want to have more property in the future
- no need to hacking like "qemu_filter_opts"

Yes, we can use qemu_find_opts("object") instead of it.

- no need to have a special flag like "is_default"


But we have to distinguish the default filter from the common
filter, use the name (id) to distinguish it ?

What's the reason that you want to distinguish default filters from
others?


The default filters will be used by COLO or MC, (In COLO, we will
use it
to control packets buffering/releasing).

A question is how will you do this?


Er, for COLO, we will enable all the default filter in the
initialization stage,
then the buffer-filter will buffer all netdev's packets,
after doing a checkpoint, we will release all the buffered packets
(Flush all default
filters' packets).

Right, that's the point. So looks several choices here:

1) Track each default filter explicitly, generate and record the netdev
ids for default filter by COLO.  Walk through the ids list and release
the packet in each filter.
2) Track the default filters implicitly. Link all buffer filters (with
zero interval) in a list during filter buffer initialization. And export
a helper for COLO to walk them and release packets.

Either looks fine, but maybe 2 is easier.


Danies recommended using object_new_with_props() instead of object_create(),
which will avoids the need of format + parse process. It makes the codes
more clean. I have fixed it in v3.

I kept the 'is_default' member for struct NetFilterState. Because comparing
with finding the default filter by the special name ([netdev->id][nop]),
it seems to be more easy for COLO to find/control the default filter with
this flag.
(We added a new helper qemu_foreach_netfilter() in COLO frame, and
we traverse these filters directly without passing any netdev related info.)

But if you think it is still not acceptable to add such a member, i'll remove it
in next verison ;)

Thanks,
Hailiang


  If VM is failover, we will set all default filters back to disabled
status.
(This is a periodic mode for COLO, different from another mode, which
we will call it
hybrid mode, that is based on colo-proxy, which is in developing by
zhangchen)

Thanks,
Hailiang

Yes, I see.


.






reply via email to

[Prev in Thread] Current Thread [Next in Thread]