On Fri, Sep 23, 2016 at 09:17:13AM +0200, Jan Tluka wrote:
Thu, Sep 22, 2016 at 10:25:24PM CEST, olichtne(a)redhat.com wrote:
>From: Ondrej Lichtner <olichtne(a)redhat.com>
>
>Sometimes when configuration of the interface failed during it's
>creation (and the device wasn't created, e.g. missing team{d, ctl}). The
>slave would register this as a "configured" Device (because it had a
>{Net, Nm}ConfigDevice object created, and would attempt to deconfigure
>it, this return an exception again and either crash the slave or leave
>it in an inconsistent state (trying to deconfigure the device again and
>again).
>
>This makes sure that deconfiguration is skipped in case LNST didn't
>receive a netlink notification that the device was created and
>registered by the kernel.
>
>Signed-off-by: Ondrej Lichtner <olichtne(a)redhat.com>
>---
> lnst/Slave/InterfaceManager.py | 13 +++++++------
> lnst/Slave/NetTestSlave.py | 5 +++--
> 2 files changed, 10 insertions(+), 8 deletions(-)
>
>diff --git a/lnst/Slave/InterfaceManager.py b/lnst/Slave/InterfaceManager.py
>index 949817c..dc390a9 100644
>--- a/lnst/Slave/InterfaceManager.py
>+++ b/lnst/Slave/InterfaceManager.py
>@@ -566,12 +566,13 @@ class Device(object):
> m_dev.clear_configuration()
>
> if self._conf != None:
>- self.address_cleanup()
>- self.down()
>- self.deconfigure()
>- self._clear_tc_qdisc()
>- self._clear_tc_filters()
>- self.destroy()
>+ if self._if_index is not None:
>+ self.address_cleanup()
>+ self.down()
>+ self.deconfigure()
>+ self._clear_tc_qdisc()
>+ self._clear_tc_filters()
>+ self.destroy()
> self._conf = None
> self._conf_dict = None
>
>diff --git a/lnst/Slave/NetTestSlave.py b/lnst/Slave/NetTestSlave.py
>index f6976db..2d0b361 100644
>--- a/lnst/Slave/NetTestSlave.py
>+++ b/lnst/Slave/NetTestSlave.py
>@@ -383,7 +383,7 @@ class SlaveMethods:
>
> def deconfigure_interface(self, if_id):
> device = self._if_manager.get_mapped_device(if_id)
>- if device is not None:
>+ if device is not None and device.get_if_index() is not None:
^^^^ Is this check necessary? You
already do the check in the hunk above.
You're right, it's not necessary, and I also just noticed that the
Device class has an attribute _configured that is set to True if
configuration succeeds so it can probably be used more reliably than
waiting for a netlink notification (wrt. race conditions).
The _configured flag is already used by the deconfigure() method, the
problems were caused by the other methods, so I'll send a v2 of the
patch that extends the use of the flag to all the other methods.
Thanks for catching this.
-Ondrej