[ovs-dev] [PATCH 1/3] ovs-vsctl: Refactor internals to increase flexibility.

Ben Pfaff blp at nicira.com
Fri Oct 16 16:36:11 UTC 2009


<reid at nicira.com> writes:

> I didn't see anything in cfg_read, but I assume the code prevents adding a
> port to more than one bridge or checks on duplicates, since we seem to take
> the first one iterated to (which may be non-deterministic since cfg is a
> dictionary, and cause confusing errors).

Yes, the code prevents adding duplicates (see check_conflicts).
Maybe I'll add a test for this though.

More below.

> On Thu, 15 Oct 2009 15:38:49 -0700, Ben Pfaff <blp at nicira.com> wrote:
>> This changes the interface of each of the command implementations, making
>> them take the configuration as an argument and return the output.  This
>> will make it easier to support alternate output formats and to execute
>> more
>> than one command per invocation (both happening in upcoming commits).
>> ---
>>  utilities/ovs-vsctl.in |  101
>> +++++++++++++++++++++--------------------------
>>  1 files changed, 45 insertions(+), 56 deletions(-)
>> 
>> diff --git a/utilities/ovs-vsctl.in b/utilities/ovs-vsctl.in
>> index ac55c42..23d8ac0 100755
>> --- a/utilities/ovs-vsctl.in
>> +++ b/utilities/ovs-vsctl.in
>> -def cmd_list_ports(bridge):
>> -    cfg = cfg_read(VSWITCHD_CONF)
>> +def cmd_list_ports(cfg, bridge):
>> +    ports = []
>>      parent, vlan = find_bridge(cfg, bridge)
>>      for port in get_bridge_ports(cfg, parent, vlan):
>>          if port != bridge:
>> -            print port
>> +            ports += [port]
>> +    return ports
> Slight preference for ports.append(port)
> Alternatively, you could avoid it altogether with a list comprehension
> (e.g.: below) or a filter, but I think your current method is very clear.
> all_ports = get_bridge_ports(cfg, parent, vlan)  # Includes bridge
> return [port for port in all_ports if port != bridge]

Thanks.  I changed += to append.

I left the loop as-is.  I guess I'm not Pythonic enough yet--I
still have trouble understanding comprehensions that include more
than just a single "for"; the order of operations is not obvious
to me.

>> -def cmd_port_to_br(port):
>> -    cfg = cfg_read(VSWITCHD_CONF)
>> +def cmd_port_to_br(cfg, port):
>>      bridge = port_to_bridge(cfg, port)
>>      if bridge:
>> -        print bridge
>> +        return bridge,
> For all the functions that now return singletons, slight preference for
> return (something,)  or  return [something]  over  return something,
> Just for the visual clarity, less chance of missing it or mistaking it for
> a typo

You're right, that's an improvement.  Fixed.

>> -def cmd_list_ifaces(bridge):
>> -    cfg = cfg_read(VSWITCHD_CONF)
>> +def cmd_list_ifaces(cfg, bridge):
>> +    ifaces = []
>>      parent, vlan = find_bridge(cfg, bridge)
>>      for iface in get_bridge_ifaces(cfg, parent, vlan):
>>          if iface != bridge:
>> -            print iface
>> +            ifaces += [iface]
>> +    return ifaces
> Prefer append

Done.




More information about the dev mailing list