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

Ethan Jackson ethan at nicira.com
Mon Feb 28 19:24:46 UTC 2011


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




More information about the dev mailing list