prplmesh-hostapd: PR 1514 review fixups

this commnit fixes all relevant comments for
#1550
https://jira.prplfoundation.org/browse/PPM-249

Signed-off-by: Ran Regev <ran.regev@devalore.com>

Author: RanRegev

common/beerocks/hostapd/CMakeLists.txt

@@ -37,7 +37,7 @@ set(PRPLMESH_HOSTAPD_TYPE ${PRPLMESH_HOSTAPD_TYPE} CACHE STRING "Which PRPLMESH_
 
 set(
     hostapd_sources
-    configuration.cpp
+    source/configuration.cpp
 )
 
 

…beerocks/hostapd/include/configuration.h …/hostapd/include/hostapd/configuration.hcommon/beerocks/hostapd/include/configuration.h renamed to common/beerocks/hostapd/include/hostapd/configuration.h common/beerocks/hostapd/include/configuration.h renamed to common/beerocks/hostapd/include/hostapd/configuration.h

@@ -211,7 +211,12 @@ class Configuration {
 private:
     std::string m_configuration_file;
 
-    // may be changed because of const functions, therefore mutable
+    // m_ok hoslds the internal state of the configuration
+    // the user may query the success/fail state of the last command
+    // simply by reading the value of this variable.
+    // the access to it is via operator bool()
+    // m_ok itslef may be changed because of a call to
+    // const function, therefore it is mutable
     mutable bool m_ok = false;
 
     // each string is a line in the original configuration file
@@ -261,8 +266,8 @@ template <class func, class iter, class ap_predicate>
 void Configuration::for_all_ap_vaps(func f, iter current_iter, const iter end, ap_predicate pred)
 {
     for_each(m_hostapd_config_vaps.begin(), m_hostapd_config_vaps.end(),
-             [this, &f, &current_iter,
-              &end, &pred](const std::pair<std::string, std::vector<std::string>> &vap) {
+             [this, &f, &current_iter, &end,
+              &pred](const std::pair<std::string, std::vector<std::string>> &vap) {
                  if (pred(vap.first)) {
                      if (end == current_iter) {
                          f(vap.first, end);

common/beerocks/hostapd/configuration.cpp …eerocks/hostapd/source/configuration.cppcommon/beerocks/hostapd/configuration.cpp renamed to common/beerocks/hostapd/source/configuration.cpp common/beerocks/hostapd/configuration.cpp renamed to common/beerocks/hostapd/source/configuration.cpp

@@ -6,10 +6,10 @@
  * See LICENSE file for more details.
  */
 
-#include "configuration.h"
 #include <algorithm>
 #include <easylogging++.h>
 #include <fstream>
+#include <hostapd/configuration.h>
 
 namespace prplmesh {
 namespace hostapd {
@@ -20,9 +20,9 @@ Configuration::operator bool() const { return m_ok; }
 
 bool Configuration::load(const std::string &vap_indication)
 {
-    // please take a look at the end of this file for
-    // the expected format of hostapd configuration file
-    // loading the file relays on the expected format
+    // please take a look at README.md (common/beerocks/hostapd/README.md) for
+    // the expected format of hostapd configuration file.
+    // loading the file relies on the expected format
     // otherwise the load fails
 
     // for cases when load is called more than once, we
@@ -317,74 +317,29 @@ bool Configuration::is_key_in_line(const std::string &line, const std::string &k
     return ret;
 }
 
-std::ostream &operator<<(std::ostream &o, const Configuration &conf)
+std::ostream &operator<<(std::ostream &os, const Configuration &conf)
 {
-    o << "== configuration details ==\n"
-      << "= ok:           " << std::boolalpha << conf.m_ok << '\n'
-      << "= last message: " << conf.m_last_message << '\n'
-      << "= file:         " << conf.m_configuration_file << '\n'
-      << "= head:         " << '\n';
+    os << "== configuration details ==\n"
+       << "= ok:           " << std::boolalpha << conf.m_ok << '\n'
+       << "= last message: " << conf.m_last_message << '\n'
+       << "= file:         " << conf.m_configuration_file << '\n'
+       << "= head:         " << '\n';
 
     for (const auto &line : conf.m_hostapd_config_head) {
-        o << line << '\n';
+        os << line << '\n';
     }
 
-    o << "== vaps (total of: " << conf.m_hostapd_config_vaps.size() << " vaps) ==\n";
+    os << "== vaps (total of: " << conf.m_hostapd_config_vaps.size() << " vaps) ==\n";
 
     for (const auto &vap : conf.m_hostapd_config_vaps) {
-        o << "   vap: " << vap.first << "\n";
+        os << "   vap: " << vap.first << "\n";
         for (const auto &line : vap.second) {
-            o << line << '\n';
+            os << line << '\n';
         }
     }
 
-    return o;
+    return os;
 }
 
 } // namespace hostapd
 } // namespace prplmesh
-
-/*
-//////////////// hostapd configuration format ////////////////
-
-hostapd has a special format that does NOT have an ini like format.
-between /// BEGIN hostapd.conf /// and /// END hostapd.conf /// is the 
-format of the file.
-note: the string "bss=" may be replaced by the
-user in the call to load() with another vap-indicator (e.g. "interface=")
-
-/// BEGIN hostapd.conf ///  
-
-## "head" part ##
-
-# everything until the first `bss=` in the file
-# is considered "head"
-# after the first `bss=` "vaps" are presented.
-# so we expect no parametrs that does not belong to vaps
-# after the first `bss=`
-# take a look below for more details
-
-# note that we don't expect a space between the key and the equal sign
-# the code in this class seeks for `key=` when a key is needed.
-# therefore `key =` (with space) will fail
-# also, everything immidiatly after the equal sign (=) is 
-# part of the value. the space before 11 in this is part of the value:
-# bassid= 11:22:33:44:55:66
-key=value 
-
-## "vaps part - we expect at least one vap to be configured ##
-
-# vap (bss and vap are interchangable) 
-bss=wlan0_0
-
-# this key (ssid) belongs to the previous vap (bss value which is wlan0_0)
-ssid=test2
-
-# another vap 
-bss=wlan0_1
-
-# this key (bssid) belongs to the previous vap wlan0_1
-bssid=00:13:10:95:fe:0b
-
-///// END hostapd.conf ///
-*/

common/beerocks/hostapd/unit_tests/configuration_test.cpp

@@ -6,13 +6,14 @@
  * See LICENSE file for more details.
  */
 
-#include "configuration.h"
 #include <fstream>
+#include <hostapd/configuration.h>
 
 #include <gtest/gtest.h>
 
 namespace {
 
+//const std::string vap_indication("bss=");
 const std::string vap_indication("interface=");
 const std::string configuration_path("/tmp/");
 const std::string configuration_file_name("omnia.conf");
@@ -56,9 +57,10 @@ const std::string configuration_content(
     "wpa=0\n"
     "ssid=dummy_ssid_0\n"
     "bridge=br-lan\n"
-    "bssid=02:9A:96:FB:59:0F\n"
+    "bssid=02:9A:96:FB:59:0F\n" +
 
-    "bss=wlan0.0\n"
+    vap_indication +
+    "wlan0.0\n"
     "ctrl_interface=/var/run/hostapd\n"
     "ap_isolate=1\n"
     "ap_max_inactivity=60\n"
@@ -100,9 +102,10 @@ const std::string configuration_content(
     "wps_state=2\n"
     "mesh_mode=fAP\n"
     "multi_ap_backhaul_ssid=\"Multi-AP-24G-2\"\n"
-    "multi_ap_backhaul_wpa_passphrase=maprocks2\n"
+    "multi_ap_backhaul_wpa_passphrase=maprocks2\n" +
 
-    "bss=wlan0.1\n"
+    vap_indication +
+    "wlan0.1\n"
     "mode=non-ap\n"
     "ctrl_interface=/var/run/hostapd\n"
     "ap_isolate=1\n"
@@ -134,7 +137,9 @@ const std::string configuration_content(
     "sFourAddrMode=1\n"
     "max_num_sta=1\n"
 
-    "#bss=wlan0.2\n"
+    "#" +
+    vap_indication +
+    "wlan0.2\n"
     "#ctrl_interface=/var/run/hostapd\n"
     "#ap_isolate=1\n"
     "#ap_max_inactivity=60\n"
@@ -154,9 +159,10 @@ const std::string configuration_content(
     "#wpa=0\n"
     "#bridge=br-lan\n"
     "#bssid=02:9A:96:FB:59:13\n"
-    "#start_disabled=1\n"
+    "#start_disabled=1\n" +
 
-    "bss=wlan0.3\n"
+    vap_indication +
+    "wlan0.3\n"
     "ctrl_interface=/var/run/hostapd\n"
     "ap_isolate=1\n"
     "ap_max_inactivity=60\n"
@@ -178,16 +184,13 @@ const std::string configuration_content(
     "start_disabled=1\n"
     "mode=ap\n");
 
-
-/*
 void clean_start()
 {
     // save the content of the string (start clean)
     std::ofstream tmp(configuration_path + configuration_file_name);
     tmp << configuration_content;
     tmp.flush();
 }
-*/
 
 TEST(configuration_test, load)
 {
@@ -228,7 +231,7 @@ TEST(configuration_test, store)
     //// end prerequsite ////
 
     // add a value to vap
-    conf.set_create_vap_value("wlan1", "was_i_stroed", "yes_you_were");
+    conf.set_create_vap_value("wlan0.1", "was_i_stroed", "yes_you_were");
     EXPECT_TRUE(conf) << conf;
 
     // store
@@ -315,8 +318,8 @@ TEST(configuration_test, get_head_values)
     //// end prerequsite ////
 
     // get existing key
-    auto val = conf.get_head_value("ctrl_interface");
-    EXPECT_EQ(val, "/var/run/hostapd");
+    auto val = conf.get_head_value("acs_num_scans");
+    EXPECT_EQ(val, "1");
 
     // get non existing key
     val = conf.get_head_value("out_of_office");
@@ -332,21 +335,23 @@ TEST(configuration_test, set_string_vap_values)
     //// start prerequsite ////
 
     // save the content of the string (start clean)
-    //   // //clean_start();
+    clean_start();
 
     // construct a configuration
     prplmesh::hostapd::Configuration conf(configuration_path + configuration_file_name);
     ASSERT_FALSE(conf) << conf;
 
     // load the dummy configuration file
     conf.load(vap_indication);
-    ;
     ASSERT_TRUE(conf) << conf;
 
     //// end prerequsite ////
 
     //// start test ////
 
+    conf.set_create_vap_value("wlan0", "ssid", "ran_home");
+    EXPECT_TRUE(conf) << conf;
+
     // replace existing value for existing key for existing vap
     conf.set_create_vap_value("wlan0.2", "disassoc_low_ack", "734");
     EXPECT_TRUE(conf) << conf;