From 760364376ebfefa5697597026a94dc0388d7ac6c Mon Sep 17 00:00:00 2001 From: Adam Dov Date: Tue, 28 Jul 2020 11:57:22 +0000 Subject: [PATCH 01/19] bml: BML_CLIENT: change sta_mac type from char to uint8_t The type of the mac address parameter is incorrect. Changed sta_mac from char[6] to uint8_t[6]. PPM-5. Signed-off-by: Itay Elenzweig Signed-off-by: Adam Dov --- controller/src/beerocks/bml/bml_defs.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controller/src/beerocks/bml/bml_defs.h b/controller/src/beerocks/bml/bml_defs.h index 9e080d44ff..57ec4e71af 100644 --- a/controller/src/beerocks/bml/bml_defs.h +++ b/controller/src/beerocks/bml/bml_defs.h @@ -599,7 +599,7 @@ struct BML_CLIENT_CONFIG { struct BML_CLIENT { // Client MAC. - char sta_mac[BML_MAC_ADDR_LEN]; + uint8_t sta_mac[BML_MAC_ADDR_LEN]; // Time of last client configuration edit (in Seconds) uint32_t timestamp_sec; From 79efc5ebfb22554f1adf8c684fe5c732a146db7f Mon Sep 17 00:00:00 2001 From: Adam Dov Date: Tue, 28 Jul 2020 12:12:55 +0000 Subject: [PATCH 02/19] beerocks_cli_bml: Fix client APIs - In client_set_client changed incoming type from INT_ARG to STRING_ARG. To use parameters as optional parameters, we need to retrieve them from the CLI as strings and convert them to integers. - Added int conversion in the client_set_client LOG print. To print the incoming values in client_set_client, need to convert them to type int, otherwise, they will be printed as Char. - Added tlvf::mac_to_string in the client_get_client LOG print. The received mac address needs to be converted to a readable format to be printed. PPM-5. Signed-off-by: Itay Elenzweig --- controller/src/beerocks/cli/beerocks_cli_bml.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/controller/src/beerocks/cli/beerocks_cli_bml.cpp b/controller/src/beerocks/cli/beerocks_cli_bml.cpp index 7883fe482b..42d7afe154 100644 --- a/controller/src/beerocks/cli/beerocks_cli_bml.cpp +++ b/controller/src/beerocks/cli/beerocks_cli_bml.cpp @@ -563,8 +563,8 @@ void cli_bml::setFunctionsMapAndArray() " selected_bands - Bitwise parameter, 1 for 2.4G, 2 for 5G, 3 for both, 0 for Disabled" " stay_on_initial_radio - 1 for true, 0 for false or (default) -1 for not configured," " stay_on_selected_device - 1 for true, 0 for false or (default) -1 for not configured", - static_cast(&cli_bml::client_set_client_caller), 2, 4, STRING_ARG, INT_ARG, - INT_ARG, INT_ARG); + static_cast(&cli_bml::client_set_client_caller), 2, 4, STRING_ARG, STRING_ARG, + STRING_ARG, STRING_ARG); insertCommandToMap("bml_client_get_client", "", "Get client with the given STA MAC.", static_cast(&cli_bml::client_get_client_caller), 1, 1, STRING_ARG); @@ -2230,9 +2230,9 @@ int cli_bml::client_set_client(const std::string &sta_mac, int8_t selected_bands { std::cout << "client_set_client: " << std::endl << " sta_mac: " << sta_mac << std::endl - << " selected_bands: " << selected_bands << std::endl - << " stay_on_initial_radio: " << stay_on_initial_radio << std::endl - << " stay_on_selected_device: " << stay_on_selected_device << std::endl; + << " selected_bands: " << int(selected_bands) << std::endl + << " stay_on_initial_radio: " << int(stay_on_initial_radio) << std::endl + << " stay_on_selected_device: " << int(stay_on_selected_device) << std::endl; BML_CLIENT_CONFIG cfg{ .stay_on_initial_radio = stay_on_initial_radio, @@ -2285,7 +2285,7 @@ int cli_bml::client_get_client(const std::string &sta_mac) return ret; }; - std::cout << "client: " << client.sta_mac << std::endl + std::cout << "client: " << tlvf::mac_to_string(client.sta_mac) << std::endl << " timestamp_sec: " << client.timestamp_sec << std::endl << " stay_on_initial_radio: " << client_bool_print(client.stay_on_initial_radio) << std::endl From b1f3d97d399183cdf900196ab7ea607c7362bf5a Mon Sep 17 00:00:00 2001 From: Adam Dov Date: Tue, 28 Jul 2020 12:29:25 +0000 Subject: [PATCH 03/19] controller: bml: fix return values Changed returning error values in the code to a negative values as expected by the BML. PPM-5. Signed-off-by: Itay Elenzweig Signed-off-by: Adam Dov --- controller/src/beerocks/bml/internal/bml_internal.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/controller/src/beerocks/bml/internal/bml_internal.cpp b/controller/src/beerocks/bml/internal/bml_internal.cpp index 43f5e6e84f..45f8293af0 100644 --- a/controller/src/beerocks/bml/internal/bml_internal.cpp +++ b/controller/src/beerocks/bml/internal/bml_internal.cpp @@ -1141,13 +1141,13 @@ int bml_internal::process_cmdu_header(std::shared_ptr beerocks_ ->addClass(); if (!response) { LOG(ERROR) << "addClass cACTION_BML_CLIENT_GET_CLIENT_LIST_RESPONSE failed"; - return BML_RET_OP_FAILED; + return (-BML_RET_OP_FAILED); } if (!m_client_list || !m_client_list_size) { LOG(ERROR) << "The pointer to the user data buffer is null!"; m_prmClientListGet->set_value(false); - break; + return (-BML_RET_INVALID_ARGS); } if (response->result() != 0) { @@ -1195,7 +1195,7 @@ int bml_internal::process_cmdu_header(std::shared_ptr beerocks_ ->addClass(); if (!response) { LOG(ERROR) << "addClass cACTION_BML_CLIENT_SET_CLIENT_RESPONSE failed"; - return BML_RET_OP_FAILED; + return (-BML_RET_OP_FAILED); } ///Signal any waiting threads @@ -1212,7 +1212,7 @@ int bml_internal::process_cmdu_header(std::shared_ptr beerocks_ ->addClass(); if (!response) { LOG(ERROR) << "addClass cACTION_BML_CLIENT_GET_CLIENT_RESPONSE failed"; - return BML_RET_OP_FAILED; + return (-BML_RET_OP_FAILED); } //Signal any waiting threads @@ -1931,7 +1931,7 @@ int bml_internal::client_get_client_list(char *client_list, unsigned int *client LOG(DEBUG) << "Promise resolved, received " << client_mac_list.size() << " clients"; if (!result) { LOG(ERROR) << "Get client list request failed"; - return -BML_RET_OP_FAILED; + return (-BML_RET_OP_FAILED); } // Translate sMacAddr list to string From f24b17682da130261c768bb826ff632fdde503bd Mon Sep 17 00:00:00 2001 From: Adam Dov Date: Wed, 29 Jul 2020 15:01:59 +0000 Subject: [PATCH 04/19] bml: cli: fix selected_bands debug prints Removed excess spaces from selected-bands debug print. PPM-5. Signed-off-by: Adam Dov --- controller/src/beerocks/cli/beerocks_cli_bml.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/controller/src/beerocks/cli/beerocks_cli_bml.cpp b/controller/src/beerocks/cli/beerocks_cli_bml.cpp index 42d7afe154..885d72720d 100644 --- a/controller/src/beerocks/cli/beerocks_cli_bml.cpp +++ b/controller/src/beerocks/cli/beerocks_cli_bml.cpp @@ -2270,13 +2270,13 @@ int cli_bml::client_get_client(const std::string &sta_mac) if (val == BML_CLIENT_SELECTED_BANDS_DISABLED) return "Disabled"; if (val & BML_CLIENT_SELECTED_BANDS_24G) - ret += "2.4 Ghz,"; + ret += "2.4GHz,"; if (val & BML_CLIENT_SELECTED_BANDS_5G) - ret += "5 Ghz,"; + ret += "5GHz,"; if (val & BML_CLIENT_SELECTED_BANDS_6G) - ret += "6 Ghz,"; + ret += "6GHz,"; if (val & BML_CLIENT_SELECTED_BANDS_60G) - ret += "60 Ghz,"; + ret += "60GHz,"; // remove ending comma if (!ret.empty() && (ret.back() == ',')) { From 6a71e2c3d9dfd593a8e49ae92a3da3fc9a92d57d Mon Sep 17 00:00:00 2001 From: Adam Dov Date: Wed, 29 Jul 2020 14:23:13 +0000 Subject: [PATCH 05/19] bml: remove the unsupported stay_on_selected_device The introduction of the stay_on_selected_device configuration, its DB support, and utilization delayed to the next phase of the smart-steering feature. Removed the stay_on_selected_device from the BML interface - from set_client and get_client APIs related structs. Removed the stay_on_selected_device from the BML CLI interface - both from set_client and get_client CLI functions. Note that the stay_on_selected_device infrastructure in the BML message to/from the controller is not removed, but not used. PPM-5. Signed-off-by: Adam Dov --- controller/src/beerocks/bml/bml_defs.h | 6 ---- .../beerocks/bml/internal/bml_internal.cpp | 19 ++++++----- .../src/beerocks/cli/beerocks_cli_bml.cpp | 33 ++++++------------- .../src/beerocks/cli/beerocks_cli_bml.h | 2 +- 4 files changed, 22 insertions(+), 38 deletions(-) diff --git a/controller/src/beerocks/bml/bml_defs.h b/controller/src/beerocks/bml/bml_defs.h index 57ec4e71af..480d377c44 100644 --- a/controller/src/beerocks/bml/bml_defs.h +++ b/controller/src/beerocks/bml/bml_defs.h @@ -589,9 +589,6 @@ struct BML_CLIENT_CONFIG { // 1 for true, 0 for false, -1 for "not configured". int8_t stay_on_initial_radio; - // 1 for true, 0 for false, -1 for "not configured". - int8_t stay_on_selected_device; - // Bitwise value of selected bands for the client. // Correlates to BML_CLIENT_SELECTED_BANDS int8_t selected_bands; @@ -610,9 +607,6 @@ struct BML_CLIENT { // 1 for true, 0 for false, -1 for "not configured". int8_t stay_on_initial_radio; - // 1 for true, 0 for false, -1 for "not configured". - int8_t stay_on_selected_device; - // Bitwise value of selected bands for the client. // Correlates to BML_CLIENT_SELECTED_BANDS int8_t selected_bands; diff --git a/controller/src/beerocks/bml/internal/bml_internal.cpp b/controller/src/beerocks/bml/internal/bml_internal.cpp index 45f8293af0..ff6660473f 100644 --- a/controller/src/beerocks/bml/internal/bml_internal.cpp +++ b/controller/src/beerocks/bml/internal/bml_internal.cpp @@ -1221,11 +1221,12 @@ int bml_internal::process_cmdu_header(std::shared_ptr beerocks_ if (m_client) { std::copy_n(response->client().sta_mac.oct, BML_MAC_ADDR_LEN, m_client->sta_mac); - m_client->timestamp_sec = response->client().timestamp_sec; - m_client->stay_on_initial_radio = response->client().stay_on_initial_radio; - m_client->stay_on_selected_device = response->client().stay_on_selected_device; - m_client->selected_bands = response->client().selected_bands; - m_client->time_life_delay_days = response->client().time_life_delay_days; + m_client->timestamp_sec = response->client().timestamp_sec; + m_client->stay_on_initial_radio = response->client().stay_on_initial_radio; + // TODO: add stay_on_selected_device to BML_CLIENT when support is added + //m_client->stay_on_selected_device = response->client().stay_on_selected_device; + m_client->selected_bands = response->client().selected_bands; + m_client->time_life_delay_days = response->client().time_life_delay_days; //Resolve promise to "true" promise_result = true; } @@ -1963,9 +1964,11 @@ int bml_internal::client_set_client(const sMacAddr &sta_mac, const BML_CLIENT_CO return (-BML_RET_OP_FAILED); } - request->sta_mac() = sta_mac; - request->client_config().stay_on_initial_radio = client_config.stay_on_initial_radio; - request->client_config().stay_on_selected_device = client_config.stay_on_selected_device; + request->sta_mac() = sta_mac; + request->client_config().stay_on_initial_radio = client_config.stay_on_initial_radio; + // TODO: add stay_on_selected_device to BML_CLIENT when support is added + // currently, it is only available as infrastructure in the request/response message + request->client_config().stay_on_selected_device = PARAMETER_NOT_CONFIGURED; request->client_config().selected_bands = client_config.selected_bands; int result = 0; diff --git a/controller/src/beerocks/cli/beerocks_cli_bml.cpp b/controller/src/beerocks/cli/beerocks_cli_bml.cpp index 885d72720d..d8f16c6b6a 100644 --- a/controller/src/beerocks/cli/beerocks_cli_bml.cpp +++ b/controller/src/beerocks/cli/beerocks_cli_bml.cpp @@ -561,10 +561,9 @@ void cli_bml::setFunctionsMapAndArray() "bml_client_set_client", " []", "Set client with the given STA MAC:" " selected_bands - Bitwise parameter, 1 for 2.4G, 2 for 5G, 3 for both, 0 for Disabled" - " stay_on_initial_radio - 1 for true, 0 for false or (default) -1 for not configured," - " stay_on_selected_device - 1 for true, 0 for false or (default) -1 for not configured", - static_cast(&cli_bml::client_set_client_caller), 2, 4, STRING_ARG, STRING_ARG, - STRING_ARG, STRING_ARG); + " stay_on_initial_radio - 1 for true, 0 for false or (default) -1 for not configured,", + static_cast(&cli_bml::client_set_client_caller), 2, 3, STRING_ARG, STRING_ARG, + STRING_ARG); insertCommandToMap("bml_client_get_client", "", "Get client with the given STA MAC.", static_cast(&cli_bml::client_get_client_caller), 1, 1, STRING_ARG); @@ -1345,13 +1344,11 @@ int cli_bml::client_set_client_caller(int numOfArgs) * Optional: * selected_bands= * stay_on_initial_radio= - * stay_on_selected_device= ]*/ std::string::size_type pos; std::string sta_mac(network_utils::WILD_MAC_STRING); - int8_t selected_bands = BML_PARAMETER_NOT_CONFIGURED; - int8_t stay_on_initial_radio = BML_PARAMETER_NOT_CONFIGURED; - int8_t stay_on_selected_device = BML_PARAMETER_NOT_CONFIGURED; + int8_t selected_bands = BML_PARAMETER_NOT_CONFIGURED; + int8_t stay_on_initial_radio = BML_PARAMETER_NOT_CONFIGURED; if (numOfArgs > 1) { sta_mac = args.stringArgs[0]; for (int i = 1; i < numOfArgs; i++) { //first optional arg @@ -1362,14 +1359,9 @@ int cli_bml::client_set_client_caller(int numOfArgs) std::string::npos) { stay_on_initial_radio = string_utils::stoi( args.stringArgs[i].substr(pos + sizeof("stay_on_initial_radio"))); - } else if ((pos = args.stringArgs[i].find("stay_on_selected_device=")) != - std::string::npos) { - stay_on_selected_device = string_utils::stoi( - args.stringArgs[i].substr(pos + sizeof("stay_on_selected_device"))); } } - return client_set_client(sta_mac, selected_bands, stay_on_initial_radio, - stay_on_selected_device); + return client_set_client(sta_mac, selected_bands, stay_on_initial_radio); } return -1; } @@ -2222,22 +2214,19 @@ int cli_bml::client_get_client_list() * @param [in] sta_mac MAC address of requested client. * @param [in] selected_bands comma-seperated selected bands. * @param [in] stay_on_initial_radio Whather to stay on initial radio or not. - * @param [in] stay_on_selected_device Whather to stay on selected device or not. * @return 0 on success. */ int cli_bml::client_set_client(const std::string &sta_mac, int8_t selected_bands, - int8_t stay_on_initial_radio, int8_t stay_on_selected_device) + int8_t stay_on_initial_radio) { std::cout << "client_set_client: " << std::endl << " sta_mac: " << sta_mac << std::endl << " selected_bands: " << int(selected_bands) << std::endl - << " stay_on_initial_radio: " << int(stay_on_initial_radio) << std::endl - << " stay_on_selected_device: " << int(stay_on_selected_device) << std::endl; + << " stay_on_initial_radio: " << int(stay_on_initial_radio) << std::endl; BML_CLIENT_CONFIG cfg{ - .stay_on_initial_radio = stay_on_initial_radio, - .stay_on_selected_device = stay_on_selected_device, - .selected_bands = selected_bands, + .stay_on_initial_radio = stay_on_initial_radio, + .selected_bands = selected_bands, }; int ret = bml_client_set_client(ctx, sta_mac.c_str(), &cfg); @@ -2289,8 +2278,6 @@ int cli_bml::client_get_client(const std::string &sta_mac) << " timestamp_sec: " << client.timestamp_sec << std::endl << " stay_on_initial_radio: " << client_bool_print(client.stay_on_initial_radio) << std::endl - << " stay_on_selected_device: " - << client_bool_print(client.stay_on_selected_device) << std::endl << " selected_bands: " << client_selected_bands_print(client.selected_bands) << std::endl << " single_band: " << client_bool_print(client.single_band) << std::endl diff --git a/controller/src/beerocks/cli/beerocks_cli_bml.h b/controller/src/beerocks/cli/beerocks_cli_bml.h index a368d1c382..a3055e77db 100644 --- a/controller/src/beerocks/cli/beerocks_cli_bml.h +++ b/controller/src/beerocks/cli/beerocks_cli_bml.h @@ -216,7 +216,7 @@ class cli_bml : public cli { bool is_single_scan = false); int client_get_client_list(); int client_set_client(const std::string &sta_mac, int8_t selected_bands, - int8_t stay_on_initial_radio, int8_t stay_on_selected_device); + int8_t stay_on_initial_radio); int client_get_client(const std::string &sta_mac); template const std::string string_from_int_array(T *arr, size_t arr_max_size); // Variable From 4ee974e60d775b419cc5157326a34fd1d57d4e66 Mon Sep 17 00:00:00 2001 From: Adam Dov Date: Tue, 28 Jul 2020 12:53:10 +0000 Subject: [PATCH 06/19] controller: bml: refactor client_get_client Added verification of "is someone waiting for the promise" before handling the response. Implement the function with error-handling first according to the agreed coding guidelines. Fixed broken return-on-error in the "get_client" implementation. PPM-5. Signed-off-by: Itay Elenzweig Signed-off-by: Adam Dov --- .../beerocks/bml/internal/bml_internal.cpp | 58 ++++++++++++------- 1 file changed, 38 insertions(+), 20 deletions(-) diff --git a/controller/src/beerocks/bml/internal/bml_internal.cpp b/controller/src/beerocks/bml/internal/bml_internal.cpp index ff6660473f..8c138e567f 100644 --- a/controller/src/beerocks/bml/internal/bml_internal.cpp +++ b/controller/src/beerocks/bml/internal/bml_internal.cpp @@ -1207,6 +1207,13 @@ int bml_internal::process_cmdu_header(std::shared_ptr beerocks_ } break; case beerocks_message::ACTION_BML_CLIENT_GET_CLIENT_RESPONSE: { LOG(DEBUG) << "ACTION_BML_CLIENT_GET_CLIENT_RESPONSE received"; + + if (!m_prmClientGet) { + LOG(WARNING) << "Received ACTION_BML_CLIENT_GET_CLIENT_RESPONSE response, " + << "but no one is waiting..."; + break; + } + auto response = beerocks_header ->addClass(); @@ -1215,26 +1222,29 @@ int bml_internal::process_cmdu_header(std::shared_ptr beerocks_ return (-BML_RET_OP_FAILED); } - //Signal any waiting threads - if (m_prmClientGet) { - bool promise_result = false; - if (m_client) { - std::copy_n(response->client().sta_mac.oct, BML_MAC_ADDR_LEN, - m_client->sta_mac); - m_client->timestamp_sec = response->client().timestamp_sec; - m_client->stay_on_initial_radio = response->client().stay_on_initial_radio; - // TODO: add stay_on_selected_device to BML_CLIENT when support is added - //m_client->stay_on_selected_device = response->client().stay_on_selected_device; - m_client->selected_bands = response->client().selected_bands; - m_client->time_life_delay_days = response->client().time_life_delay_days; - //Resolve promise to "true" - promise_result = true; - } - m_prmClientGet->set_value(promise_result); - } else { - LOG(WARNING) << "Received ACTION_BML_CLIENT_GET_CLIENT_RESPONSE response, " - << "but no one is waiting..."; + if (response->result() != 0) { + LOG(ERROR) << "CLIENT_GET_CLIENT_REQUEST failed with error: " << response->result(); + m_prmClientGet->set_value(false); + break; + } + + if (!m_client) { + LOG(ERROR) << "The pointer to the user client data is null!"; + m_prmClientGet->set_value(false); + break; } + + std::copy_n(response->client().sta_mac.oct, BML_MAC_ADDR_LEN, m_client->sta_mac); + m_client->timestamp_sec = response->client().timestamp_sec; + m_client->stay_on_initial_radio = response->client().stay_on_initial_radio; + // TODO: add stay_on_selected_device to BML_CLIENT when support is added + //m_client->stay_on_selected_device = response->client().stay_on_selected_device; + m_client->selected_bands = response->client().selected_bands; + m_client->single_band = response->client().single_band; + m_client->time_life_delay_days = response->client().time_life_delay_days; + + //Resolve promise to "true" + m_prmClientGet->set_value(true); } break; default: { LOG(WARNING) << "unhandled header BML action type 0x" << std::hex @@ -2036,7 +2046,15 @@ int bml_internal::client_get_client(const sMacAddr &sta_mac, BML_CLIENT *client) // Clear the promise holder m_prmClientGet = nullptr; - LOG_IF(iRet != BML_RET_OK, ERROR) << "Get client request returned with error code:" << iRet; + if (iRet != BML_RET_OK) { + LOG(ERROR) << "Get client request returned with error code:" << iRet; + return (iRet); + } + + if (!prmClientGet.get_value()) { + LOG(ERROR) << "Get client request failed"; + return (-BML_RET_OP_FAILED); + } return BML_RET_OK; } From 21c8e86df4c5c7954ab9c7a239572c55b39fb891 Mon Sep 17 00:00:00 2001 From: Adam Dov Date: Tue, 28 Jul 2020 13:19:39 +0000 Subject: [PATCH 07/19] controller: bml: validate set_client input parameters The BML APIs provide an optional-parameters interface, meaning the user can use the API with some of the parameters set to "NOT_CONFIGURED" value, and these parameters not configured in the DB. However, using the API with all the parameters set to "NOT_CONFIGURED" value is not a valid case and should be handled as an error. Added verification that at least one of the given parameters not set to "NOT_CONFIGURED" value. PPM-5. Signed-off-by: Itay Elenzweig Signed-off-by: Adam Dov --- controller/src/beerocks/bml/internal/bml_internal.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/controller/src/beerocks/bml/internal/bml_internal.cpp b/controller/src/beerocks/bml/internal/bml_internal.cpp index 8c138e567f..4756d8b0aa 100644 --- a/controller/src/beerocks/bml/internal/bml_internal.cpp +++ b/controller/src/beerocks/bml/internal/bml_internal.cpp @@ -1965,6 +1965,12 @@ int bml_internal::client_set_client(const sMacAddr &sta_mac, const BML_CLIENT_CO { LOG(DEBUG) << "client_set_client"; + if ((client_config.stay_on_initial_radio == BML_PARAMETER_NOT_CONFIGURED) && + (client_config.selected_bands == BML_PARAMETER_NOT_CONFIGURED)) { + LOG(WARNING) << "No parameter is requested to be configured, returning"; + return (-BML_RET_INVALID_ARGS); + } + auto request = message_com::create_vs_message( cmdu_tx); From 1336b0ee7b3d7b06360089e89ac1fb94c4515dce Mon Sep 17 00:00:00 2001 From: Adam Dov Date: Wed, 22 Jul 2020 05:37:57 +0000 Subject: [PATCH 08/19] controller: db: change aging check to use the remaining time The current condition of comparing the client's passed-timelife-delay against max-timelife-delay is correct but doesn't reflect the actual logic behind it which is "is remaining timelife over". This change enables a more coherent comparison of current-client-remaining-timelife-delay against a candidate-client-for-removal-timelife-delay (if the DB is full). Changed the aging condition to "client-remaining-time < 0". PPM-5. Signed-off-by: Adam Dov --- controller/src/beerocks/master/db/db.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/controller/src/beerocks/master/db/db.cpp b/controller/src/beerocks/master/db/db.cpp index 39fab4c7d4..abaffaecd9 100644 --- a/controller/src/beerocks/master/db/db.cpp +++ b/controller/src/beerocks/master/db/db.cpp @@ -2889,7 +2889,8 @@ bool db::load_persistent_db_clients() * 2.b. Is "unfriendly" something we know at boot? */ static const int max_timelife_delay_sec = config.max_timelife_delay_days * 24 * 3600; - if (client_timelife_passed_sec > max_timelife_delay_sec) { + auto client_remaining_timelife_sec = max_timelife_delay_sec - client_timelife_passed_sec; + if (client_remaining_timelife_sec <= 0) { LOG(ERROR) << "Invalid entry - configured data has aged for client entry " << client_entry; // Calling BPL API directly as there's no need to increment/decrement counter at this point From 23523a582a0d94208075e5376058f01691a2223b Mon Sep 17 00:00:00 2001 From: Adam Dov Date: Wed, 22 Jul 2020 07:17:33 +0000 Subject: [PATCH 09/19] controller: db: change client_db_entry_to_mac to get a copy The current client_db_entry_to_mac receives a const& to the mac address and internally makes a copy to perform an in-place replacement of characters. Changed the function to receive a copy instead of a const& and changed the implementation accordingly. PPM-5. Signed-off-by: Adam Dov --- controller/src/beerocks/master/db/db.cpp | 10 ++++------ controller/src/beerocks/master/db/db.h | 2 +- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/controller/src/beerocks/master/db/db.cpp b/controller/src/beerocks/master/db/db.cpp index abaffaecd9..ab29d8a11e 100644 --- a/controller/src/beerocks/master/db/db.cpp +++ b/controller/src/beerocks/master/db/db.cpp @@ -59,17 +59,15 @@ std::string db::client_db_entry_from_mac(const sMacAddr &mac) return db_entry; } -sMacAddr db::client_db_entry_to_mac(const std::string &db_entry) +sMacAddr db::client_db_entry_to_mac(std::string db_entry) { - std::string entry = db_entry; + std::replace(db_entry.begin(), db_entry.end(), '_', ':'); - std::replace(entry.begin(), entry.end(), '_', ':'); - - if (!network_utils::is_valid_mac(entry)) { + if (!network_utils::is_valid_mac(db_entry)) { return network_utils::ZERO_MAC; } - return tlvf::mac_from_string(entry); + return tlvf::mac_from_string(db_entry); } std::string db::timestamp_to_string_seconds(const std::chrono::steady_clock::time_point timestamp) diff --git a/controller/src/beerocks/master/db/db.h b/controller/src/beerocks/master/db/db.h index 35b212a91d..c1ea1f56f2 100644 --- a/controller/src/beerocks/master/db/db.h +++ b/controller/src/beerocks/master/db/db.h @@ -196,7 +196,7 @@ class db { * @param db_entry Client entry name in persistent db. * @return sMacAddr MAC address of the client the db_entry is representing. On failure ZERO_MAC is returned. */ - static sMacAddr client_db_entry_to_mac(const std::string &db_entry); + static sMacAddr client_db_entry_to_mac(std::string db_entry); /** * @brief Get string representation of number of seconds in timestamp. From ed01d75787b747b49163433a5cd7da6f430281b3 Mon Sep 17 00:00:00 2001 From: Adam Dov Date: Wed, 22 Jul 2020 08:34:39 +0000 Subject: [PATCH 10/19] controller: db: add `using ValuesMap` for cleaner code Added `using ValuesMap = std::unordered_map` and replaced DB APIs and private function to use the new `ValuesMap` instead of `std::unordered_map`. The above change is a fix to comment raised in a previous PR, and it results in much cleaner code. PPM-5. Signed-off-by: Adam Dov --- controller/src/beerocks/master/db/db.cpp | 30 ++++++++++-------------- controller/src/beerocks/master/db/db.h | 22 ++++++++--------- 2 files changed, 24 insertions(+), 28 deletions(-) diff --git a/controller/src/beerocks/master/db/db.cpp b/controller/src/beerocks/master/db/db.cpp index ab29d8a11e..3e38d2868e 100644 --- a/controller/src/beerocks/master/db/db.cpp +++ b/controller/src/beerocks/master/db/db.cpp @@ -2380,8 +2380,7 @@ bool db::is_client_in_persistent_db(const sMacAddr &mac) return bpl::db_has_entry(type_to_string(beerocks::eType::TYPE_CLIENT), client_db_entry); } -bool db::add_client_to_persistent_db(const sMacAddr &mac, - const std::unordered_map ¶ms) +bool db::add_client_to_persistent_db(const sMacAddr &mac, const ValuesMap ¶ms) { // if persistent db is disabled if (!config.persistent_db) { @@ -2459,7 +2458,7 @@ bool db::set_client_time_life_delay(const sMacAddr &mac, } else { LOG(DEBUG) << "configuring persistent-db, timelife = " << time_life_delay_sec.count(); - std::unordered_map values_map; + ValuesMap values_map; values_map[TIMESTAMP_STR] = timestamp_to_string_seconds(timestamp); values_map[TIMELIFE_DELAY_STR] = std::to_string(time_life_delay_sec.count()); @@ -2508,7 +2507,7 @@ bool db::set_client_stay_on_initial_radio(const sMacAddr &mac, bool stay_on_init LOG(DEBUG) << "configuring persistent-db, initial_radio_enable = " << stay_on_initial_radio; - std::unordered_map values_map; + ValuesMap values_map; values_map[TIMESTAMP_STR] = timestamp_to_string_seconds(timestamp); values_map[INITIAL_RADIO_ENABLE_STR] = std::to_string(stay_on_initial_radio); @@ -2557,7 +2556,7 @@ bool db::set_client_initial_radio(const sMacAddr &mac, const sMacAddr &initial_r } else { LOG(DEBUG) << "configuring persistent-db, initial_radio = " << initial_radio_mac; - std::unordered_map values_map; + ValuesMap values_map; values_map[TIMESTAMP_STR] = timestamp_to_string_seconds(timestamp); values_map[INITIAL_RADIO_STR] = tlvf::mac_to_string(initial_radio_mac); @@ -2606,7 +2605,7 @@ bool db::set_client_stay_on_selected_band(const sMacAddr &mac, bool stay_on_sele LOG(DEBUG) << "configuring persistent-db, selected_band_enable = " << stay_on_selected_band; - std::unordered_map values_map; + ValuesMap values_map; values_map[TIMESTAMP_STR] = timestamp_to_string_seconds(timestamp); values_map[SELECTED_BAND_ENABLE_STR] = std::to_string(stay_on_selected_band); @@ -2655,7 +2654,7 @@ bool db::set_client_selected_bands(const sMacAddr &mac, beerocks::eFreqType sele } else { LOG(DEBUG) << ", configuring persistent-db, selected_bands = " << int(selected_bands); - std::unordered_map values_map; + ValuesMap values_map; values_map[TIMESTAMP_STR] = timestamp_to_string_seconds(timestamp); values_map[SELECTED_BANDS_STR] = std::to_string(selected_bands); @@ -2741,7 +2740,7 @@ bool db::update_client_persistent_db(const sMacAddr &mac) return true; } - std::unordered_map values_map; + ValuesMap values_map; //fill values map of client persistent params values_map[TIMESTAMP_STR] = timestamp_to_string_seconds(node->client_parameters_last_edit); @@ -2797,7 +2796,7 @@ bool db::load_persistent_db_clients() return false; } - std::unordered_map> clients; + std::unordered_map clients; if (!bpl::db_get_entries_by_type(type_to_string(beerocks::eType::TYPE_CLIENT), clients)) { LOG(ERROR) << "Failed to get all clients from persistent DB"; return false; @@ -2820,8 +2819,7 @@ bool db::load_persistent_db_clients() // Add node for client and fill with persistent data. auto add_new_client_with_persistent_data_to_nodes_list = - [&](const sMacAddr &client_mac, - const std::unordered_map values_map) -> bool { + [&](const sMacAddr &client_mac, const ValuesMap values_map) -> bool { // Add client node with defaults and in default location if (!add_node(client_mac)) { LOG(ERROR) << "Failed to add client node for client_entry " << client_entry; @@ -4221,8 +4219,7 @@ void db::set_prplmesh(const sMacAddr &mac) get_node(mac)->is_prplmesh = true; } -bool db::update_client_entry_in_persistent_db( - const sMacAddr &mac, const std::unordered_map &values_map) +bool db::update_client_entry_in_persistent_db(const sMacAddr &mac, const ValuesMap &values_map) { auto db_entry = client_db_entry_from_mac(mac); auto type_client_str = type_to_string(beerocks::eType::TYPE_CLIENT); @@ -4240,8 +4237,7 @@ bool db::update_client_entry_in_persistent_db( return true; } -bool db::set_node_params_from_map(const sMacAddr &mac, - const std::unordered_map &values_map) +bool db::set_node_params_from_map(const sMacAddr &mac, const ValuesMap &values_map) { auto node = get_node(mac); if (!node) { @@ -4288,8 +4284,8 @@ bool db::set_node_params_from_map(const sMacAddr &mac, return true; } -bool db::add_client_entry_and_update_counter( - const std::string &entry_name, const std::unordered_map &values_map) +bool db::add_client_entry_and_update_counter(const std::string &entry_name, + const ValuesMap &values_map) { if (!bpl::db_add_entry(type_to_string(beerocks::eType::TYPE_CLIENT), entry_name, values_map)) { LOG(ERROR) << "failed to add client entry " << entry_name << " to persistent db"; diff --git a/controller/src/beerocks/master/db/db.h b/controller/src/beerocks/master/db/db.h index c1ea1f56f2..ad00bc6897 100644 --- a/controller/src/beerocks/master/db/db.h +++ b/controller/src/beerocks/master/db/db.h @@ -45,6 +45,11 @@ class db { } sBmlListener; public: + /** + * @brief An unordered map of parameters and their values. + */ + using ValuesMap = std::unordered_map; + /** * @brief Client parameter names. * The parameter names can be used to set/get multiple parameters in one-shot. @@ -652,12 +657,10 @@ class db { * @brief Adds a client to the persistent db, if already exists, remove old entry and add a new one. * * @param mac MAC address of a client. - * @param params An unordered map of key-value of client parameters. + * @param params An unordered map of key-value of client parameters and their values. * @return true on success, otherwise false. */ - bool add_client_to_persistent_db(const sMacAddr &mac, - const std::unordered_map ¶ms = - std::unordered_map()); + bool add_client_to_persistent_db(const sMacAddr &mac, const ValuesMap ¶ms = {}); /** * @brief Get the client's parameters last edit time. @@ -1091,8 +1094,7 @@ class db { * @param values_map A map of client params and their values. * @return true on success, otherwise false. */ - bool update_client_entry_in_persistent_db( - const sMacAddr &mac, const std::unordered_map &values_map); + bool update_client_entry_in_persistent_db(const sMacAddr &mac, const ValuesMap &values_map); /** * @brief Sets the node params (runtime db) from a param-value map. @@ -1101,8 +1103,7 @@ class db { * @param values_map A map of client params and their values. * @return true on success, otherwise false. */ - bool set_node_params_from_map(const sMacAddr &mac, - const std::unordered_map &values_map); + bool set_node_params_from_map(const sMacAddr &mac, const ValuesMap &values_map); /** * @brief Adds a client entry to persistent_db with configured parameters and increments clients counter. @@ -1111,9 +1112,8 @@ class db { * @param values_map A map of client params and their values. * @return true on success, otherwise false. */ - bool add_client_entry_and_update_counter( - const std::string &entry_name, - const std::unordered_map &values_map); + bool add_client_entry_and_update_counter(const std::string &entry_name, + const ValuesMap &values_map); /** * @brief Removes a client entry from persistent_db and decrements clients counter. From 59f88200027146a28b25c1ec5fc50438bd857c7e Mon Sep 17 00:00:00 2001 From: Adam Dov Date: Thu, 23 Jul 2020 23:02:32 +0000 Subject: [PATCH 11/19] controller: db: change selected_bands type The current implementation of selected bands mistakenly used the existing beerocks::eFreqType as the storage type. This is not extensible and doesn't define the full possible configurations. Moreover, by using the newly introduced eClientSelectedBands instead, which is used in the BML<->controller messaging, we can simplify the set and get operations and any future extension of supported values. Changed in node class the storage type of the selected bands and its description. Changed the DB APIs to use the int8_t instead of beerocks::eFreqType. Changed the selected bands` default value from beerocks::eFreqType::FREQ_UNKNOWN to beerocks::PARAMETER_NOT_CONFIGURED. PPM-5. Signed-off-by: Adam Dov --- controller/src/beerocks/master/db/db.cpp | 14 +++++++------- controller/src/beerocks/master/db/db.h | 8 ++++---- controller/src/beerocks/master/db/node.h | 5 +++-- 3 files changed, 14 insertions(+), 13 deletions(-) diff --git a/controller/src/beerocks/master/db/db.cpp b/controller/src/beerocks/master/db/db.cpp index 3e38d2868e..15f5cffdcd 100644 --- a/controller/src/beerocks/master/db/db.cpp +++ b/controller/src/beerocks/master/db/db.cpp @@ -2635,7 +2635,7 @@ eTriStateBool db::get_client_stay_on_selected_band(const sMacAddr &mac) return node->client_stay_on_selected_band; } -bool db::set_client_selected_bands(const sMacAddr &mac, beerocks::eFreqType selected_bands, +bool db::set_client_selected_bands(const sMacAddr &mac, int8_t selected_bands, bool save_to_persistent_db) { auto node = get_node_verify_type(mac, beerocks::TYPE_CLIENT); @@ -2652,7 +2652,7 @@ bool db::set_client_selected_bands(const sMacAddr &mac, beerocks::eFreqType sele if (!config.persistent_db) { LOG(DEBUG) << "persistent db is disabled"; } else { - LOG(DEBUG) << ", configuring persistent-db, selected_bands = " << int(selected_bands); + LOG(DEBUG) << ", configuring persistent-db, selected_bands = " << selected_bands; ValuesMap values_map; values_map[TIMESTAMP_STR] = timestamp_to_string_seconds(timestamp); @@ -2672,12 +2672,12 @@ bool db::set_client_selected_bands(const sMacAddr &mac, beerocks::eFreqType sele return true; } -beerocks::eFreqType db::get_client_selected_bands(const sMacAddr &mac) +int8_t db::get_client_selected_bands(const sMacAddr &mac) { auto node = get_node_verify_type(mac, beerocks::TYPE_CLIENT); if (!node) { LOG(ERROR) << "client node not found for mac " << mac; - return beerocks::eFreqType::FREQ_UNKNOWN; + return PARAMETER_NOT_CONFIGURED; } return node->client_selected_bands; @@ -2698,7 +2698,7 @@ bool db::clear_client_persistent_db(const sMacAddr &mac) node->client_stay_on_initial_radio = eTriStateBool::NOT_CONFIGURED; node->client_initial_radio = network_utils::ZERO_MAC; node->client_stay_on_selected_band = eTriStateBool::NOT_CONFIGURED; - node->client_selected_bands = beerocks::eFreqType::FREQ_UNKNOWN; + node->client_selected_bands = PARAMETER_NOT_CONFIGURED; // if persistent db is enabled if (config.persistent_db) { @@ -2771,7 +2771,7 @@ bool db::update_client_persistent_db(const sMacAddr &mac) values_map[SELECTED_BAND_ENABLE_STR] = std::to_string(enable); } - if (node->client_selected_bands != beerocks::eFreqType::FREQ_UNKNOWN) { + if (node->client_selected_bands != PARAMETER_NOT_CONFIGURED) { LOG(DEBUG) << "setting client selected-bands in persistent-db to for " << mac << " to " << node->client_selected_bands; values_map[SELECTED_BANDS_STR] = std::to_string(node->client_selected_bands); @@ -4275,7 +4275,7 @@ bool db::set_node_params_from_map(const sMacAddr &mac, const ValuesMap &values_m } else if (param.first == SELECTED_BANDS_STR) { LOG(DEBUG) << "setting node client_selected_bands to " << param.second << " for " << mac; - node->client_selected_bands = beerocks::eFreqType(string_utils::stoi(param.second)); + node->client_selected_bands = string_utils::stoi(param.second); } else { LOG(WARNING) << "Unknown parameter, skipping: " << param.first << " for " << mac; } diff --git a/controller/src/beerocks/master/db/db.h b/controller/src/beerocks/master/db/db.h index ad00bc6897..306e0cd793 100644 --- a/controller/src/beerocks/master/db/db.h +++ b/controller/src/beerocks/master/db/db.h @@ -751,20 +751,20 @@ class db { * @brief Set the client's selected-bands. * * @param mac MAC address of a client. - * @param selected_bands Client selected band/bands. FREQ_UNKNOWN is considered as "not-configured". + * @param selected_bands Client selected band/bands. Possible values are bitwise options of eClientSelectedBands. * @param save_to_persistent_db If set to true, update the persistent-db (write-through), default is true. * @return true on success, otherwise false. */ - bool set_client_selected_bands(const sMacAddr &mac, beerocks::eFreqType selected_bands, + bool set_client_selected_bands(const sMacAddr &mac, int8_t selected_bands, bool save_to_persistent_db = true); /** * @brief Get the client's selected-bands. * * @param mac MAC address of a client. - * @return Selected band/bands. + * @return Selected band/bands. Possible values are bitwise options of eClientSelectedBands. */ - beerocks::eFreqType get_client_selected_bands(const sMacAddr &mac); + int8_t get_client_selected_bands(const sMacAddr &mac); /** * @brief Clear client's persistent information. diff --git a/controller/src/beerocks/master/db/node.h b/controller/src/beerocks/master/db/node.h index 89043fd78d..67a7facd4a 100644 --- a/controller/src/beerocks/master/db/node.h +++ b/controller/src/beerocks/master/db/node.h @@ -292,8 +292,9 @@ class node { eTriStateBool client_stay_on_selected_band = eTriStateBool::NOT_CONFIGURED; // The selected bands the client should be steered to if the client_stay_on_selected_band is set. - // Default value is FREQ_UNKNOWN (also indicates value is not configured) - beerocks::eFreqType client_selected_bands = beerocks::eFreqType::FREQ_UNKNOWN; + // Default value is PARAMETER_NOT_CONFIGURED. + // Possible values are bitwise options of eClientSelectedBands. + int8_t client_selected_bands = beerocks::PARAMETER_NOT_CONFIGURED; /* * Persistent configurations - end From c93032c5d62c529631ed6cbdca8963199f1a04c3 Mon Sep 17 00:00:00 2001 From: Adam Dov Date: Wed, 22 Jul 2020 09:43:20 +0000 Subject: [PATCH 12/19] controller: db: load_persistent_db_clients: handle db is full As part of the load_persistent_db_clients() a corner-case where the user added to the persistent DB more than the allowed max-clients-db-size must be handled. Added a check of the number-of-added-clients against the max-clients-db-size - if the condition is met - the clients' DB is full. If clients DB is full, find a candidate client for removal and compare it against the client read from persistent DB and we are trying to add. If the new client's remaining timelife is greater than the timelife of the candidate_for_removal, then the candidate is removed from the runtime DB and the persistent DB and the new client is added instead to the runtime DB. If the new client's remaining timelife is smaller or equal to the timelife of the candidate_for_removal, then the new client is removed from the persistent DB and we continue to the next client. PPM-5. Signed-off-by: Adam Dov --- controller/src/beerocks/master/db/db.cpp | 79 +++++++++++++++++++++--- 1 file changed, 71 insertions(+), 8 deletions(-) diff --git a/controller/src/beerocks/master/db/db.cpp b/controller/src/beerocks/master/db/db.cpp index 15f5cffdcd..1625c35814 100644 --- a/controller/src/beerocks/master/db/db.cpp +++ b/controller/src/beerocks/master/db/db.cpp @@ -2808,9 +2808,10 @@ bool db::load_persistent_db_clients() } // Counters for client nodes-add success/fail (filtered-out client entries are not counted) - int add_node_error_count = 0; - int set_node_error_count = 0; - int clients_added_no_error = 0; + uint16_t add_node_error_count = 0; + uint16_t set_node_error_count = 0; + uint16_t clients_added_no_error = 0; + uint16_t clients_not_added_or_removed_due_to_full_db = 0; // Add clients to runtime db - invalid client (no timestamp or aged entries) are filtered out for (const auto &client : clients) { @@ -2872,12 +2873,15 @@ bool db::load_persistent_db_clients() continue; } + // Save current time as a separate variable for fair comparison of current client + // remaining-timelife-delay against a candidate for removal in case the DB is full. + auto now = std::chrono::steady_clock::now(); + // Aged clients are removed from persistent db and not added to runtime db - auto timestamp_sec = beerocks::string_utils::stoi(timestamp_it->second); - auto timestamp = db::timestamp_from_seconds(timestamp_sec); - auto client_timelife_passed_sec = std::chrono::duration_cast( - std::chrono::steady_clock::now() - timestamp) - .count(); + auto timestamp_sec = beerocks::string_utils::stoi(timestamp_it->second); + auto timestamp = db::timestamp_from_seconds(timestamp_sec); + auto client_timelife_passed_sec = + std::chrono::duration_cast(now - timestamp).count(); /* TODO: aging validation against specific client configuration and unfriendly-device-max-timelife-delay: * 1. If client has timelife_delay_str param configured, it should be checked against it instead of global max-timelife-delay param. @@ -2897,6 +2901,61 @@ bool db::load_persistent_db_clients() continue; } + // If clients DB is full - find candidate for removal and compare against the current client. + // Note that this is a corner case and at the init stage where this functionality is performed we do + // not expect for this condition to be met. + // This is only for robustness against user misuse (adding manually more clients than clients_persistent_db_max_size). + if (clients_added_no_error >= config.clients_persistent_db_max_size) { + ++clients_not_added_or_removed_due_to_full_db; + // Find candidate client for removal + auto candidate_for_removal_mac = get_candidate_client_for_removal(); + if (candidate_for_removal_mac == network_utils::ZERO_MAC) { + LOG(WARNING) << "Failed to find candidate client for removal, unable to check if " + "possible to add " + << client_mac; + continue; + } + + // Get candidate node + auto candidate_node = + get_node_verify_type(candidate_for_removal_mac, eType::TYPE_CLIENT); + if (!candidate_node) { + LOG(WARNING) << "Failed to get node for client " << candidate_for_removal_mac; + continue; + } + + // Calculate candidate client remaining timelife delay. + auto candidate_remaining_time_sec = + std::chrono::duration_cast( + now - candidate_node->client_parameters_last_edit) + .count(); + + // If current client is has less remaining time, just remove it from persistent DB + if (client_remaining_timelife_sec <= candidate_remaining_time_sec) { + LOG(DEBUG) << "Clients DB is full, client has the least remaining timelife, it is " + "not added to the runtime DB and removed from persistent DB: " + << client_mac; + if (!bpl::db_remove_entry(type_client_str, client_entry)) { + LOG(ERROR) << "Failed to remove entry " << client_entry + << " from persistent db"; + } + continue; + } + + // Free-up space in the DB + // clear candidate client's persistent data in runtime db and remove from persistent db + if (!clear_client_persistent_db(candidate_for_removal_mac)) { + LOG(ERROR) << "failed to clear client persistent data and remove it from " + "persistent db for client " + << candidate_for_removal_mac << ", unable to add client " << client_mac; + continue; + } + + // The candidate client which is removed was previously counted as added-no-error. + // Decrease the related counter (which will be increased back as part of the new client add). + --clients_added_no_error; + } + // Add client node if (!add_new_client_with_persistent_data_to_nodes_list(client_mac, client_data_map)) { LOG(ERROR) << "Failed to add client node with persistent data for client " << client_mac @@ -2909,6 +2968,10 @@ bool db::load_persistent_db_clients() << "Failed to add nodes for " << add_node_error_count << " clients"; LOG_IF(set_node_error_count, ERROR) << "Failed to set nodes with values from persistent db for " << set_node_error_count << " clients"; + LOG_IF(clients_not_added_or_removed_due_to_full_db, DEBUG) + << "Filtered clients due to max DB capacity reached: " + << clients_not_added_or_removed_due_to_full_db + << ", max-capacity: " << config.clients_persistent_db_max_size; LOG(DEBUG) << "Added " << clients_added_no_error << " clients successfully"; // Set clients count to number of clients added successfully to runtime db From f405802c248dd1e87cfeecc56a5cf341a8aff117 Mon Sep 17 00:00:00 2001 From: Adam Dov Date: Wed, 22 Jul 2020 12:43:26 +0000 Subject: [PATCH 13/19] controller: db: add API to get all configured clients Preparative commit. Added API to get all the clients in the runtime DB that have persistent information configured. The assumption is that a client with persistent data configured must have a timestamp configured (which differs from the default value that the not-configured clients have). The API returns a dqueu of the MAC addresses of the above-mentioned clients. PPM-5. Signed-off-by: Adam Dov --- controller/src/beerocks/master/db/db.cpp | 18 ++++++++++++++++++ controller/src/beerocks/master/db/db.h | 7 +++++++ 2 files changed, 25 insertions(+) diff --git a/controller/src/beerocks/master/db/db.cpp b/controller/src/beerocks/master/db/db.cpp index 1625c35814..5b6ca649c1 100644 --- a/controller/src/beerocks/master/db/db.cpp +++ b/controller/src/beerocks/master/db/db.cpp @@ -2980,6 +2980,24 @@ bool db::load_persistent_db_clients() return true; } +std::deque db::get_clients_with_persistent_data_configured() +{ + std::deque configured_clients; + for (auto node_map : nodes) { + for (auto kv : node_map) { + if ((kv.second->get_type() == eType::TYPE_CLIENT) && (kv.second->mac == kv.first) && + (kv.second->client_parameters_last_edit != + std::chrono::steady_clock::time_point::min())) { + configured_clients.push_back(tlvf::mac_from_string(kv.first)); + } + } + } + + LOG_IF(configured_clients.empty(), DEBUG) << "No clients are found"; + + return configured_clients; +} + // // CLI // diff --git a/controller/src/beerocks/master/db/db.h b/controller/src/beerocks/master/db/db.h index 306e0cd793..6c76ae5087 100644 --- a/controller/src/beerocks/master/db/db.h +++ b/controller/src/beerocks/master/db/db.h @@ -791,6 +791,13 @@ class db { */ bool load_persistent_db_clients(); + /** + * @brief Get the clients with persistent data configured object + * + * @return std::deque containing mac addresses of clients with configured persistent data + */ + std::deque get_clients_with_persistent_data_configured(); + // // CLI // From ad49b868a0451706a52ea0b09bd42d4f607ac87c Mon Sep 17 00:00:00 2001 From: Adam Dov Date: Sun, 26 Jul 2020 07:30:28 +0000 Subject: [PATCH 14/19] controller: read list of clients from DB Changed the ACTION_BML_CLIENT_GET_CLIENT_LIST_REQUEST handling to read the clients from persistent DB using the get_clients_with_persistent_data_configured() DB API. Removed the related cpp-check issue from cppcheck_existing_issues.txt. PPM-5. Signed-off-by: Adam Dov --- ci/cppcheck/cppcheck_existing_issues.txt | 1 - controller/src/beerocks/master/son_management.cpp | 5 ++--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/ci/cppcheck/cppcheck_existing_issues.txt b/ci/cppcheck/cppcheck_existing_issues.txt index 499e1a4341..9546116589 100644 --- a/ci/cppcheck/cppcheck_existing_issues.txt +++ b/ci/cppcheck/cppcheck_existing_issues.txt @@ -96,7 +96,6 @@ controller/src/beerocks/master/son_actions.cpp: performance: Function parameter controller/src/beerocks/master/son_actions.cpp: performance: Function parameter 'sta_mac' should be passed by const reference. [passedByValue] std::string sta_mac, std::string bssid) controller/src/beerocks/master/son_actions.cpp: style: Local variable 'prev_task_id' shadows outer variable [shadowVariable] int prev_task_id = database.get_association_handling_task_id(mac); controller/src/beerocks/master/son_actions.cpp: style: The scope of the variable 'prev_task_id' can be reduced. [variableScope] int prev_task_id; -controller/src/beerocks/master/son_management.cpp: style: Condition '!client_list.size()' is always true [knownConditionTrueFalse] if (!client_list.size()) { controller/src/beerocks/master/son_management.cpp: style: Variable 'op_error_code' is assigned a value that is never used. [unreadVariable] op_error_code = eChannelScanOperationCode::SCAN_IN_PROGRESS; controller/src/beerocks/master/son_management.cpp: style: Variable 'op_error_code' is assigned a value that is never used. [unreadVariable] auto op_error_code = eChannelScanOperationCode::SUCCESS; controller/src/beerocks/master/son_management.cpp: style: Variable 'result_status' is assigned a value that is never used. [unreadVariable] result_status = last_scan_success; diff --git a/controller/src/beerocks/master/son_management.cpp b/controller/src/beerocks/master/son_management.cpp index b7cd6de536..98a550649e 100644 --- a/controller/src/beerocks/master/son_management.cpp +++ b/controller/src/beerocks/master/son_management.cpp @@ -2060,9 +2060,8 @@ void son_management::handle_bml_message(Socket *sd, } }; - // TODO: replace with a list of configured clients read from controller-db - std::vector client_list; - if (!client_list.size()) { + auto client_list = database.get_clients_with_persistent_data_configured(); + if (client_list.empty()) { LOG(DEBUG) << "client list is empty!"; // Send a valid response with an empty list send_response(true); From ef222a75fd93c4a881a001891e56f750fb0f0a8b Mon Sep 17 00:00:00 2001 From: Adam Dov Date: Sun, 26 Jul 2020 12:50:23 +0000 Subject: [PATCH 15/19] controller: read client information from DB Change the ACTION_BML_CLIENT_GET_CLIENT_REQUEST handling to read the client information from the DB using the DB APIs. The handling also takes care of the following scenarios: "client does not exist in DB" - handled as an error. "client timestamp not exist" - handled as an error. PPM-5. Signed-off-by: Adam Dov --- .../src/beerocks/master/son_management.cpp | 52 ++++++++++++++++--- 1 file changed, 44 insertions(+), 8 deletions(-) diff --git a/controller/src/beerocks/master/son_management.cpp b/controller/src/beerocks/master/son_management.cpp index 98a550649e..56ec8f6a80 100644 --- a/controller/src/beerocks/master/son_management.cpp +++ b/controller/src/beerocks/master/son_management.cpp @@ -2134,16 +2134,52 @@ void son_management::handle_bml_message(Socket *sd, break; } - //TODO: fill client information from controller DB - response->client().sta_mac = request->sta_mac(); - response->client().timestamp_sec = 0; - response->client().stay_on_initial_radio = PARAMETER_NOT_CONFIGURED; + auto client_mac = request->sta_mac(); + if (!database.has_node(client_mac)) { + LOG(DEBUG) << "Requested client " << client_mac << " is not listed in the DB"; + response->result() = 1; //Fail. + message_com::send_cmdu(sd, cmdu_tx); + break; + } + + // A configured client must have a valid timestamp configured + auto client_timestamp = database.get_client_parameters_last_edit(client_mac); + if (client_timestamp == std::chrono::steady_clock::time_point::min()) { + LOG(DEBUG) << "Requested client " << client_mac + << " doesn't have a valid timestamp listed in the DB"; + response->result() = 1; //Fail. + message_com::send_cmdu(sd, cmdu_tx); + break; + } + + // Client mac + response->client().sta_mac = client_mac; + // Timestamp + response->client().timestamp_sec = client_timestamp.time_since_epoch().count(); + // Stay on initial radio + response->client().stay_on_initial_radio = + int(database.get_client_stay_on_initial_radio(client_mac)); + // Selected bands + auto selected_band_enable = database.get_client_stay_on_selected_band(client_mac); + if (selected_band_enable == eTriStateBool::NOT_CONFIGURED) { + response->client().selected_bands = PARAMETER_NOT_CONFIGURED; + } else { + response->client().selected_bands = database.get_client_selected_bands(client_mac); + } + // Timelife Delay - scaled from seconds to days + auto timelife_delay_sec = database.get_client_time_life_delay(client_mac); + response->client().time_life_delay_days = + (timelife_delay_sec == std::chrono::seconds::zero()) + ? PARAMETER_NOT_CONFIGURED + : std::chrono::duration_cast(timelife_delay_sec).count() / 24; + + // Currently not supported in DB + // Stay on selected device response->client().stay_on_selected_device = PARAMETER_NOT_CONFIGURED; - response->client().selected_bands = eClientSelectedBands::eSelectedBands_Disabled; - response->client().single_band = PARAMETER_NOT_CONFIGURED; - response->client().time_life_delay_days = PARAMETER_NOT_CONFIGURED; - response->result() = 0; //Success. + // Does client support only single band + response->client().single_band = PARAMETER_NOT_CONFIGURED; + response->result() = 0; //Success. message_com::send_cmdu(sd, cmdu_tx); break; } From 19796545df53538adcaeb162021c9566407e51ca Mon Sep 17 00:00:00 2001 From: Adam Dov Date: Tue, 28 Jul 2020 07:49:28 +0000 Subject: [PATCH 16/19] controller: set client information to DB Change the ACTION_BML_CLIENT_SET_CLIENT_REQUEST handling to configure the client information to the runtime DB using the DB APIs. If persistent DB is enabled - the update_client_persistent_db() API called to update the persistent DB with the client information (to prevent multiple persistent DB access overhead). Note that if the client is not yet part of the runtime DB, a node for it added before-hand. Moreover, managing the persistent DB max size limit is handled internally and is transparent to the user. PPM-5. Signed-off-by: Adam Dov --- .../src/beerocks/master/son_management.cpp | 96 ++++++++++++++++++- 1 file changed, 93 insertions(+), 3 deletions(-) diff --git a/controller/src/beerocks/master/son_management.cpp b/controller/src/beerocks/master/son_management.cpp index 56ec8f6a80..360471ca8f 100644 --- a/controller/src/beerocks/master/son_management.cpp +++ b/controller/src/beerocks/master/son_management.cpp @@ -2110,10 +2110,100 @@ void son_management::handle_bml_message(Socket *sd, break; } - //TODO: add client to persistent DB if required and set client parameters - response->result() = 0; //Success. + auto send_response = [&](bool result) -> bool { + response->result() = (result) ? 0 : 1; + return message_com::send_cmdu(sd, cmdu_tx); + }; - message_com::send_cmdu(sd, cmdu_tx); + // If none of the parameters is configured - return error. + // This flow should be blocked by the BML interface and should not be reached. + if ((request->client_config().stay_on_initial_radio == PARAMETER_NOT_CONFIGURED) && + (request->client_config().stay_on_selected_device == PARAMETER_NOT_CONFIGURED) && + (request->client_config().selected_bands == PARAMETER_NOT_CONFIGURED)) { + LOG(ERROR) + << "Received ACTION_BML_CLIENT_SET_CLIENT request without parameters to configure"; + send_response(false); + break; + } + + // Get client mac. + auto client_mac = request->sta_mac(); + + // If client doesn't have node in runtime DB - add node to runtime DB. + if (!database.has_node(client_mac)) { + LOG(DEBUG) << "Setting a client which doesn't exist in DB, adding client to DB"; + if (!database.add_node(client_mac)) { + LOG(ERROR) << "Failed to add client node for client " << client_mac; + send_response(false); + break; + } + } + + // Set stay_on_initial_radio if requested. + if (request->client_config().stay_on_initial_radio != PARAMETER_NOT_CONFIGURED) { + auto stay_on_initial_radio = + (eTriStateBool(request->client_config().stay_on_initial_radio) == + eTriStateBool::ENABLE); + if (!database.set_client_stay_on_initial_radio(client_mac, stay_on_initial_radio, + false)) { + LOG(ERROR) << " Failed to set stay-on-initial-radio to " << stay_on_initial_radio + << " for client " << client_mac; + send_response(false); + break; + } + } + + // Set stay_on_selected_device if requested. + if (request->client_config().stay_on_selected_device != PARAMETER_NOT_CONFIGURED) { + LOG(DEBUG) + << "The stay-on-selected-device configuration is not yet supported in the DB"; + + // TODO: add stay_on_selected_device support in the persistent DB. + // auto stay_on_selected_device = + // (eTriStateBool(request->client_config().stay_on_selected_device) == + // eTriStateBool::ENABLE); + // if (!database.set_client_stay_on_selected_device(client_mac, stay_on_selected_device, + // false)) { + // LOG(ERROR) << " Failed to set stay-on-selected-device to " + // << stay_on_selected_device << " for client " << client_mac; + // send_response(false); + // break; + // } + } + + // Set stay_on_selected_bands and selected_bands if requested. + if (request->client_config().selected_bands != PARAMETER_NOT_CONFIGURED) { + auto selected_bands = eClientSelectedBands(request->client_config().selected_bands); + auto stay_on_selected_band = + (selected_bands != eClientSelectedBands::eSelectedBands_Disabled); + // Set stay_on_selected_bands. + if (!database.set_client_stay_on_selected_band(client_mac, stay_on_selected_band, + false)) { + LOG(ERROR) << " Failed to stay_on_selected_band to " << stay_on_selected_band + << " for client " << client_mac; + send_response(false); + break; + } + // Set selected_bands. + if (!database.set_client_selected_bands(client_mac, selected_bands, false)) { + LOG(ERROR) << " Failed to set selected-bands to " << selected_bands + << " for client " << client_mac; + send_response(false); + break; + } + } + + //if persistent_db is enabled, call the "update_client_persistent_db" + if (database.config.persistent_db) { + if (!database.update_client_persistent_db(client_mac)) { + LOG(ERROR) + << "Information is saved to runtime-DB but failed to set to persistent DB"; + send_response(false); + break; + } + } + + send_response(true); break; } case beerocks_message::ACTION_BML_CLIENT_GET_CLIENT_REQUEST: { From bafa6586528b2bc73c22debf45683b1953b0e11a Mon Sep 17 00:00:00 2001 From: Adam Dov Date: Tue, 28 Jul 2020 15:48:45 +0000 Subject: [PATCH 17/19] db: set the initial-radio when stay-on-initial-radio is set When configuring the stay_on_initial_radio the initial_radio needs to be set as well. If stay_on_initial_radio is disabled - clear the initial_radio configuration. If stay_on_initial_radio is enabled and the client is connected - set the initial_radio to the parent radio (not bssid) mac. PPM-5. Signed-off-by: Adam Dov --- controller/src/beerocks/master/db/db.cpp | 31 +++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/controller/src/beerocks/master/db/db.cpp b/controller/src/beerocks/master/db/db.cpp index 5b6ca649c1..d17d3fbcf0 100644 --- a/controller/src/beerocks/master/db/db.cpp +++ b/controller/src/beerocks/master/db/db.cpp @@ -2498,6 +2498,8 @@ bool db::set_client_stay_on_initial_radio(const sMacAddr &mac, bool stay_on_init LOG(DEBUG) << "stay_on_initial_radio = " << stay_on_initial_radio; + auto is_client_connected = (node->state == STATE_CONNECTED); + auto timestamp = std::chrono::steady_clock::now(); if (save_to_persistent_db) { // if persistent db is disabled @@ -2510,6 +2512,14 @@ bool db::set_client_stay_on_initial_radio(const sMacAddr &mac, bool stay_on_init ValuesMap values_map; values_map[TIMESTAMP_STR] = timestamp_to_string_seconds(timestamp); values_map[INITIAL_RADIO_ENABLE_STR] = std::to_string(stay_on_initial_radio); + // clear initial-radio data on disabling of stay_on_initial_radio + if (!stay_on_initial_radio) { + values_map[INITIAL_RADIO_STR] = std::string(); + } else if (is_client_connected) { + // if enabling stay-on-initial-radio and client is already connected, update the initial_radio as well + auto bssid = node->parent_mac; + values_map[INITIAL_RADIO_STR] = get_node_parent_radio(bssid); + } // update the persistent db if (!update_client_entry_in_persistent_db(mac, values_map)) { @@ -2521,6 +2531,15 @@ bool db::set_client_stay_on_initial_radio(const sMacAddr &mac, bool stay_on_init node->client_stay_on_initial_radio = (stay_on_initial_radio) ? eTriStateBool::ENABLE : eTriStateBool::DISABLE; + // clear initial-radio data on disabling of stay_on_initial_radio + if (!stay_on_initial_radio) { + node->client_initial_radio = network_utils::ZERO_MAC; + // if enabling stay-on-initial-radio and client is already connected, update the initial_radio as well + } else if (is_client_connected) { + auto bssid = node->parent_mac; + auto parent_radio_mac = get_node_parent_radio(bssid); + node->client_initial_radio = tlvf::mac_from_string(parent_radio_mac); + } node->client_parameters_last_edit = timestamp; return true; @@ -4340,9 +4359,19 @@ bool db::set_node_params_from_map(const sMacAddr &mac, const ValuesMap &values_m } else if (param.first == INITIAL_RADIO_ENABLE_STR) { LOG(DEBUG) << "setting node client_stay_on_initial_radio to " << param.second << " for " << mac; - + auto stay_on_initial_radio = (param.second == "1"); node->client_stay_on_initial_radio = (param.second == "1") ? eTriStateBool::ENABLE : eTriStateBool::DISABLE; + + // clear initial-radio data on disabling of stay_on_initial_radio + if (!stay_on_initial_radio) { + node->client_initial_radio = network_utils::ZERO_MAC; + // if enabling stay-on-initial-radio and client is already connected, update the initial_radio as well + } else if (node->state == STATE_CONNECTED) { + auto bssid = node->parent_mac; + auto parent_radio_mac = get_node_parent_radio(bssid); + node->client_initial_radio = tlvf::mac_from_string(parent_radio_mac); + } } else if (param.first == INITIAL_RADIO_STR) { LOG(DEBUG) << "setting node client_initial_radio to " << param.second << " for " << mac; From 4b1952d027c858569dd1131919af89080ab45537 Mon Sep 17 00:00:00 2001 From: Adam Dov Date: Wed, 29 Jul 2020 07:19:42 +0000 Subject: [PATCH 18/19] controller: db: set the initial-radio on client connection The stay-on-initial-radio can be configured for disconnected clients as well as clients which never connected yet. This means, that when the client connects, we need to check if the stay-on-initial-radio is enabled and if so set the initial-radio. Note that the initial-radio is set only if it wasn't set before - to prevent overriding previous configuration. The value set to the initial-radio is the parent radio (not bssid) mac. Added the above-described functionality to the association_handling_task::finalize_new_connection() which is called on client connection. PPM-5. Signed-off-by: Adam Dov --- .../master/tasks/association_handling_task.cpp | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/controller/src/beerocks/master/tasks/association_handling_task.cpp b/controller/src/beerocks/master/tasks/association_handling_task.cpp index cfab15d3b8..d8b6377323 100644 --- a/controller/src/beerocks/master/tasks/association_handling_task.cpp +++ b/controller/src/beerocks/master/tasks/association_handling_task.cpp @@ -372,6 +372,24 @@ void association_handling_task::finalize_new_connection() */ if (!database.get_node_handoff_flag(sta_mac)) { if (database.get_node_type(sta_mac) == beerocks::TYPE_CLIENT) { + // The client's stay-on-initial-radio can be enabled prior to the client connection. + // If this is the case, when the client connects the initial-radio should be configured (if not already configured) + // to allow the functionality of stay-on-initial-radio. + // Note: The initial-radio is persistent configuration and if is already set, the client-connection flow should + // not override the existing configuration. + auto client_mac = tlvf::mac_from_string(sta_mac); + if ((database.get_client_stay_on_initial_radio(client_mac) == eTriStateBool::ENABLE) && + (database.get_client_initial_radio(client_mac) == network_utils::ZERO_MAC)) { + auto bssid = database.get_node_parent(sta_mac); + auto parent_radio_mac = database.get_node_parent_radio(bssid); + // If stay_on_initial_radio is enabled and initial_radio is not set yet, set to parent radio mac (not bssid) + if (!database.set_client_initial_radio(client_mac, + tlvf::mac_from_string(parent_radio_mac), + database.config.persistent_db)) { + LOG(WARNING) << "Failed to set client " << client_mac << " initial radio to " + << parent_radio_mac; + } + } auto new_task = std::make_shared( database, cmdu_tx, tasks, sta_mac, 6000, "handle_completed_connection"); tasks.add_task(new_task); From 9a6a759ae154e2986327d86dc5c6b6699668d3a5 Mon Sep 17 00:00:00 2001 From: Adam Dov Date: Sun, 2 Aug 2020 08:31:44 +0000 Subject: [PATCH 19/19] controller: db: fix function return value on success to true As part of testing the full flows of the persistent Db we encountered an issue where a successful removal of a client entry from the persistent DB resulted in a false-positive error. The cause of this error is that remove_client_entry_and_update_counter() in the db class returned false both for success and for failure. Changed the successful return value of the above-mentioned function from false to true. Signed-off-by: Adam Dov --- controller/src/beerocks/master/db/db.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controller/src/beerocks/master/db/db.cpp b/controller/src/beerocks/master/db/db.cpp index d17d3fbcf0..fc7ba91b44 100644 --- a/controller/src/beerocks/master/db/db.cpp +++ b/controller/src/beerocks/master/db/db.cpp @@ -4415,7 +4415,7 @@ bool db::remove_client_entry_and_update_counter(const std::string &entry_name) } --m_persistent_db_clients_count; - return false; + return true; } bool db::remove_candidate_client()