Merge pull request #171 from polycube-network/pr/fix_align_ifaces

fix align interfaces logic

Author: frisso

src/polycubed/src/port.cpp

@@ -134,18 +134,22 @@ void Port::set_peer(const std::string &peer) {
     if (peer_ == peer) {
       return;
     }
+
+    // if previous peer was an iface, remove subscriptions
+    if (auto extiface_old = dynamic_cast<ExtIface*>(peer_port_)) {
+      // Subscribe to the list of events
+      for (auto &it : subscription_list)
+        extiface_old->unsubscribe_parameter(uuid().str(), it.first);
+    }
   }
+
   ServiceController::set_port_peer(*this, peer);
 
-  {
-    std::lock_guard<std::mutex> guard(port_mutex_);
-    peer_ = peer;
-  }
+  std::lock_guard<std::mutex> guard(port_mutex_);
+  peer_ = peer;
 
   // Check if peer is a ExtIface
-  ExtIface* extiface = dynamic_cast<ExtIface*>(get_peer_iface());
-  if (extiface) {
-    std::lock_guard<std::mutex> lock(subscription_list_mutex);
+  if (auto extiface = dynamic_cast<ExtIface*>(peer_port_)) {
     // Subscribe to the list of events
     for (auto &it : subscription_list)
       extiface->subscribe_parameter(uuid().str(), it.first, it.second);
@@ -308,33 +312,34 @@ void Port::unconnect(PeerIface &p1, PeerIface &p2) {
 
 void Port::subscribe_peer_parameter(const std::string &param_name,
                                     ParameterEventCallback &callback) {
-  std::lock_guard<std::mutex> lock(subscription_list_mutex);
+  std::lock_guard<std::mutex> lock(port_mutex_);
   // Add event to the list
   subscription_list.emplace(param_name, callback);
 
   // Check if the port already has a peer (of type ExtIface)
   // Subscribe to the peer parameter only if the peer is an netdev
   // (we are not interested in align different cubes' ports).
-  ExtIface* extiface = dynamic_cast<ExtIface*>(get_peer_iface());
+  ExtIface* extiface = dynamic_cast<ExtIface*>(peer_port_);
   if (extiface)
     extiface->subscribe_parameter(uuid().str(), param_name, callback);
 }
 
 void Port::unsubscribe_peer_parameter(const std::string &param_name) {
-  std::lock_guard<std::mutex> lock(subscription_list_mutex);
+  std::lock_guard<std::mutex> lock(port_mutex_);
   // Remove event from the list
   subscription_list.erase(param_name);
 
   // Only unsubscribe if peer is ExtIface
-  ExtIface* extiface = dynamic_cast<ExtIface*>(get_peer_iface());
+  ExtIface* extiface = dynamic_cast<ExtIface*>(peer_port_);
   if (extiface)
     extiface->unsubscribe_parameter(uuid().str(), param_name);
 }
 
 void Port::set_peer_parameter(const std::string &param_name,
                               const std::string &value) {
+  std::lock_guard<std::mutex> lock(port_mutex_);
   // Check if peer is a ExtIface
-  ExtIface* extiface = dynamic_cast<ExtIface*>(get_peer_iface());
+  ExtIface* extiface = dynamic_cast<ExtIface*>(peer_port_);
   if (extiface)
     extiface->set_parameter(param_name, value);
 }

src/polycubed/src/port.h

@@ -122,7 +122,6 @@ class Port : public polycube::service::PortIface, public PeerIface {
   uint16_t calculate_index() const;
 
   std::unordered_map<std::string, ParameterEventCallback> subscription_list;
-  std::mutex subscription_list_mutex;
 };
 
 }  // namespace polycubed

src/polycubed/src/utils/netlink.h

@@ -110,8 +110,10 @@ class Netlink {
 
   void notify(const Event &event, int ifindex, const std::string &ifname) {
     std::map<int, std::function<void(int, const std::string &)>> ob_copy;
-    std::lock_guard<std::mutex> lock(notify_mutex);
-    ob_copy.insert(observers_[event].begin(), observers_[event].end());
+    {
+      std::lock_guard<std::mutex> lock(notify_mutex);
+      ob_copy.insert(observers_[event].begin(), observers_[event].end());
+    }
 
     for (const auto &it : ob_copy) {
       auto obs = it.second;

src/services/pcn-router/src/Ports.cpp

@@ -92,72 +92,64 @@ Ports::Ports(polycube::service::Cube<Ports> &parent,
 
   // lambda function to align IP and Netmask between Port and ExtIface
   ParameterEventCallback f_ip;
-  f_ip = [&](const std::string name_iface, const std::string cidr) {
-    if (peer() == name_iface) {
-      try {
-        if (cidr.empty()) {
-          // set the ip address of the port on the netdev
-          std::string ip = getIp();
-          int prefix = utils::get_netmask_length(getNetmask());
-          parent_.netlink_instance_router_.add_iface_ip(peer(), ip, prefix);
-        } else {
-          // set the ip address of the netdev on the port
-          // cidr = ip_address/prefix
-          std::istringstream split(cidr);
-          std::vector<std::string> info;
-
-          char split_char = '/';
-          for (std::string each; std::getline(split, each, split_char);
-               info.push_back(each));
-          std::string new_ip = info[0];
-          std::string new_netmask =
-              utils::get_netmask_from_CIDR(std::stoi(info[1]));
-
-          std::string old_ip = getIp();
-          std::string old_netmask = getNetmask();
-
-          logger()->debug(
-              "Align ip and netmask of port {0} with those of interface {1}",
-              name(), name_iface);
-
-          if (old_ip != new_ip)
-            setIp_Netlink(new_ip);
-          if (old_netmask != new_netmask)
-            setNetmask_Netlink(new_netmask);
-        }
-      } catch (std::exception &e) {
-        logger()->trace("iface_ip_notification - False ip notification: {0}",
-                        e.what());
+  f_ip = [&](const std::string param_name, const std::string cidr) {
+    try {
+      if (!cidr.empty()) {
+        // set the ip address of the netdev on the port
+        // cidr = ip_address/prefix
+        std::istringstream split(cidr);
+        std::vector<std::string> info;
+
+        char split_char = '/';
+        for (std::string each; std::getline(split, each, split_char);
+             info.push_back(each))
+          ;
+        std::string new_ip = info[0];
+        std::string new_netmask =
+            utils::get_netmask_from_CIDR(std::stoi(info[1]));
+
+        std::string old_ip = getIp();
+        std::string old_netmask = getNetmask();
+
+        logger()->debug("Align ip and netmask of port {0}", name());
+
+        if (old_ip != new_ip)
+          setIp_Netlink(new_ip);
+        if (old_netmask != new_netmask)
+          setNetmask_Netlink(new_netmask);
       }
+    } catch (std::exception &e) {
+      logger()->trace("iface_ip_notification - False ip notification: {0}",
+                      e.what());
     }
   };
 
   // lambda function to align MAC between Port and ExtIface
   ParameterEventCallback f_mac;
-  f_mac = [&](const std::string name_iface, const std::string mac) {
-    if (peer() == name_iface) {
-      try {
-        std::string old_mac = getMac();
-        if (old_mac != mac) {
-          logger()->debug("Align mac of port {0} with those of interface {1}",
-                          name(), name_iface);
-          doSetMac(mac);
-        }
-      } catch (std::exception &e) {
-        logger()->trace("iface_mac_notification - False mac notification: {0}",
-                        e.what());
+  f_mac = [&](const std::string param_name, const std::string mac) {
+    try {
+      if (mac_ != mac) {
+        logger()->debug("Align mac of port {0}", name());
+        doSetMac(mac);
       }
+    } catch (std::exception &e) {
+      logger()->trace("iface_mac_notification - False mac notification: {0}",
+                      e.what());
     }
   };
 
-  // Register the new port to IP and MAC notifications arriving from ExtIface
-  subscribe_peer_parameter("IP", f_ip);
-  subscribe_peer_parameter("MAC", f_mac);
+  if (!parent_.get_shadow()) {
+    // Register the new port to IP and MAC notifications arriving from ExtIface
+    subscribe_peer_parameter("IP", f_ip);
+    subscribe_peer_parameter("MAC", f_mac);
+  }
 }
 
 Ports::~Ports() {
-  unsubscribe_peer_parameter("IP");
-  unsubscribe_peer_parameter("MAC");
+  if (!parent_.get_shadow()) {
+    unsubscribe_peer_parameter("IP");
+    unsubscribe_peer_parameter("MAC");
+  }
 }
 
 void Ports::update(const PortsJsonObject &conf) {
@@ -355,26 +347,24 @@ void Ports::setMac(const std::string &value) {
   set_peer_parameter("MAC", value);
 }
 
-void Ports::doSetMac(const std::string &value) {
-  if (mac_ != value) {
-    std::string new_mac = value;
-    /* Update the port in the datapath */
-    uint16_t index = this->index();
-    auto router_port = parent_.get_hash_table<uint16_t, r_port>("router_port");
+void Ports::doSetMac(const std::string &new_mac) {
+  if (mac_ == new_mac) {
+    return;
+  }
 
-    try {
-      r_port value = router_port.get(index);
-      value.mac = utils::mac_string_to_be_uint(new_mac);
-    } catch (...) {
-      logger()->error("Port {0} not found in the data path", this->name());
-    }
+  /* Update the port in the datapath */
+  uint16_t index = this->index();
+  auto router_port = parent_.get_hash_table<uint16_t, r_port>("router_port");
 
-    logger()->debug(
-        "Updated mac port: {0} (index: {4}) [mac: {1} - ip: {2} - netmask: {3}]",
-        getName(), new_mac, getIp(), getNetmask(), index);
+  r_port map_value = router_port.get(index);
+  map_value.mac = utils::mac_string_to_be_uint(new_mac);
+  router_port.set(index, map_value);
 
-    mac_ = new_mac;
-  }
+  logger()->debug(
+      "Updated mac port: {0} (index: {4}) [mac: {1} - ip: {2} - netmask: {3}]",
+      getName(), new_mac, getIp(), getNetmask(), index);
+
+  mac_ = new_mac;
 }
 
 std::shared_ptr<spdlog::logger> Ports::logger() {