Hi, Ben,
I think a more line "mirrors &= ~ctx->mirrors;" is needed at the end of the
"while" loop.
Otherwise, duplicated mirror action may be applied in recursive
mirror_packet called by output_normal.
Here are the test cases:
Test case for the bug I mentioned in previous mail:
1. setup port: vnet0 tag = 201, vnet1 tag=200
2. setup flow: add-flow in_port=vnet0,actions:vnet1
3. setup mirror( mirror1 or mirror2 ). and packet from vnet0 to vnet1
a) mirror1: select_all=false, select_src_port=[ ],
select_dst_port=[vnet1], select_vlan=[200], output=vnet2
==> result: this mirror works.
b) mirror2: select_all=false, select_src_port=[vnet0],
select_dst_port=[vnet1], select_vlan=[200], output=vnet2
==> result: this mirror doesn't work.
Test case for the bug I mentioned in this mail:
1. set port: vnet0, vnet1, vnet2
2. setup 2 mirrors:
mirror1: select_all=true,output_vlan=500
mirror2: select_all=true,output_vlan=501
3. packet from vnet0, result: duplicated mirror actions are generated.
Thanks,
Huanle
2016-01-20 0:57 GMT+08:00 Ben Pfaff <b...@ovn.org>:
> On Sat, Jan 16, 2016 at 02:21:03PM +0800, hanxueluo wrote:
> > Hi, guys I have a question about the code below in function
> > "mirror_packet". Some mirrors in "mirrors" may be skipped because the
> > select_vlan doesn't match ("if (vlans && !bitmap_is_set(vlans,
> > vlan)"), but they may be useful for next xbundle. So I think we should
> > record only the mirrors which are truely applied, not all of them. am
> > I right? The code doesn't exist in old version. I'm not sure whether
> > it's added on purpose. /* Record these mirrors so that we don't mirror
> > to them again. */ ctx->mirrors |= mirrors;
>
> I think you're right. Do you have a test case? Will you take a look at
> this patch?
> http://openvswitch.org/pipermail/dev/2016-January/064625.html
>
> Thanks,
>
> Ben.
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
