[ovs-discuss] [nic19-2 5/5] xenserver: Fix "brctl show" compatibility by introducing "brctl" wrapper.

[Justin Pettit]

Thanks for jumping in on the code reviews, Reid!  So, in addition to  
the things Reid mentioned, here are a few items that you may want to  
consider:

> +# Returns the first line of the file named 'name', with the  
> trailing new-line
> +# (if any) stripped off.
> +def read_first_line_of_file(name):
> +    file = None
> +    try:
> +        file = open(name, 'r')
> +        return file.readline().rstrip('\n')
> +    finally:
> +        if file != None:
> +            file.close()

This code will barf if the file can't be read, since the exception is  
still raised from the "finally" clause.  Even if the exception were  
caught, it would still return None, which will make callers like  
get_bridge_id() raise an exception.  What if y

def read_first_line_of_file(name):
	file = None
	line = ""
	try:
		file = open(name, 'r')
		line = file.readline().rstrip('\n')
	finally:
		if file:
			file.close()
	return line

> +def cmd_show():
> ...
> +        bridge_ports = [port for port in bridge_ports if port !=  
> linux_bridge]

If we know that linux_bridge is always included in bridge_ports, I  
think it would be clearer to write:

	bridge_ports.remove(linux_bridge)

> +        bridge_ports.sort()

Do you think Citrix's QA scripts are expecting ports to be printed in  
the same order as brctl?  On my XenServer, they don't appear to be  
sorted:

	xenbr0		0000.002219d76850	no		vif2.0
								eth0

> +        print >>sys.stderr, "%s (use --help for help)" % msg

While this is correct, the general style I have seen is:

	sys.stderr.write("%s (use --help for help)" % msg)

The cfg_read() function also prints to stderr in this way.

The rest of the patch set looks good to me.

--Justin