[ovs-dev] [PATCH] xenserver: Add support for disabling in-band management via XAPI.
aevans at nicira.com
Mon Feb 28 20:52:11 UTC 2011
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.
> 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:
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.
More information about the dev