Issue#71: add check in add_route for gw_src/gw_dst

When adding a NetZoneRoute, check if gw_src and gw_dst belongs to the
respective netzones.

We need to recursively search for the netpoint inside the netzone since
the netpoint can be from one of its children.

diff --git a/MANIFEST.in b/MANIFEST.in
index 3d7703f..56c61a9 100644 (file)
--- a/MANIFEST.in
+++ b/MANIFEST.in
@@ -729,6 +729,9 @@ include teshsuite/s4u/host-on-off/host-on-off.cpp
 include teshsuite/s4u/host-on-off/host-on-off.tesh
 include teshsuite/s4u/is-router/is-router.cpp
 include teshsuite/s4u/is-router/is-router.tesh
+include teshsuite/s4u/issue71/issue71.cpp
+include teshsuite/s4u/issue71/issue71.tesh
+include teshsuite/s4u/issue71/platform_bad.xml
 include teshsuite/s4u/listen_async/listen_async.cpp
 include teshsuite/s4u/listen_async/listen_async.tesh
 include teshsuite/s4u/ns3-from-src-to-itself/ns3-from-src-to-itself.cpp

diff --git a/include/simgrid/kernel/routing/NetZoneImpl.hpp b/include/simgrid/kernel/routing/NetZoneImpl.hpp
index 2485bed..257c8e7 100644 (file)
--- a/include/simgrid/kernel/routing/NetZoneImpl.hpp
+++ b/include/simgrid/kernel/routing/NetZoneImpl.hpp
@@ -107,6 +107,9 @@ protected:
                         /* OUT */ std::vector<resource::LinkImpl*>& links, double* latency,
                         std::unordered_set<NetZoneImpl*>& netzones);
 
+  /** @brief Get the NetZone that is represented by the netpoint */
+  const NetZoneImpl* get_netzone_recursive(const NetPoint* netpoint) const;
+
 public:
   enum class RoutingMode {
     base,     /**< Base case: use simple link lists for routing     */
@@ -161,6 +164,8 @@ public:
 
   /** @brief Seal your netzone once you're done adding content, and before routing stuff through it */
   void seal();
+  /** @brief Check if netpoint is a member of this NetZone or some of the childrens */
+  bool is_component_recursive(const NetPoint* netpoint) const;
   virtual int add_component(kernel::routing::NetPoint* elm); /* A host, a router or a netzone, whatever */
   virtual void add_route(kernel::routing::NetPoint* src, kernel::routing::NetPoint* dst,
                          kernel::routing::NetPoint* gw_src, kernel::routing::NetPoint* gw_dst,

diff --git a/include/simgrid/kernel/routing/StarZone.hpp b/include/simgrid/kernel/routing/StarZone.hpp
index 0c2f093..c8ad0be 100644 (file)
--- a/include/simgrid/kernel/routing/StarZone.hpp
+++ b/include/simgrid/kernel/routing/StarZone.hpp
@@ -89,7 +89,7 @@ private:
   void add_links_to_route(const std::vector<resource::LinkImpl*>& links, Route* route, double* latency,
                           std::unordered_set<resource::LinkImpl*>& added_links) const;
   /** @brief Auxiliary methods to check params received in add_route method */
-  void check_add_route_param(const NetPoint* src, const NetPoint* dst, const NetPoint* gw_src, const NetPoint* gw_dst,
+  void check_add_route_param(const NetPoint* src, const NetPoint* dst, NetPoint* gw_src, NetPoint* gw_dst,
                              bool symmetrical) const;
   std::unordered_map<unsigned int, StarRoute> routes_;
 };

diff --git a/src/kernel/routing/NetZoneImpl.cpp b/src/kernel/routing/NetZoneImpl.cpp
index ef69fde..5706ce1 100644 (file)
--- a/src/kernel/routing/NetZoneImpl.cpp
+++ b/src/kernel/routing/NetZoneImpl.cpp
@@ -539,6 +539,36 @@ void NetZoneImpl::set_host_model(std::shared_ptr<surf::HostModel> host_model)
   host_model_ = std::move(host_model);
 }
 
+const NetZoneImpl* NetZoneImpl::get_netzone_recursive(const NetPoint* netpoint) const
+{
+  xbt_assert(netpoint && netpoint->is_netzone(), "Netpoint %s must be of the type NetZone",
+             netpoint ? netpoint->get_cname() : "nullptr");
+
+  if (netpoint == netpoint_)
+    return this;
+
+  for (auto* children : children_) {
+    const NetZoneImpl* netzone = children->get_netzone_recursive(netpoint);
+    if (netzone)
+      return netzone;
+  }
+  return nullptr;
+}
+
+bool NetZoneImpl::is_component_recursive(const NetPoint* netpoint) const
+{
+  /* check direct components */
+  for (const auto* elem : vertices_) {
+    if (elem == netpoint)
+      return true;
+  }
+  /* check childrens */
+  for (const auto* children : children_) {
+    if (children->is_component_recursive(netpoint))
+      return true;
+  }
+  return false;
+}
 } // namespace routing
 } // namespace kernel
 } // namespace simgrid

diff --git a/src/kernel/routing/RoutedZone.cpp b/src/kernel/routing/RoutedZone.cpp
index 8be4dfe..cc10d43 100644 (file)
--- a/src/kernel/routing/RoutedZone.cpp
+++ b/src/kernel/routing/RoutedZone.cpp
@@ -184,6 +184,17 @@ void RoutedZone::add_route_check_params(NetPoint* src, NetPoint* dst, NetPoint*
                gw_dst->get_cname(), dstName);
     xbt_assert(not link_list.empty(), "Empty route (between %s@%s and %s@%s) forbidden.", srcName, gw_src->get_cname(),
                dstName, gw_dst->get_cname());
+    const auto* netzone_src = get_netzone_recursive(src);
+    xbt_assert(netzone_src->is_component_recursive(gw_src),
+               "Invalid NetzoneRoute from %s@%s to %s@%s: gw_src %s belongs to %s, not to %s.", srcName,
+               gw_src->get_cname(), dstName, gw_dst->get_cname(), gw_src->get_cname(),
+               gw_src->get_englobing_zone()->get_cname(), srcName);
+
+    const auto* netzone_dst = get_netzone_recursive(dst);
+    xbt_assert(netzone_dst->is_component_recursive(gw_dst),
+               "Invalid NetzoneRoute from %s@%s to %s@%s: gw_dst %s belongs to %s, not to %s.", srcName,
+               gw_src->get_cname(), dstName, gw_dst->get_cname(), gw_dst->get_cname(),
+               gw_dst->get_englobing_zone()->get_cname(), dst->get_cname());
     s4u::NetZone::on_route_creation(symmetrical, gw_src, gw_dst, gw_src, gw_dst, link_list);
   }
 }

diff --git a/src/kernel/routing/StarZone.cpp b/src/kernel/routing/StarZone.cpp
index 724c711..3afd979 100644 (file)
--- a/src/kernel/routing/StarZone.cpp
+++ b/src/kernel/routing/StarZone.cpp
@@ -86,8 +86,8 @@ void StarZone::get_graph(const s_xbt_graph_t* graph, std::map<std::string, xbt_n
   }
 }
 
-void StarZone::check_add_route_param(const NetPoint* src, const NetPoint* dst, const NetPoint* gw_src,
-                                     const NetPoint* gw_dst, bool symmetrical) const
+void StarZone::check_add_route_param(const NetPoint* src, const NetPoint* dst, NetPoint* gw_src, NetPoint* gw_dst,
+                                     bool symmetrical) const
 {
   const char* src_name = src ? src->get_cname() : "nullptr";
   const char* dst_name = dst ? dst->get_cname() : "nullptr";
@@ -111,6 +111,12 @@ void StarZone::check_add_route_param(const NetPoint* src, const NetPoint* dst, c
       throw std::invalid_argument(
           xbt::string_printf("StarZone::add_route(): src(%s) is a netzone, gw_src(%s) cannot be a netzone",
                              src->get_cname(), gw_src->get_cname()));
+
+    const auto* netzone_src = get_netzone_recursive(src);
+    if (not netzone_src->is_component_recursive(gw_src))
+      throw std::invalid_argument(xbt::string_printf(
+          "Invalid NetzoneRoute from %s@%s to %s: gw_src %s belongs to %s, not to %s.", src_name, gw_src->get_cname(),
+          dst_name, gw_src->get_cname(), gw_src->get_englobing_zone()->get_cname(), src_name));
   }
 
   if (dst && dst->is_netzone()) {
@@ -121,6 +127,12 @@ void StarZone::check_add_route_param(const NetPoint* src, const NetPoint* dst, c
       throw std::invalid_argument(
           xbt::string_printf("StarZone::add_route(): dst(%s) is a netzone, gw_dst(%s) cannot be a netzone",
                              dst->get_cname(), gw_dst->get_cname()));
+
+    const auto* netzone_dst = get_netzone_recursive(dst);
+    if (not netzone_dst->is_component_recursive(gw_dst))
+      throw std::invalid_argument(xbt::string_printf(
+          "Invalid NetzoneRoute from %s@%s to %s: gw_dst %s belongs to %s, not to %s.", dst_name, gw_dst->get_cname(),
+          src_name, gw_dst->get_cname(), gw_dst->get_englobing_zone()->get_cname(), dst_name));
   }
 }
 

diff --git a/src/kernel/routing/StarZone_test.cpp b/src/kernel/routing/StarZone_test.cpp
index 41c481f..d3967a9 100644 (file)
--- a/src/kernel/routing/StarZone_test.cpp
+++ b/src/kernel/routing/StarZone_test.cpp
@@ -50,6 +50,8 @@ TEST_CASE("kernel::routing::StarZone: Adding routes (netzones): exception", "")
       new simgrid::kernel::routing::NetPoint("netpoint1", simgrid::kernel::routing::NetPoint::Type::NetZone);
   auto* netpoint2 =
       new simgrid::kernel::routing::NetPoint("netpoint2", simgrid::kernel::routing::NetPoint::Type::NetZone);
+  auto* zone3     = new simgrid::kernel::routing::StarZone("test3");
+  auto* netpoint3 = zone3->create_router("netpoint3");
 
   SECTION("src: is a netzone and gw_src: nullptr")
   {
@@ -70,6 +72,18 @@ TEST_CASE("kernel::routing::StarZone: Adding routes (netzones): exception", "")
   {
     REQUIRE_THROWS_AS(zone->add_route(nullptr, netpoint2, nullptr, netpoint1, {}, false), std::invalid_argument);
   }
+
+  SECTION("issue71: gw_src isn't member of the src netzone")
+  {
+    REQUIRE_THROWS_AS(zone->add_route(zone->get_netpoint(), nullptr, netpoint3, nullptr, {}, false),
+                      std::invalid_argument);
+  }
+
+  SECTION("issue71: gw_dst isn't member of the dst netzone")
+  {
+    REQUIRE_THROWS_AS(zone->add_route(nullptr, zone->get_netpoint(), nullptr, netpoint3, {}, false),
+                      std::invalid_argument);
+  }
 }
 
 // One day we may be able to test contracts and asserts with catch2
@@ -219,14 +233,13 @@ TEST_CASE("kernel::routing::StarZone: Get routes (netzones)", "")
   simgrid::s4u::Engine e("test");
   auto* zone = new simgrid::kernel::routing::StarZone("test");
 
-  auto* subzone1 =
-      (new simgrid::kernel::routing::NetPoint("subzone1", simgrid::kernel::routing::NetPoint::Type::NetZone))
-          ->set_englobing_zone(zone);
-  auto* subzone2 =
-      (new simgrid::kernel::routing::NetPoint("subzone2", simgrid::kernel::routing::NetPoint::Type::NetZone))
-          ->set_englobing_zone(zone);
-  auto* router1 = new simgrid::kernel::routing::NetPoint("router1", simgrid::kernel::routing::NetPoint::Type::Router);
-  auto* router2 = new simgrid::kernel::routing::NetPoint("router2", simgrid::kernel::routing::NetPoint::Type::Router);
+  auto* subzone1 = new simgrid::kernel::routing::StarZone("subzone1");
+  subzone1->set_parent(zone);
+  auto* subzone2 = new simgrid::kernel::routing::StarZone("subzone2");
+  subzone2->set_parent(zone);
+
+  auto* router1 = subzone1->create_router("router1");
+  auto* router2 = subzone2->create_router("router2");
 
   SECTION("Get route: netzone")
   {
@@ -234,13 +247,13 @@ TEST_CASE("kernel::routing::StarZone: Get routes (netzones)", "")
     links.push_back(zone->create_link("link1", {100})->set_latency(10)->get_impl());
     std::vector<simgrid::kernel::resource::LinkImpl*> links2;
     links2.push_back(zone->create_link("link2", {200})->set_latency(20)->get_impl());
-    zone->add_route(subzone1, nullptr, router1, nullptr, links, true);
-    zone->add_route(subzone2, nullptr, router2, nullptr, links2, true);
+    zone->add_route(subzone1->get_netpoint(), nullptr, router1, nullptr, links, true);
+    zone->add_route(subzone2->get_netpoint(), nullptr, router2, nullptr, links2, true);
     zone->seal();
 
     double lat = 0.0;
     simgrid::kernel::routing::Route route;
-    zone->get_local_route(subzone1, subzone2, &route, &lat);
+    zone->get_local_route(subzone1->get_netpoint(), subzone2->get_netpoint(), &route, &lat);
     REQUIRE(lat == 30);
     REQUIRE(route.gw_src_ == router1);
     REQUIRE(route.gw_dst_ == router2);

diff --git a/teshsuite/s4u/CMakeLists.txt b/teshsuite/s4u/CMakeLists.txt
index 86416f7..43f0ca1 100644 (file)
--- a/teshsuite/s4u/CMakeLists.txt
+++ b/teshsuite/s4u/CMakeLists.txt
@@ -8,7 +8,7 @@ foreach(x actor actor-autorestart actor-suspend
         storage_client_server listen_async pid
         trace-integration
         seal-platform
-       vm-live-migration vm-suicide)
+       vm-live-migration vm-suicide issue71)
   add_executable       (${x}  EXCLUDE_FROM_ALL ${x}/${x}.cpp)
   target_link_libraries(${x}  simgrid)
   set_target_properties(${x}  PROPERTIES RUNTIME_OUTPUT_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/${x})
@@ -35,7 +35,7 @@ foreach(x actor actor-autorestart actor-suspend
 endforeach()
 
 foreach(x basic-link-test basic-parsing-test host-on-off host-on-off-actors host-on-off-recv is-router listen_async 
-               pid storage_client_server trace-integration seal-platform)
+               pid storage_client_server trace-integration seal-platform issue71)
   set(tesh_files    ${tesh_files}    ${CMAKE_CURRENT_SOURCE_DIR}/${x}/${x}.tesh)
   ADD_TESH(tesh-s4u-${x} --setenv srcdir=${CMAKE_HOME_DIRECTORY}/teshsuite/s4u/${x} --setenv platfdir=${CMAKE_HOME_DIRECTORY}/examples/platforms --cd ${CMAKE_BINARY_DIR}/teshsuite/s4u/${x} ${CMAKE_HOME_DIRECTORY}/teshsuite/s4u/${x}/${x}.tesh)
 endforeach()
@@ -95,4 +95,5 @@ set(xml_files     ${xml_files}      ${CMAKE_CURRENT_SOURCE_DIR}/activity-lifecyc
                                     ${CMAKE_CURRENT_SOURCE_DIR}/trace-integration/test-hbp1-c1s1-c3s2.xml
                                     ${CMAKE_CURRENT_SOURCE_DIR}/trace-integration/test-hbp2.5-hbp1.5.xml
                                     ${CMAKE_CURRENT_SOURCE_DIR}/vm-live-migration/platform.xml
+                                    ${CMAKE_CURRENT_SOURCE_DIR}/issue71/platform_bad.xml
                                    PARENT_SCOPE)

diff --git a/teshsuite/s4u/issue71/issue71.cpp b/teshsuite/s4u/issue71/issue71.cpp
new file mode 100644 (file)
index 0000000..cc1a018
--- /dev/null
+++ b/teshsuite/s4u/issue71/issue71.cpp
@@ -0,0 +1,37 @@
+#include <simgrid/s4u.hpp>
+#include <vector>
+
+using namespace std;
+using namespace simgrid;
+
+static void runner()
+{
+  auto e                    = s4u::Engine::get_instance();
+  simgrid::s4u::Host* host0 = e->host_by_name("c1_0");
+  simgrid::s4u::Host* host1 = e->host_by_name("c2_0");
+
+  std::vector<double> comp = {1e6, 1e6};
+  std::vector<double> comm = {1, 2, 3, 4};
+
+  vector<simgrid::s4u::Host*> h1 = {host0, host1};
+  simgrid::s4u::this_actor::parallel_execute(h1, comp, comm);
+}
+
+int main(int argc, char* argv[])
+{
+  s4u::Engine e(&argc, argv);
+  e.set_config("host/model:ptask_L07");
+
+  xbt_assert(argc == 2,
+             "\nUsage: %s platform_ok.xml\n"
+             "\tor: %s platform_bad.xml\n",
+             argv[0], argv[0]);
+
+  const char* platform_file = argv[1];
+  e.load_platform(platform_file);
+
+  simgrid::s4u::Actor::create("actor", e.host_by_name("c1_0"), runner);
+
+  e.run();
+  return 0;
+}

diff --git a/teshsuite/s4u/issue71/issue71.tesh b/teshsuite/s4u/issue71/issue71.tesh
new file mode 100644 (file)
index 0000000..d1b9f26
--- /dev/null
+++ b/teshsuite/s4u/issue71/issue71.tesh
@@ -0,0 +1,5 @@
+! expect signal SIGIOT
+$ ${bindir:=.}/issue71 ${srcdir:=.}/platform_bad.xml "--log=root.fmt:[%10.6r]%e(%i:%a@%h)%e%m%n" --log=no_loc
+> [  0.000000] (0:maestro@) Configuration change: Set 'host/model' to 'ptask_L07'
+> [  0.000000] (0:maestro@) Switching to the L07 model to handle parallel tasks.
+> [  0.000000] (0:maestro@) Invalid NetzoneRoute from cluster1@router_c2 to cluster2@router_c1: gw_src router_c2 belongs to cluster2, not to cluster1.

diff --git a/teshsuite/s4u/issue71/platform_bad.xml b/teshsuite/s4u/issue71/platform_bad.xml
new file mode 100644 (file)
index 0000000..72abe0b
--- /dev/null
+++ b/teshsuite/s4u/issue71/platform_bad.xml
@@ -0,0 +1,20 @@
+<?xml version='1.0'?>
+<!DOCTYPE platform SYSTEM "https://simgrid.org/simgrid.dtd">
+<platform version="4.1">
+  <zone id="world" routing="Full">
+    <cluster id="cluster1" router_id="router_c1"
+       prefix="c1_" radical="0-0" suffix=""
+       speed="1Gf" bw="125MBps" lat="50us"  bb_bw="2.25GBps" bb_lat="500us">
+    </cluster>
+
+    <cluster id="cluster2" router_id="router_c2"
+      prefix="c2_" radical="0-0" suffix=""
+      speed="1Gf" bw="125MBps" lat="50us"  bb_bw="2.25GBps" bb_lat="500us">
+    </cluster>
+
+    <link id="backbone" bandwidth="1.25Gbps" latency="50us"/>
+    <zoneRoute src="cluster1" dst="cluster2" gw_src="router_c2" gw_dst="router_c1"> <!-- !!! -->
+      <link_ctn id="backbone"/>
+    </zoneRoute>
+  </zone>
+</platform>