shill: static ip config should not change default_route when no route specified
Fixes a bug introduced in crrev/c/2681397 that when original ip_property
contains both routes and default_route, default_route will be set to
false upon setting StaticIPConfig even if no routes are specified there.
BUG=b:183736713
TEST=unit
Change-Id: I8471efcd58b470e71c1fa4c156b672b1ff725e34
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2805109
Tested-by: Taoyu Li <taoyl@chromium.org>
Reviewed-by: Hugo Benichi <hugobenichi@google.com>
Commit-Queue: Hugo Benichi <hugobenichi@google.com>
(cherry picked from commit a36426f663820f09c69d220519d992ba390a7cbb)
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2814185
Commit-Queue: Taoyu Li <taoyl@chromium.org>
diff --git a/shill/static_ip_parameters.cc b/shill/static_ip_parameters.cc
index 409cadc..940660d 100644
--- a/shill/static_ip_parameters.cc
+++ b/shill/static_ip_parameters.cc
@@ -194,30 +194,34 @@
}
}
-void StaticIPParameters::ApplyRoutes(const string& property,
- const string& gateway,
- vector<IPConfig::Route>* value_out) {
+void StaticIPParameters::ApplyRoutes(IPConfig::Properties* props) {
vector<string> saved_routes;
- for (const auto& route : *value_out) {
+ for (const auto& route : props->routes) {
saved_routes.push_back(route.host + "/" +
base::NumberToString(route.prefix));
}
- saved_args_.Set<Strings>(property, saved_routes);
+ saved_args_.Set<Strings>(kIncludedRoutesProperty, saved_routes);
- if (!args_.Contains<Strings>(property)) {
+ if (!args_.Contains<Strings>(kIncludedRoutesProperty)) {
return;
}
- value_out->clear();
- ParseRoutes(args_.Get<Strings>(property), gateway, value_out);
+ props->routes.clear();
+ ParseRoutes(args_.Get<Strings>(kIncludedRoutesProperty), props->gateway,
+ &props->routes);
+ // Remove default route from the connection if kIncludedRoutesProperty is set.
+ props->default_route = false;
}
-void StaticIPParameters::RestoreRoutes(const string& property,
- const string& gateway,
- vector<IPConfig::Route>* value_out) {
- value_out->clear();
- if (saved_args_.Contains<Strings>(property)) {
- ParseRoutes(saved_args_.Get<Strings>(property), gateway, value_out);
+void StaticIPParameters::RestoreRoutes(IPConfig::Properties* props) {
+ props->routes.clear();
+ if (saved_args_.Contains<Strings>(kIncludedRoutesProperty)) {
+ ParseRoutes(saved_args_.Get<Strings>(kIncludedRoutesProperty),
+ props->gateway, &props->routes);
}
+ // TODO(b/184533440): original props->default_route could be lost after Apply
+ // -> Restore a StaticIPConfig with IncludedRoutes. This only has an impact
+ // when a IPConfig::Refresh is called after applying such a StaticIPConfig,
+ // and is temporary since StaticIPConfig is re-applied right after.
}
void StaticIPParameters::ApplyTo(IPConfig::Properties* props) {
@@ -236,11 +240,7 @@
ApplyString(kPeerAddressProperty, &props->peer_address);
ApplyInt(kPrefixlenProperty, &props->subnet_prefix);
ApplyStrings(kExcludedRoutesProperty, &props->exclusion_list);
- ApplyRoutes(kIncludedRoutesProperty, props->gateway, &props->routes);
- // Remove default route from the connection if IncludedRoutes is set.
- if (!props->routes.empty()) {
- props->default_route = false;
- }
+ ApplyRoutes(props);
}
void StaticIPParameters::RestoreTo(IPConfig::Properties* props) {
@@ -253,7 +253,7 @@
props->peer_address = saved_args_.Lookup<string>(kPeerAddressProperty, "");
props->subnet_prefix = saved_args_.Lookup<int32_t>(kPrefixlenProperty, 0);
RestoreStrings(kExcludedRoutesProperty, &props->exclusion_list);
- RestoreRoutes(kIncludedRoutesProperty, props->gateway, &props->routes);
+ RestoreRoutes(props);
ClearSavedParameters();
}
diff --git a/shill/static_ip_parameters.h b/shill/static_ip_parameters.h
index 95b7b66..8f8c32e 100644
--- a/shill/static_ip_parameters.h
+++ b/shill/static_ip_parameters.h
@@ -98,12 +98,8 @@
void ParseRoutes(const std::vector<std::string>& route_list,
const std::string& gateway,
std::vector<IPConfig::Route>* value_out);
- void ApplyRoutes(const std::string& property,
- const std::string& gateway,
- std::vector<IPConfig::Route>* value_out);
- void RestoreRoutes(const std::string& property,
- const std::string& gateway,
- std::vector<IPConfig::Route>* value_out);
+ void ApplyRoutes(IPConfig::Properties* props);
+ void RestoreRoutes(IPConfig::Properties* props);
KeyValueStore GetSavedIPConfig(Error* error);
KeyValueStore GetStaticIPConfig(Error* error);
diff --git a/shill/static_ip_parameters_test.cc b/shill/static_ip_parameters_test.cc
index 7f4415f..79d819a 100644
--- a/shill/static_ip_parameters_test.cc
+++ b/shill/static_ip_parameters_test.cc
@@ -65,6 +65,7 @@
EXPECT_FALSE(ipconfig_props_.subnet_prefix);
EXPECT_TRUE(ipconfig_props_.exclusion_list.empty());
EXPECT_TRUE(ipconfig_props_.routes.empty());
+ EXPECT_TRUE(ipconfig_props_.default_route);
}
// Modify an IP address string in some predictable way. There's no need
// for the output string to be valid from a networking perspective.
@@ -112,6 +113,7 @@
ipconfig_props_.routes[0].prefix);
EXPECT_EQ(VersionedAddress(kIncludedRoute0.gateway, version),
ipconfig_props_.routes[0].gateway);
+ EXPECT_FALSE(ipconfig_props_.default_route);
}
void ExpectPopulatedIPConfig() { ExpectPopulatedIPConfigWithVersion(0); }
void ExpectPropertiesWithVersion(PropertyStore* store,
@@ -157,6 +159,7 @@
ipconfig_props_.subnet_prefix = kPrefixLen;
ipconfig_props_.exclusion_list = {kExcludedRoute0, kExcludedRoute1};
ipconfig_props_.routes = {kIncludedRoute0};
+ ipconfig_props_.default_route = false;
}
void SetStaticProperties(PropertyStore* store) {
SetStaticPropertiesWithVersion(store, 0);
@@ -184,6 +187,14 @@
Error error;
store->SetKeyValueStoreProperty(kStaticIPConfigProperty, args, &error);
}
+ void SetStaticPropertiesWithoutRoute(PropertyStore* store) {
+ KeyValueStore args;
+ args.Set<string>(kAddressProperty, kAddress);
+ args.Set<string>(kGatewayProperty, kGateway);
+ args.Set<int32_t>(kMtuProperty, kMtu);
+ Error error;
+ store->SetKeyValueStoreProperty(kStaticIPConfigProperty, args, &error);
+ }
KeyValueStore* static_args() { return &static_params_.args_; }
KeyValueStore* saved_args() { return &static_params_.saved_args_; }
@@ -207,6 +218,18 @@
ExpectPopulatedIPConfig();
}
+TEST_F(StaticIPParametersTest, DefaultRoute) {
+ IPConfig::Properties props;
+ PropertyStore store;
+ static_params_.PlumbPropertyStore(&store);
+ SetStaticPropertiesWithoutRoute(&store);
+ static_params_.ApplyTo(&props);
+ EXPECT_TRUE(props.default_route);
+ SetStaticProperties(&store);
+ static_params_.ApplyTo(&props);
+ EXPECT_FALSE(props.default_route);
+}
+
TEST_F(StaticIPParametersTest, ControlInterface) {
PropertyStore store;
int version = 0;
