On 2/15/16, 1:42 PM, "dev on behalf of Russell Bryant"
<dev-boun...@openvswitch.org on behalf of russ...@ovn.org> wrote:
>On 02/12/2016 08:12 PM, Darrell Ball wrote:
>> Signed-off-by: Darrell Ball <db...@vmware.com>
>> ---
>> vtep/ovs-vtep | 20 ++++++++++++++++++--
>> 1 file changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/vtep/ovs-vtep b/vtep/ovs-vtep
>> index 46a5692..54bf0db 100755
>> --- a/vtep/ovs-vtep
>> +++ b/vtep/ovs-vtep
>> @@ -135,15 +135,31 @@ class Logical_Switch(object):
>> ovs_ofctl("add-flow %s table=1,priority=1,in_port=%s,action=%s"
>> % (self.short_name, port_no, ",".join(flood_ports)))
>>
>> + existing_flow = 0
>> + appended_flood_tunnel_port = 0
>
>I would use the boolean type, instead. Instead of 0, set these both to
>False.
If this was not Python with its very “flexible” boolean object,
I would have probably favored a "boolean type" hands down
Regardless, its better to use the keywords True and False so I’ll
adopt them
Thanks
>
>> + flows = ovs_ofctl("dump-flows %s table=1,cookie=0x4848/-1"
>> + % self.short_name).splitlines()
>> + for f in flows:
>> + if ("table" in f):
>
>These parenthesis can be removed.
Sure
>
>> + existing_flow = 1
>
> existing_flow = True
ack
>
>> + break
>> +
>
>If you're just looking for "table" somewhere in the output, there's no
>need to split it up into lines first. This should be sufficient.
>
>Remove .splitlines() and you can replace the for loop with:
>
> existing_flow = flows.find('table') > -1
I was on the fence on using the find method
Based on time efficiency, the two options are pretty much the same for the
associated specific use cases,
However, I considered efficiency almost irrelevant for the vtep-emulator :-)
I tried to keep the “in” operator semantics since its superior over “find” in
isolation,
but the for loop part is ugly; I can use the find method
>
>> # Traffic coming from a VTEP physical port should only be flooded to
>> # one 'unknown-dst' and to all other physical ports that belong to
>> that
>> # VTEP device and this logical switch.
>> for tunnel in self.unknown_dsts:
>> port_no = self.tunnels[tunnel][0]
>> flood_ports.append(port_no)
>> + appended_flood_tunnel_port = 1
>
> appended_flood_tunnel_port = True
Ack
>
>> break
>> -
>> - ovs_ofctl("add-flow %s table=1,priority=0,action=%s"
>> + # Handle remote tunnel port set changes
>> + if ((existing_flow == 1) and (appended_flood_tunnel_port == 1)):
>
> if existing_flow and appended_flood_tunnel_port:
I’ll be using
if existing_flow == True and appended_flood_tunnel_port == True:
semantics
My reasoning is with Python booleans, I can assign 2
or even ‘apple’ to existing_flow and the check
if existing_flow:
would evaluate to True
>
>> + ovs_ofctl(
>> + "mod-flows %s
>> table=1,priority=0,cookie=0x4848/-1,action=%s"
>> + % (self.short_name, ",".join(flood_ports)))
>> + elif (existing_flow == 0):
>
> elif not existing_flow:
Sure
>
>> + ovs_ofctl(
>> + "add-flow %s table=1,priority=0,cookie=0x4848,action=%s"
>> % (self.short_name, ",".join(flood_ports)))
>>
>> def add_lbinding(self, lbinding):
>>
>
>
>--
>Russell Bryant
>_______________________________________________
>dev mailing list
>dev@openvswitch.org
>http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
