[ovs-dev] [PATCH] netdev: Disallow creation of netdevs with unsupported options.

Jesse Gross jesse at nicira.com
Wed Aug 3 10:14:32 UTC 2011


On Wed, Aug 3, 2011 at 1:17 PM, Ethan Jackson <ethan at nicira.com> wrote:
>>> This patch fixes the issue by disallowing the creation of netdevs
>>> with unsupported configuration options.
>>
>> Currently in netdev-vport.c:parse_tunnel_config() we just warn if
>> there is an unknown option.  I think if we simply returned an error at
>> that point it would have the same effect and we wouldn't need to
>> duplicate the logic for valid options.
>
> That would work in most cases.  It doesn't work in the case where a
> tunnel is created with proper configuration options, and then an
> invalid configuration option is added.  You run into the same problem
> because netdev_open() checks if the configuration is equal before
> attempting to open the netdev.

Why doesn't it work?  Whenever the configuration needs to set (either
for a new device or to change the config), parse needs to be called.
If it returns an error at that point, the operation will fail, as it
should, and the previous state will remain.  This is different from
the current situation where we allow an invalid config initially and
then later fail to handle the exact same config.

I think there's a more generic problem here that bridge.c simply
doesn't handle changes to the config when ovs-vswitchd isn't running.
Ports will get enumerated and netdev_open() will get called on them
with the new configuration.  However, this configuration normally gets
passed down to the kernel when the ports are added but this won't
happen since they already exist.  netdev_set_config() also doesn't get
called because the netdevs are being opened for the first time.  The
end result is that netdev_open() chokes because ofproto has already
opened the port with the kernel config.

Perhaps if we just reconfigured ports at ovs-vswitchd startup then we
could avoid this whole issue by not needing to do argument comparison
at all.

Regardless, if we do have to go down the original path, I don't think
we should have a second function to check the config.  We already have
one parse function, you can just call that sooner if necessary.

> I'd be curious what Ben's opinion is.  I think the real problem here
> may be with the structure of netdev_open().  Perhaps we should always
> be opening the new netdev and replacing the old one with it.  I'll
> have to think about it.

The problem with that is that you lost any state associated with the
port.  In the case of internal devices, you lose the IP addresses.
For virtual ports, you lose the stats.



More information about the dev mailing list