From 81755fa502502324157b2ef592288e5782298049 Mon Sep 17 00:00:00 2001 From: Bruno Donassolo Date: Thu, 22 Apr 2021 18:47:15 +0200 Subject: [PATCH] Raise exception for invalid parameters in StarZone We've talked about using exceptions for invalid parameters in user' calls. Adjust for StarZone. Bonus: We can test it in UT with catch --- src/kernel/routing/StarZone.cpp | 39 ++++++++++++++++++---------- src/kernel/routing/StarZone_test.cpp | 37 ++++++++++++++++++-------- 2 files changed, 51 insertions(+), 25 deletions(-) diff --git a/src/kernel/routing/StarZone.cpp b/src/kernel/routing/StarZone.cpp index c311f34369..72fb1e4499 100644 --- a/src/kernel/routing/StarZone.cpp +++ b/src/kernel/routing/StarZone.cpp @@ -8,6 +8,7 @@ #include "simgrid/kernel/routing/RoutedZone.hpp" #include "src/surf/network_interface.hpp" #include "src/surf/xml/platf_private.hpp" // RouteCreationArgs and friends +#include "xbt/string.hpp" XBT_LOG_NEW_DEFAULT_SUBCATEGORY(surf_route_star, surf, "Routing part of surf"); @@ -92,25 +93,35 @@ void StarZone::check_add_route_param(const NetPoint* src, const NetPoint* dst, c const char* src_name = src ? src->get_cname() : "nullptr"; const char* dst_name = dst ? dst->get_cname() : "nullptr"; - xbt_assert((src || dst) && (not dst || not src || src == dst), - "Cannot add route from %s to %s. In a StarZone, route must be: i) from source host to everyone, ii) from " - "everyone to a single host or iii) loopback, same source and destination", - src_name, dst_name); - xbt_assert(not symmetrical || src, - "Cannot add route from %s to %s. In a StarZone, symmetrical routes must be set from source to everyone " - "(not the contrary).", - src_name, dst_name); + if ((not src && not dst) || (dst && src && src != dst)) + throw std::invalid_argument(xbt::string_printf( + "Cannot add route from %s to %s. In a StarZone, route must be: i) from source netpoint to everyone, ii) from " + "everyone to a single netpoint or iii) loopback, same source and destination", + src_name, dst_name)); + + if (symmetrical && not src) + throw std::invalid_argument(xbt::string_printf("Cannot add route from %s to %s. In a StarZone, symmetrical routes " + "must be set from source to everyone (not the contrary)", + src_name, dst_name)); if (src && src->is_netzone()) { - xbt_assert(gw_src, "add_route(): source %s is a netzone but gw_src isn't configured", src->get_cname()); - xbt_assert(not gw_src->is_netzone(), "add_route(): src(%s) is a netzone, gw_src(%s) cannot be a netzone", - src->get_cname(), gw_src->get_cname()); + if (not gw_src) + throw std::invalid_argument(xbt::string_printf( + "StarZone::add_route(): source %s is a netzone but gw_src isn't configured", src->get_cname())); + if (gw_src->is_netzone()) + 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())); } if (dst && dst->is_netzone()) { - xbt_assert(gw_dst, "add_route(): destination %s is a netzone but gw_dst isn't configured", dst->get_cname()); - xbt_assert(not gw_dst->is_netzone(), "add_route(): dst(%s) is a netzone, gw_dst(%s) cannot be a netzone", - dst->get_cname(), gw_dst->get_cname()); + if (not gw_dst) + throw std::invalid_argument(xbt::string_printf( + "StarZone::add_route(): destination %s is a netzone but gw_dst isn't configured", dst->get_cname())); + if (gw_dst->is_netzone()) + 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())); } } diff --git a/src/kernel/routing/StarZone_test.cpp b/src/kernel/routing/StarZone_test.cpp index 46126ba52f..849489c90f 100644 --- a/src/kernel/routing/StarZone_test.cpp +++ b/src/kernel/routing/StarZone_test.cpp @@ -32,23 +32,30 @@ TEST_CASE("kernel::routing::StarZone: Creating Zone", "[creation]") REQUIRE(simgrid::s4u::create_star_zone("test")); } -// One day we may be able to test contracts and asserts with catch2 -// https://github.com/catchorg/Catch2/issues/853 -TEST_CASE("kernel::routing::StarZone: Adding routes (hosts): assert", "[.][assert]") +TEST_CASE("kernel::routing::StarZone: Adding routes (hosts): exception", "") { EngineWrapper e("test"); auto* zone = new simgrid::kernel::routing::StarZone("test"); auto* netpoint1 = new simgrid::kernel::routing::NetPoint("netpoint1", simgrid::kernel::routing::NetPoint::Type::Host); auto* netpoint2 = new simgrid::kernel::routing::NetPoint("netpoint2", simgrid::kernel::routing::NetPoint::Type::Host); - SECTION("src and dst: nullptr") { zone->add_route(nullptr, nullptr, nullptr, nullptr, {}, false); } + SECTION("src and dst: nullptr") + { + REQUIRE_THROWS_AS(zone->add_route(nullptr, nullptr, nullptr, nullptr, {}, false), std::invalid_argument); + } - SECTION("src: nullptr and symmetrical: true") { zone->add_route(nullptr, netpoint2, nullptr, nullptr, {}, true); } + SECTION("src: nullptr and symmetrical: true") + { + REQUIRE_THROWS_AS(zone->add_route(nullptr, netpoint2, nullptr, nullptr, {}, true), std::invalid_argument); + } - SECTION("src and dst: not nullptr") { zone->add_route(netpoint1, netpoint2, nullptr, nullptr, {}, false); } + SECTION("src and dst: not nullptr") + { + REQUIRE_THROWS_AS(zone->add_route(netpoint1, netpoint2, nullptr, nullptr, {}, false), std::invalid_argument); + } } -TEST_CASE("kernel::routing::StarZone: Adding routes (netzones): assert", "[.][assert]") +TEST_CASE("kernel::routing::StarZone: Adding routes (netzones): exception", "") { EngineWrapper e("test"); auto* zone = new simgrid::kernel::routing::StarZone("test"); @@ -57,21 +64,29 @@ TEST_CASE("kernel::routing::StarZone: Adding routes (netzones): assert", "[.][as auto* netpoint2 = new simgrid::kernel::routing::NetPoint("netpoint2", simgrid::kernel::routing::NetPoint::Type::NetZone); - SECTION("src: is a netzone and gw_src: nullptr") { zone->add_route(netpoint1, nullptr, nullptr, nullptr, {}, false); } + SECTION("src: is a netzone and gw_src: nullptr") + { + REQUIRE_THROWS_AS(zone->add_route(netpoint1, nullptr, nullptr, nullptr, {}, false), std::invalid_argument); + } SECTION("src: is a netzone and gw_src: is a netzone") { - zone->add_route(netpoint1, nullptr, netpoint2, nullptr, {}, false); + REQUIRE_THROWS_AS(zone->add_route(netpoint1, nullptr, netpoint2, nullptr, {}, false), std::invalid_argument); } - SECTION("dst: is a netzone and gw_dst: nullptr") { zone->add_route(nullptr, netpoint2, nullptr, nullptr, {}, false); } + SECTION("dst: is a netzone and gw_dst: nullptr") + { + REQUIRE_THROWS_AS(zone->add_route(nullptr, netpoint2, nullptr, nullptr, {}, false), std::invalid_argument); + } SECTION("dst: is a netzone and gw_dst: is a netzone") { - zone->add_route(nullptr, netpoint2, nullptr, netpoint1, {}, false); + REQUIRE_THROWS_AS(zone->add_route(nullptr, netpoint2, nullptr, netpoint1, {}, false), std::invalid_argument); } } +// One day we may be able to test contracts and asserts with catch2 +// https://github.com/catchorg/Catch2/issues/853 TEST_CASE("kernel::routing::StarZone: Get routes: assert", "[.][assert]") { /* workaround to initialize things, they must be done in this particular order */ -- 2.20.1