[ovs-dev] [PATCH] xenserver: Add support for disabling in-band management via XAPI.

[Andrew Evans]

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