Thanks for writing this up. Looks pretty good, couple of minor comments below.
It's really unfortunate that we have to reproduce the exact same logic in three places to do anything in this code base. At some point in the future (hopefully) I will have time to refactor this stuff cause it's a pain to maintain. That said, the patch pretty much looks like it works. A couple of general comments (and one specific one). Instead of allowing vswitch-disable-in-band to take on the values "true" or "false" I would prefer we allow them to take on the values "0" or "1" (n > 0?). This is more consistent with other xenserver other-config settings such as use_carrier, and a couple of other random ones I found here: http://docs.vmd.citrix.com/XenServer/5.0.0/1.0/en_gb/guest.html If the setting is explicitly set to false I think it would be good to explicitly set the vswitch setting to false instead of simply removing it. If the setting is unset then we should remove it, this gives us a bit more flexibility to change the default behavior of the vswitch when the setting is unset in the future. Also I think this will be a bit closer to what users will expect to happen. > + ovs_dib = vswitchCfgQuery(['get', 'Bridge', bridge, > 'other_config:disable-in-band']).strip('"') > + > + if xapi_dib != ovs_dib: > + if xapi_dib: > + vswitchCfgMod(['--', 'set', 'Bridge', bridge, > 'other_config:disable-in-band=' + xapi_dib]) > + else: > + vswitchCfgMod(['--', 'remove', 'Bridge', bridge, > 'other_config', 'disable-in-band' + xapi_dib]) You dont' need to append xapi_dib in the remove case here. Ethan Jackson _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev_openvswitch.org
