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

[Ben Pfaff]

Justin Pettit <jpettit at nicira.com> writes:

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

Well, that's what I want that function to do.  I borrowed it
straight from interface-reconfigure.

But you're right that I don't want the whole thing to go up in
smoke just because we can't get an Ethernet address.  So I added
an exception handler to the caller, and now it returns
8000.002320ffffff if reading the file fails.  (That's more
Google-able than 8000.000000000000.)

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

We don't know whether it's there.

I rewrote it as:
  if linux_bridge in bridge_ports:
      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

I sorted them because I found the output easier to read that way.

I hope that the Citrix QA scripts are not sensitive to ordering.
If they are, then we are going to lose, because vswitch ordering
is going to be different from the bridge ordering.

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

OK, changed.

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

Ditto.

I'll follow up with the updated patch when I finish applying the
remaining comments.