From 863b69c0584e2ee663cf75485172e094238ab7ac Mon Sep 17 00:00:00 2001 From: Shuotian Cheng Date: Thu, 1 Nov 2018 01:07:31 -0700 Subject: [PATCH] [teamsyncd]: Fix bug when removing selectable in select function (#665) In the current implementation, the selectable is removed inside the select() function. However, the select function will retrieve multiple file descriptors at one time. If the file descriptor is removed, it will cause segmentation fault. Signed-off-by: Shu0T1an ChenG --- teamsyncd/teamsync.cpp | 52 +++++++++++++++++++++++++++++++---------- teamsyncd/teamsync.h | 17 +++++++++----- teamsyncd/teamsyncd.cpp | 2 ++ 3 files changed, 53 insertions(+), 18 deletions(-) diff --git a/teamsyncd/teamsync.cpp b/teamsyncd/teamsync.cpp index 6e49971d5dab..5db7d3c45594 100644 --- a/teamsyncd/teamsync.cpp +++ b/teamsyncd/teamsync.cpp @@ -24,6 +24,26 @@ TeamSync::TeamSync(DBConnector *db, DBConnector *stateDb, Select *select) : { } +void TeamSync::doSelectableTask() +{ + /* Start to track the new team instances */ + for (auto s : m_selectablesToAdd) + { + m_select->addSelectable(m_teamSelectables[s].get()); + } + + m_selectablesToAdd.clear(); + + /* No longer track the deprecated team instances */ + for (auto s : m_selectablesToRemove) + { + m_select->removeSelectable(m_teamSelectables[s].get()); + m_teamSelectables.erase(s); + } + + m_selectablesToRemove.clear(); +} + void TeamSync::onMsg(int nlmsg_type, struct nl_object *obj) { struct rtnl_link *link = (struct rtnl_link *)obj; @@ -31,6 +51,7 @@ void TeamSync::onMsg(int nlmsg_type, struct nl_object *obj) return; string lagName = rtnl_link_get_name(link); + /* Listens to LAG messages */ char *type = rtnl_link_get_type(link); if (!type || (strcmp(type, TEAM_DRV_NAME) != 0)) @@ -63,35 +84,40 @@ void TeamSync::addLag(const string &lagName, int ifindex, bool admin_state, lagName.c_str(), admin_state ? "up" : "down", oper_state ? "up" : "down"); /* Return when the team instance has already been tracked */ - if (m_teamPorts.find(lagName) != m_teamPorts.end()) + if (m_teamSelectables.find(lagName) != m_teamSelectables.end()) return; - /* Start track the team instance */ - auto sync = make_shared(lagName, ifindex, &m_lagMemberTable); - m_select->addSelectable(sync.get()); - m_teamPorts[lagName] = sync; - fvVector.clear(); FieldValueTuple s("state", "ok"); fvVector.push_back(s); m_stateLagTable.set(lagName, fvVector); + + /* Create the team instance */ + auto sync = make_shared(lagName, ifindex, &m_lagMemberTable); + m_teamSelectables[lagName] = sync; + m_selectablesToAdd.insert(lagName); } void TeamSync::removeLag(const string &lagName) { + /* Delete all members */ + auto selectable = m_teamSelectables[lagName]; + for (auto it : selectable->m_lagMembers) + { + m_lagMemberTable.del(lagName + ":" + it.first); + } + /* Delete the LAG */ m_lagTable.del(lagName); SWSS_LOG_INFO("Remove %s", lagName.c_str()); /* Return when the team instance hasn't been tracked before */ - if (m_teamPorts.find(lagName) == m_teamPorts.end()) + if (m_teamSelectables.find(lagName) == m_teamSelectables.end()) return; - /* No longer track the current team instance */ - m_select->removeSelectable(m_teamPorts[lagName].get()); - m_teamPorts.erase(lagName); m_stateLagTable.del(lagName); + m_selectablesToRemove.insert(lagName); } const struct team_change_handler TeamSync::TeamPortSync::gPortChangeHandler = { @@ -114,7 +140,8 @@ TeamSync::TeamPortSync::TeamPortSync(const string &lagName, int ifindex, } int err = team_init(m_team, ifindex); - if (err) { + if (err) + { team_free(m_team); m_team = NULL; SWSS_LOG_ERROR("Unable to init team socket"); @@ -123,7 +150,8 @@ TeamSync::TeamPortSync::TeamPortSync(const string &lagName, int ifindex, } err = team_change_handler_register(m_team, &gPortChangeHandler, this); - if (err) { + if (err) + { team_free(m_team); m_team = NULL; SWSS_LOG_ERROR("Unable to register port change event"); diff --git a/teamsyncd/teamsync.h b/teamsyncd/teamsync.h index c53ad224e80a..83b42a836478 100644 --- a/teamsyncd/teamsync.h +++ b/teamsyncd/teamsync.h @@ -18,12 +18,12 @@ class TeamSync : public NetMsg public: TeamSync(DBConnector *db, DBConnector *stateDb, Select *select); - /* - * Listens to RTM_NEWLINK and RTM_DELLINK to undestand if there is a new - * team device - */ + /* Listen to RTM_NEWLINK, RTM_DELLINK to track team devices */ virtual void onMsg(int nlmsg_type, struct nl_object *obj); + /* Handle all selectables add/removal events */ + void doSelectableTask(); + class TeamPortSync : public Selectable { public: @@ -35,6 +35,8 @@ class TeamSync : public NetMsg int getFd() override; void readData() override; + /* member_name -> enabled|disabled */ + std::map m_lagMembers; protected: int onChange(); static int teamdHandler(struct team_handle *th, void *arg, @@ -45,7 +47,6 @@ class TeamSync : public NetMsg struct team_handle *m_team; std::string m_lagName; int m_ifindex; - std::map m_lagMembers; /* map[ifname] = status (enabled|disabled) */ }; protected: @@ -58,7 +59,11 @@ class TeamSync : public NetMsg ProducerStateTable m_lagTable; ProducerStateTable m_lagMemberTable; Table m_stateLagTable; - std::map > m_teamPorts; + + /* Store selectables needed to be updated in doSelectableTask function */ + std::set m_selectablesToAdd; + std::set m_selectablesToRemove; + std::map > m_teamSelectables; }; } diff --git a/teamsyncd/teamsyncd.cpp b/teamsyncd/teamsyncd.cpp index 2ac52ebf72d2..7e61c3efa407 100644 --- a/teamsyncd/teamsyncd.cpp +++ b/teamsyncd/teamsyncd.cpp @@ -35,6 +35,8 @@ int main(int argc, char **argv) { Selectable *temps; s.select(&temps); + + sync.doSelectableTask(); } } catch (const std::exception& e)