On 7/17/17, 12:48 AM, "Ilya Maximets" <i.maxim...@samsung.com> wrote:

    Not a full review. And I most likely will not review the logic.
    Just one comment inline.
    
    Best regards, Ilya Maximets.
    
    > One SNAT test is based on a single ping being successful;
    > to make the result more predictable, static arp binding is now used.
    > Occasionally, tracing shows the reply side stack does not respond,
    > but this is much less common with this change.
    > I considered changing the test design itself, but I thought that
    > would not be testing the same situation.
    > 
    > Signed-off-by: Darrell Ball <dlu998 at gmail.com>
    > ---
    >  tests/system-traffic.at | 35 +++++++++++++++++++++--------------
    >  1 file changed, 21 insertions(+), 14 deletions(-)
    > 
    > diff --git a/tests/system-traffic.at b/tests/system-traffic.at
    > index 1ebf928..bc126bc 100644
    > --- a/tests/system-traffic.at
    > +++ b/tests/system-traffic.at
    > @@ -2693,8 +2693,28 @@ OVS_TRAFFIC_VSWITCHD_START()
    >  ADD_NAMESPACES(at_ns0, at_ns1)
    >  
    >  ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
    > -NS_CHECK_EXEC([at_ns0], [ip link set dev p0 address 80:88:88:88:88:88])
    > +NS_CHECK_EXEC([at_ns0], [ip link set dev p0 address e6:66:c1:11:11:11])
    > +NS_CHECK_EXEC([at_ns0], [arp -s 10.1.1.2 e6:66:c1:22:22:22])
    > +
    >  ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
    > +NS_CHECK_EXEC([at_ns1], [ip link set dev p1 address e6:66:c1:22:22:22])
    > +NS_CHECK_EXEC([at_ns1], [arp -s 10.1.1.1 e6:66:c1:11:11:11])
    > +NS_CHECK_EXEC([at_ns1], [arp -s 10.1.1.240 e6:66:c1:11:11:11])
    > +NS_CHECK_EXEC([at_ns1], [arp -s 10.1.1.241 e6:66:c1:11:11:11])
    > +NS_CHECK_EXEC([at_ns1], [arp -s 10.1.1.242 e6:66:c1:11:11:11])
    > +NS_CHECK_EXEC([at_ns1], [arp -s 10.1.1.243 e6:66:c1:11:11:11])
    > +NS_CHECK_EXEC([at_ns1], [arp -s 10.1.1.244 e6:66:c1:11:11:11])
    > +NS_CHECK_EXEC([at_ns1], [arp -s 10.1.1.245 e6:66:c1:11:11:11])
    > +NS_CHECK_EXEC([at_ns1], [arp -s 10.1.1.246 e6:66:c1:11:11:11])
    > +NS_CHECK_EXEC([at_ns1], [arp -s 10.1.1.247 e6:66:c1:11:11:11])
    > +NS_CHECK_EXEC([at_ns1], [arp -s 10.1.1.248 e6:66:c1:11:11:11])
    > +NS_CHECK_EXEC([at_ns1], [arp -s 10.1.1.249 e6:66:c1:11:11:11])
    > +NS_CHECK_EXEC([at_ns1], [arp -s 10.1.1.250 e6:66:c1:11:11:11])
    > +NS_CHECK_EXEC([at_ns1], [arp -s 10.1.1.251 e6:66:c1:11:11:11])
    > +NS_CHECK_EXEC([at_ns1], [arp -s 10.1.1.252 e6:66:c1:11:11:11])
    > +NS_CHECK_EXEC([at_ns1], [arp -s 10.1.1.253 e6:66:c1:11:11:11])
    > +NS_CHECK_EXEC([at_ns1], [arp -s 10.1.1.254 e6:66:c1:11:11:11])
    > +NS_CHECK_EXEC([at_ns1], [arp -s 10.1.1.255 e6:66:c1:11:11:11])
    
    How about to replace above lines with a loop:
    
    for i in 1 `seq 240 255`; do
        NS_CHECK_EXEC([at_ns1], [arp -s 10.1.1.$i e6:66:c1:11:11:11])
    done
    
    ?

The above comment is not unreasonable.
I’ll probably switch to use a for loop.

Thanks


    
    >  
    >  dnl Allow any traffic from ns0->ns1. Only allow nd, return traffic from 
ns1->ns0.
    >  AT_DATA([flows.txt], [dnl
    > @@ -2707,20 +2727,7 @@ dnl
    >  
in_port=2,ct_state=+trk,ct_zone=1,ip,action=ct(table=1,commit,zone=1,exec(set_field:1->ct_mark)),1
    >  table=1,in_port=2,ct_mark=1,ct_state=+rpl,ct_zone=1,ip,action=1
    >  dnl
    > -dnl ARP
    > -priority=100 arp arp_op=1 
action=move:OXM_OF_ARP_TPA[[]]->NXM_NX_REG2[[]],resubmit(,8),goto_table:10
    > -priority=10 arp action=normal
    >  priority=0,action=drop
    > -dnl
    > -dnl MAC resolution table for IP in reg2, stores mac in OXM_OF_PKT_REG0
    > 
-table=8,reg2=0x0a0101f0/0xfffffff0,action=load:0x808888888888->OXM_OF_PKT_REG0[[]]
    > -table=8,priority=0,action=load:0->OXM_OF_PKT_REG0[[]]
    > -dnl ARP responder mac filled in at OXM_OF_PKT_REG0, or 0 for normal 
action.
    > -dnl TPA IP in reg2.
    > -dnl Swaps the fields of the ARP message to turn a query to a response.
    > -table=10 priority=100 arp xreg0=0 action=normal
    > -table=10 
priority=10,arp,arp_op=1,action=load:2->OXM_OF_ARP_OP[[]],move:OXM_OF_ARP_SHA[[]]->OXM_OF_ARP_THA[[]],move:OXM_OF_PKT_REG0[[0..47]]->OXM_OF_ARP_SHA[[]],move:OXM_OF_ARP_SPA[[]]->OXM_OF_ARP_TPA[[]],move:NXM_NX_REG2[[]]->OXM_OF_ARP_SPA[[]],move:NXM_OF_ETH_SRC[[]]->NXM_OF_ETH_DST[[]],move:OXM_OF_PKT_REG0[[0..47]]->NXM_OF_ETH_SRC[[]],move:NXM_OF_IN_PORT[[]]->NXM_NX_REG3[[0..15]],load:0->NXM_OF_IN_PORT[[]],output:NXM_NX_REG3[[0..15]]
    > -table=10 priority=0 action=drop
    >  ])
    >  
    >  AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
    > -- 
    > 1.9.1
    

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to