On 2/28/11 11:24 AM, Ethan Jackson wrote: > It's really unfortunate that we have to reproduce the exact same logic > in three places to do anything in this code base.
Agreed. > That said, the patch pretty much looks like it works. Yes, I did test it. :) > 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 Actually, the only occurrance of "other_config" I see there is this: xe vif-param-set uuid=vif_uuid other_config:ethtool-tx=off My first inclination was to keep the values consistent between XAPI and OVS, but I'll change it to use 0/1 since you say that's the precedent that's been set. > 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. Ok. I did that originally but it made the code more complicated, so I boiled it down to set-to-true-or-remove. But I agree that it'd be better for this shim to not try to know what OVS will do with any given value, so I'll change it. >> + vswitchCfgMod(['--', 'remove', 'Bridge', bridge, >> 'other_config', 'disable-in-band' + xapi_dib]) > > You dont' need to append xapi_dib in the remove case here. Whoops, that's a remnant of a previous iteration where it was setting to "false". Good catch, but it's about to change anyway. :) I'll follow up with another patch. -Andrew _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev_openvswitch.org
