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

reid at nicira.com reid at nicira.com
Fri Oct 16 02:19:57 UTC 2009


Looks good, a few preference comments inline.
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).

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]

> -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

> -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






More information about the dev mailing list