C++ string parser for wpa_supplicant
Clash Royale CLAN TAG#URR8PPP
.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;
up vote
3
down vote
favorite
I have created a naive string parser for unsolicited wpa_supplicant messages. I could potentially extend this in the future. I am not sure if this is the best way to parse these kinds of messages. I would prefer something resembling a switch statement but I am not sure I could do that without increasing code complexity. I would like a more scalable solution though, in case I need to look for many different message types in the future.
Code:
#include <stdio.h>
#include <string>
#include <map>
#define DEBUG_PRINT printf
class wpa_monitor
public:
void handle_wpa_message( const std::string& message );
private:
std::map< std::string, std::string > mac_ssid_association_map;
;
void wpa_monitor::handle_wpa_message( const std::string& message )
DEBUG_PRINT( "WPA event: '%s'n", message.c_str() );
size_t pos;
pos = message.find( "Trying to associate with " );
if( pos != std::string::npos )
std::string mac_substring = message.substr( pos + sizeof( "Trying to associate with " ) - 1 );
std::string mac_addr = mac_substring.substr( 0, mac_substring.find( " " ) );
if( mac_addr.empty() )
return;
pos = message.find( "SSID='" );
if( pos == std::string::npos )
return;
std::string ssid_substring = message.substr( pos + sizeof( "SSID='" ) - 1 );
std::string ssid = ssid_substring.substr( 0, ssid_substring.find( "'" ) );
DEBUG_PRINT( "Associating mac_address '%s' with ssid '%s'n", mac_addr.c_str(), ssid.c_str() );
mac_ssid_association_map.insert( std::make_pair< std::string, std::string >( mac_addr, ssid ) );
return;
pos = message.find( "CTRL-EVENT-CONNECTED - Connection to " );
if( pos != std::string::npos )
std::string mac_substring = message.substr( pos + sizeof( "CTRL-EVENT-CONNECTED - Connection to " ) - 1 );
std::string mac_addr = mac_substring.substr( 0, mac_substring.find( " " ) );
if( mac_addr.empty() )
return;
std::map< std::string, std::string >::iterator it = mac_ssid_association_map.find( mac_addr );
if( it != mac_ssid_association_map.end() )
DEBUG_PRINT( "We are now connected to ssid '%s'n", it->second.c_str() );
return;
pos = message.find( "CTRL-EVENT-SCAN-RESULTS" );
if( pos != std::string::npos )
//scan results ready
DEBUG_PRINT( "Scan results are readyn" );
return;
Example Output (personal data obfuscated):
WPA event: '<3>CTRL-EVENT-SCAN-STARTED '
WPA event: '<3>CTRL-EVENT-BSS-ADDED 0 00:00:00:00:00:00'
WPA event: '<3>CTRL-EVENT-BSS-ADDED 1 00:00:00:00:00:01'
WPA event: '<3>CTRL-EVENT-BSS-ADDED 2 00:00:00:00:00:02'
WPA event: '<3>CTRL-EVENT-SCAN-RESULTS '
Scan results are ready
WPA event: '<3>WPS-AP-AVAILABLE '
WPA event: '<3>Trying to associate with 00:00:00:00:00:00 (SSID='my_ssid' freq=5745 MHz)'
Associating mac_address '00:00:00:00:00:00' with ssid 'my_ssid'
WPA event: '<3>Associated with 00:00:00:00:00:00'
WPA event: '<3>CTRL-EVENT-SUBNET-STATUS-UPDATE status=0'
WPA event: '<3>WPA: Key negotiation completed with 00:00:00:00:00:00 [PTK=CCMP GTK=TKIP]'
WPA event: '<3>CTRL-EVENT-CONNECTED - Connection to 00:00:00:00:00:00 completed [id=1 id_str=]'
We are now connected to ssid 'my_ssid'
c++ strings parsing
add a comment |Â
up vote
3
down vote
favorite
I have created a naive string parser for unsolicited wpa_supplicant messages. I could potentially extend this in the future. I am not sure if this is the best way to parse these kinds of messages. I would prefer something resembling a switch statement but I am not sure I could do that without increasing code complexity. I would like a more scalable solution though, in case I need to look for many different message types in the future.
Code:
#include <stdio.h>
#include <string>
#include <map>
#define DEBUG_PRINT printf
class wpa_monitor
public:
void handle_wpa_message( const std::string& message );
private:
std::map< std::string, std::string > mac_ssid_association_map;
;
void wpa_monitor::handle_wpa_message( const std::string& message )
DEBUG_PRINT( "WPA event: '%s'n", message.c_str() );
size_t pos;
pos = message.find( "Trying to associate with " );
if( pos != std::string::npos )
std::string mac_substring = message.substr( pos + sizeof( "Trying to associate with " ) - 1 );
std::string mac_addr = mac_substring.substr( 0, mac_substring.find( " " ) );
if( mac_addr.empty() )
return;
pos = message.find( "SSID='" );
if( pos == std::string::npos )
return;
std::string ssid_substring = message.substr( pos + sizeof( "SSID='" ) - 1 );
std::string ssid = ssid_substring.substr( 0, ssid_substring.find( "'" ) );
DEBUG_PRINT( "Associating mac_address '%s' with ssid '%s'n", mac_addr.c_str(), ssid.c_str() );
mac_ssid_association_map.insert( std::make_pair< std::string, std::string >( mac_addr, ssid ) );
return;
pos = message.find( "CTRL-EVENT-CONNECTED - Connection to " );
if( pos != std::string::npos )
std::string mac_substring = message.substr( pos + sizeof( "CTRL-EVENT-CONNECTED - Connection to " ) - 1 );
std::string mac_addr = mac_substring.substr( 0, mac_substring.find( " " ) );
if( mac_addr.empty() )
return;
std::map< std::string, std::string >::iterator it = mac_ssid_association_map.find( mac_addr );
if( it != mac_ssid_association_map.end() )
DEBUG_PRINT( "We are now connected to ssid '%s'n", it->second.c_str() );
return;
pos = message.find( "CTRL-EVENT-SCAN-RESULTS" );
if( pos != std::string::npos )
//scan results ready
DEBUG_PRINT( "Scan results are readyn" );
return;
Example Output (personal data obfuscated):
WPA event: '<3>CTRL-EVENT-SCAN-STARTED '
WPA event: '<3>CTRL-EVENT-BSS-ADDED 0 00:00:00:00:00:00'
WPA event: '<3>CTRL-EVENT-BSS-ADDED 1 00:00:00:00:00:01'
WPA event: '<3>CTRL-EVENT-BSS-ADDED 2 00:00:00:00:00:02'
WPA event: '<3>CTRL-EVENT-SCAN-RESULTS '
Scan results are ready
WPA event: '<3>WPS-AP-AVAILABLE '
WPA event: '<3>Trying to associate with 00:00:00:00:00:00 (SSID='my_ssid' freq=5745 MHz)'
Associating mac_address '00:00:00:00:00:00' with ssid 'my_ssid'
WPA event: '<3>Associated with 00:00:00:00:00:00'
WPA event: '<3>CTRL-EVENT-SUBNET-STATUS-UPDATE status=0'
WPA event: '<3>WPA: Key negotiation completed with 00:00:00:00:00:00 [PTK=CCMP GTK=TKIP]'
WPA event: '<3>CTRL-EVENT-CONNECTED - Connection to 00:00:00:00:00:00 completed [id=1 id_str=]'
We are now connected to ssid 'my_ssid'
c++ strings parsing
1
You'll receive better reviews if you show a complete example. For example, I recommend that you edit to show the definition ofwpa_monitor
and/or the necessary#include
lines, and amain()
that shows how to call your function. It can really help reviewers if they are able to compile and run your program. A sample (anonymised) input would also help, if you can provide that.
â Toby Speight
Feb 8 at 8:26
1
@TobySpeight I have edited it a bit more to show some of the dependencies along with a simplified class definition. Not too much more i can show without violating NDA in this case sadly (not to mention hooking this up to wpa_supplicant requires a lot of code which would not be related to my question). Perhaps next time I will take the time to make something synthetic.
â Nathan Owen
Feb 8 at 22:11
That's great - thanks for improving the question!
â Toby Speight
Feb 9 at 9:36
add a comment |Â
up vote
3
down vote
favorite
up vote
3
down vote
favorite
I have created a naive string parser for unsolicited wpa_supplicant messages. I could potentially extend this in the future. I am not sure if this is the best way to parse these kinds of messages. I would prefer something resembling a switch statement but I am not sure I could do that without increasing code complexity. I would like a more scalable solution though, in case I need to look for many different message types in the future.
Code:
#include <stdio.h>
#include <string>
#include <map>
#define DEBUG_PRINT printf
class wpa_monitor
public:
void handle_wpa_message( const std::string& message );
private:
std::map< std::string, std::string > mac_ssid_association_map;
;
void wpa_monitor::handle_wpa_message( const std::string& message )
DEBUG_PRINT( "WPA event: '%s'n", message.c_str() );
size_t pos;
pos = message.find( "Trying to associate with " );
if( pos != std::string::npos )
std::string mac_substring = message.substr( pos + sizeof( "Trying to associate with " ) - 1 );
std::string mac_addr = mac_substring.substr( 0, mac_substring.find( " " ) );
if( mac_addr.empty() )
return;
pos = message.find( "SSID='" );
if( pos == std::string::npos )
return;
std::string ssid_substring = message.substr( pos + sizeof( "SSID='" ) - 1 );
std::string ssid = ssid_substring.substr( 0, ssid_substring.find( "'" ) );
DEBUG_PRINT( "Associating mac_address '%s' with ssid '%s'n", mac_addr.c_str(), ssid.c_str() );
mac_ssid_association_map.insert( std::make_pair< std::string, std::string >( mac_addr, ssid ) );
return;
pos = message.find( "CTRL-EVENT-CONNECTED - Connection to " );
if( pos != std::string::npos )
std::string mac_substring = message.substr( pos + sizeof( "CTRL-EVENT-CONNECTED - Connection to " ) - 1 );
std::string mac_addr = mac_substring.substr( 0, mac_substring.find( " " ) );
if( mac_addr.empty() )
return;
std::map< std::string, std::string >::iterator it = mac_ssid_association_map.find( mac_addr );
if( it != mac_ssid_association_map.end() )
DEBUG_PRINT( "We are now connected to ssid '%s'n", it->second.c_str() );
return;
pos = message.find( "CTRL-EVENT-SCAN-RESULTS" );
if( pos != std::string::npos )
//scan results ready
DEBUG_PRINT( "Scan results are readyn" );
return;
Example Output (personal data obfuscated):
WPA event: '<3>CTRL-EVENT-SCAN-STARTED '
WPA event: '<3>CTRL-EVENT-BSS-ADDED 0 00:00:00:00:00:00'
WPA event: '<3>CTRL-EVENT-BSS-ADDED 1 00:00:00:00:00:01'
WPA event: '<3>CTRL-EVENT-BSS-ADDED 2 00:00:00:00:00:02'
WPA event: '<3>CTRL-EVENT-SCAN-RESULTS '
Scan results are ready
WPA event: '<3>WPS-AP-AVAILABLE '
WPA event: '<3>Trying to associate with 00:00:00:00:00:00 (SSID='my_ssid' freq=5745 MHz)'
Associating mac_address '00:00:00:00:00:00' with ssid 'my_ssid'
WPA event: '<3>Associated with 00:00:00:00:00:00'
WPA event: '<3>CTRL-EVENT-SUBNET-STATUS-UPDATE status=0'
WPA event: '<3>WPA: Key negotiation completed with 00:00:00:00:00:00 [PTK=CCMP GTK=TKIP]'
WPA event: '<3>CTRL-EVENT-CONNECTED - Connection to 00:00:00:00:00:00 completed [id=1 id_str=]'
We are now connected to ssid 'my_ssid'
c++ strings parsing
I have created a naive string parser for unsolicited wpa_supplicant messages. I could potentially extend this in the future. I am not sure if this is the best way to parse these kinds of messages. I would prefer something resembling a switch statement but I am not sure I could do that without increasing code complexity. I would like a more scalable solution though, in case I need to look for many different message types in the future.
Code:
#include <stdio.h>
#include <string>
#include <map>
#define DEBUG_PRINT printf
class wpa_monitor
public:
void handle_wpa_message( const std::string& message );
private:
std::map< std::string, std::string > mac_ssid_association_map;
;
void wpa_monitor::handle_wpa_message( const std::string& message )
DEBUG_PRINT( "WPA event: '%s'n", message.c_str() );
size_t pos;
pos = message.find( "Trying to associate with " );
if( pos != std::string::npos )
std::string mac_substring = message.substr( pos + sizeof( "Trying to associate with " ) - 1 );
std::string mac_addr = mac_substring.substr( 0, mac_substring.find( " " ) );
if( mac_addr.empty() )
return;
pos = message.find( "SSID='" );
if( pos == std::string::npos )
return;
std::string ssid_substring = message.substr( pos + sizeof( "SSID='" ) - 1 );
std::string ssid = ssid_substring.substr( 0, ssid_substring.find( "'" ) );
DEBUG_PRINT( "Associating mac_address '%s' with ssid '%s'n", mac_addr.c_str(), ssid.c_str() );
mac_ssid_association_map.insert( std::make_pair< std::string, std::string >( mac_addr, ssid ) );
return;
pos = message.find( "CTRL-EVENT-CONNECTED - Connection to " );
if( pos != std::string::npos )
std::string mac_substring = message.substr( pos + sizeof( "CTRL-EVENT-CONNECTED - Connection to " ) - 1 );
std::string mac_addr = mac_substring.substr( 0, mac_substring.find( " " ) );
if( mac_addr.empty() )
return;
std::map< std::string, std::string >::iterator it = mac_ssid_association_map.find( mac_addr );
if( it != mac_ssid_association_map.end() )
DEBUG_PRINT( "We are now connected to ssid '%s'n", it->second.c_str() );
return;
pos = message.find( "CTRL-EVENT-SCAN-RESULTS" );
if( pos != std::string::npos )
//scan results ready
DEBUG_PRINT( "Scan results are readyn" );
return;
Example Output (personal data obfuscated):
WPA event: '<3>CTRL-EVENT-SCAN-STARTED '
WPA event: '<3>CTRL-EVENT-BSS-ADDED 0 00:00:00:00:00:00'
WPA event: '<3>CTRL-EVENT-BSS-ADDED 1 00:00:00:00:00:01'
WPA event: '<3>CTRL-EVENT-BSS-ADDED 2 00:00:00:00:00:02'
WPA event: '<3>CTRL-EVENT-SCAN-RESULTS '
Scan results are ready
WPA event: '<3>WPS-AP-AVAILABLE '
WPA event: '<3>Trying to associate with 00:00:00:00:00:00 (SSID='my_ssid' freq=5745 MHz)'
Associating mac_address '00:00:00:00:00:00' with ssid 'my_ssid'
WPA event: '<3>Associated with 00:00:00:00:00:00'
WPA event: '<3>CTRL-EVENT-SUBNET-STATUS-UPDATE status=0'
WPA event: '<3>WPA: Key negotiation completed with 00:00:00:00:00:00 [PTK=CCMP GTK=TKIP]'
WPA event: '<3>CTRL-EVENT-CONNECTED - Connection to 00:00:00:00:00:00 completed [id=1 id_str=]'
We are now connected to ssid 'my_ssid'
c++ strings parsing
edited Feb 8 at 22:06
asked Feb 8 at 2:03
Nathan Owen
436
436
1
You'll receive better reviews if you show a complete example. For example, I recommend that you edit to show the definition ofwpa_monitor
and/or the necessary#include
lines, and amain()
that shows how to call your function. It can really help reviewers if they are able to compile and run your program. A sample (anonymised) input would also help, if you can provide that.
â Toby Speight
Feb 8 at 8:26
1
@TobySpeight I have edited it a bit more to show some of the dependencies along with a simplified class definition. Not too much more i can show without violating NDA in this case sadly (not to mention hooking this up to wpa_supplicant requires a lot of code which would not be related to my question). Perhaps next time I will take the time to make something synthetic.
â Nathan Owen
Feb 8 at 22:11
That's great - thanks for improving the question!
â Toby Speight
Feb 9 at 9:36
add a comment |Â
1
You'll receive better reviews if you show a complete example. For example, I recommend that you edit to show the definition ofwpa_monitor
and/or the necessary#include
lines, and amain()
that shows how to call your function. It can really help reviewers if they are able to compile and run your program. A sample (anonymised) input would also help, if you can provide that.
â Toby Speight
Feb 8 at 8:26
1
@TobySpeight I have edited it a bit more to show some of the dependencies along with a simplified class definition. Not too much more i can show without violating NDA in this case sadly (not to mention hooking this up to wpa_supplicant requires a lot of code which would not be related to my question). Perhaps next time I will take the time to make something synthetic.
â Nathan Owen
Feb 8 at 22:11
That's great - thanks for improving the question!
â Toby Speight
Feb 9 at 9:36
1
1
You'll receive better reviews if you show a complete example. For example, I recommend that you edit to show the definition of
wpa_monitor
and/or the necessary #include
lines, and a main()
that shows how to call your function. It can really help reviewers if they are able to compile and run your program. A sample (anonymised) input would also help, if you can provide that.â Toby Speight
Feb 8 at 8:26
You'll receive better reviews if you show a complete example. For example, I recommend that you edit to show the definition of
wpa_monitor
and/or the necessary #include
lines, and a main()
that shows how to call your function. It can really help reviewers if they are able to compile and run your program. A sample (anonymised) input would also help, if you can provide that.â Toby Speight
Feb 8 at 8:26
1
1
@TobySpeight I have edited it a bit more to show some of the dependencies along with a simplified class definition. Not too much more i can show without violating NDA in this case sadly (not to mention hooking this up to wpa_supplicant requires a lot of code which would not be related to my question). Perhaps next time I will take the time to make something synthetic.
â Nathan Owen
Feb 8 at 22:11
@TobySpeight I have edited it a bit more to show some of the dependencies along with a simplified class definition. Not too much more i can show without violating NDA in this case sadly (not to mention hooking this up to wpa_supplicant requires a lot of code which would not be related to my question). Perhaps next time I will take the time to make something synthetic.
â Nathan Owen
Feb 8 at 22:11
That's great - thanks for improving the question!
â Toby Speight
Feb 9 at 9:36
That's great - thanks for improving the question!
â Toby Speight
Feb 9 at 9:36
add a comment |Â
2 Answers
2
active
oldest
votes
up vote
4
down vote
accepted
I'll make no comment on the use of C-style <stdio.h>
, since that's likely an artefact of the surrounding code, rather than something you'd have written yourself.
A missing header and a typo/oversight
The header required for std::make_pair
is <utility>
. Your implementation may have defined it as a side-effect of including <map>
, but it's not portable to rely on that.
size_t pos
ought to be std::size_t pos
.
Test program
I managed to re-create some input from your sample output:
int main()
static auto const messages =
"<3>CTRL-EVENT-SCAN-STARTED ",
"<3>CTRL-EVENT-BSS-ADDED 0 00:00:00:00:00:00",
"<3>CTRL-EVENT-BSS-ADDED 1 00:00:00:00:00:01",
"<3>CTRL-EVENT-BSS-ADDED 2 00:00:00:00:00:02",
"<3>CTRL-EVENT-SCAN-RESULTS ",
"<3>WPS-AP-AVAILABLE ",
"<3>Trying to associate with 00:00:00:00:00:00 (SSID='my_ssid' freq=5745 MHz)",
"<3>Associated with 00:00:00:00:00:00",
"<3>CTRL-EVENT-SUBNET-STATUS-UPDATE status=0",
"<3>WPA: Key negotiation completed with 00:00:00:00:00:00 [PTK=CCMP GTK=TKIP]",
"<3>CTRL-EVENT-CONNECTED - Connection to 00:00:00:00:00:00 completed [id=1 id_str=]",
;
wpa_monitor monitor;
for (auto m: messages)
monitor.handle_wpa_message(m);
Naming
I avoid names that include their type, as it can be confusing if you later change that type. So I'd call the map mac_to_ssid
instead. That also has the benefit of being shorter, so easy to type and to read.
Extensibility
You'll find that a single large method gets harder to navigate as it gets bigger. As hinted in another answer, it makes sense to delegate to a bunch of simple methods that each handle a single case. You could iterate through a vector, but I'd stick to something simple like this:
class wpa_monitor
public:
bool handle_wpa_message(const std::string& message);
private:
std::map<std::string, std::string> mac_ssid_association_map = ;
bool handle_associate(const std::string& message);
bool handle_connected(const std::string& message);
bool handle_scanresult(const std::string& message);
;
bool wpa_monitor::handle_wpa_message(const std::string& message)
handle_connected(message)
Now we can look at each matcher in turn.
Refactor some common code
We repeatedly search for a string and take an interest in its end, like this:
auto pos = message.find("Trying to associate with ");
if (pos == std::string::npos)
return false;
std::string mac_substring = message.substr(pos + sizeof "Trying to associate with " - 1);
Obviously there's scope for error if we don't keep the two copies of the string in agreement. Instead, we can write a small function to do this:
static std::size_t find_end(const std::string& s, const std::string& substr)
auto pos = s.find(substr);
if (pos != std::string::npos)
pos += substr.size();
return pos;
That makes the caller simpler and more robust:
auto pos = find_end(message, "Trying to associate with ");
if (pos == std::string::npos)
return false;
auto const mac_substring = message.substr(pos);
Actually, that's not really the MAC substring, as it continues to the end of the string. We really want another helper, accepting a termination character:
static std::string find_in_delimiters(const std::string& s,
const std::string& open, const char close)
auto start = s.find(open);
if (start == std::string::npos)
return ;
start += open.size();
auto end = s.find(close, start);
if (end == std::string::npos)
return ;
return s.substr(start, end-start);
And we simplify the callers even more:
auto mac_addr = find_in_delimiters(message, "Trying to associate with ", ' ');
if (mac_addr.empty())
return false;
More on make_pair()
I get a failure to compile, as GCC seems to want rvalue references for make_pair<string,string>()
. I could resolve that with a std::move()
like this:
mac_ssid_association_map.insert( std::make_pair< std::string, std::string >( std::move(mac_addr), std::move(ssid) ) );
However, we can let template type deduction do its thing:
mac_ssid_association_map.insert(std::make_pair(std::move(mac_addr), std::move(ssid)));
Or even just:
mac_ssid_association_map.insert(std::move(mac_addr), std::move(ssid));
Use auto
instead of exact iterator types
Here's a place where changing the map type could be a maintenance headache:
std::map< std::string, std::string >::iterator it = mac_ssid_association_map.find( mac_addr );
We can use auto
, which also makes it more readable:
auto it = mac_ssid_association_map.find(mac_addr);
My version
#include <cstdio>
#include <string>
#include <map>
#include <utility>
#define DEBUG_PRINT std::printf
class wpa_monitor
public:
bool handle_wpa_message(const std::string& message);
private:
std::map<std::string, std::string> mac_to_ssid = ;
bool handle_associate(const std::string& message);
bool handle_connected(const std::string& message);
bool handle_scanresult(const std::string& message);
;
// Helper method - returns substring of s contained between
// open and close, or empty if not matched
static std::string find_in_delimiters(const std::string& s,
const std::string& open, const char close)
auto start = s.find(open);
if (start == s.npos)
return ;
start += open.size();
auto end = s.find(close, start);
if (end == s.npos)
return ;
return s.substr(start, end-start);
// Handlers for specific message types
bool wpa_monitor::handle_associate(const std::string& message)
auto mac_addr = find_in_delimiters(message, "Trying to associate with ", ' ');
if (mac_addr.empty())
return false;
auto ssid = find_in_delimiters(message, "SSID='", ''');
if (ssid.empty())
return false;
DEBUG_PRINT("Associating mac_address '%s' with ssid '%s'n", mac_addr.c_str(), ssid.c_str());
mac_to_ssid.insert(std::move(mac_addr), std::move(ssid));
return true;
bool wpa_monitor::handle_connected(const std::string& message)
auto mac_addr = find_in_delimiters(message, "CTRL-EVENT-CONNECTED - Connection to ", ' ');
if (mac_addr.empty())
return false;
auto it = mac_to_ssid.find(mac_addr);
if (it != mac_to_ssid.end())
DEBUG_PRINT("We are now connected to ssid '%s'n", it->second.c_str());
return true;
bool wpa_monitor::handle_scanresult(const std::string& message)
auto pos = message.find("CTRL-EVENT-SCAN-RESULTS");
if (pos == message.npos)
return false;
//scan results ready
DEBUG_PRINT("Scan results are readyn");
return true;
// Test program
int main()
static auto const messages =
"<3>CTRL-EVENT-SCAN-STARTED ",
"<3>CTRL-EVENT-BSS-ADDED 0 00:00:00:00:00:00",
"<3>CTRL-EVENT-BSS-ADDED 1 00:00:00:00:00:01",
"<3>CTRL-EVENT-BSS-ADDED 2 00:00:00:00:00:02",
"<3>CTRL-EVENT-SCAN-RESULTS ",
"<3>WPS-AP-AVAILABLE ",
"<3>Trying to associate with 00:00:00:00:00:00 (SSID='my_ssid' freq=5745 MHz)",
"<3>Associated with 00:00:00:00:00:00",
"<3>CTRL-EVENT-SUBNET-STATUS-UPDATE status=0",
"<3>WPA: Key negotiation completed with 00:00:00:00:00:00 [PTK=CCMP GTK=TKIP]",
"<3>CTRL-EVENT-CONNECTED - Connection to 00:00:00:00:00:00 completed [id=1 id_str=]",
;
wpa_monitor monitor;
for (auto m: messages)
monitor.handle_wpa_message(m);
Thanks for the thorough review and recommendations! I suppose I should have specified that I was using C++98 which may have been the reason for some of the cumbersome/different semantics in my code, however I did not want to limit answers to C++98 as I am learning modern C++ at the moment and pushing to get C++17 support in the next few months (not to mention a C++98 review would not be as useful to those who don't have my constraints). I am working in a heavily C oriented culture at the moment and it is very helpful to get C++ code reviews from the people in the C++ community.
â Nathan Owen
Feb 9 at 20:43
I tend to use C++11 as a baseline these days (on the one project where I'm stuck with C++03, I feel frustration), so much of my answer might not be relevant. OTOH, perhaps it provides you with some ammunition in any argument^W discussion over tooling - I'm believe the modern C++ is much more readable. The Refactor, Extensibility and Naming sections should still be relevant for any language version (and, indeed, other languages).
â Toby Speight
Feb 12 at 9:20
add a comment |Â
up vote
3
down vote
You're right to look for a more scalable implementation. From where I stand, the most simple one would be to associate, in a container, the strings you look for to identify a message and the function you then want to apply to the message. Something like:
/* given:
#include <string>
#include <map>
#include <algorithm>
#include <functional>
*/
void wpa_handler::handle_wpa_message( const std::string& message )
/* given, as class member:
std::map<std::string, std::function<void(const std::string&)>> message_dispatch;
// that you populate somewhere in the construction process:
message_dispatch["Trying to associate with "] = handle_function1;
message_dispatch["CTRL-EVENT-CONNECTED - Connection to "] = handle_function2;
// etc.
*/
auto handler = std::find_if(message_dispatch.begin(), message_dispatch.end(), [&message](auto&& pair)
return message.find(pair.first) != std::string::npos;
);
if (handler == message_dispatch.end())
// handle unknown message recieved -> exception?
else
handler->second(message);
Another, maybe yet simpler option, is to have a vector of your handling functions, and pass the message to each of them. It's then the handling function which is responsible for detecting if it shall handle that message or not. The problem is that you can't know if the message was handled, and by which handler, unless you implement a feedback mechanism.
The feedback mechanism could be as simple asreturn true;
in the handler. Then you'd havefor (auto& h: handlers) if (h.handle(message)) break;
â Toby Speight
Feb 8 at 10:00
add a comment |Â
2 Answers
2
active
oldest
votes
2 Answers
2
active
oldest
votes
active
oldest
votes
active
oldest
votes
up vote
4
down vote
accepted
I'll make no comment on the use of C-style <stdio.h>
, since that's likely an artefact of the surrounding code, rather than something you'd have written yourself.
A missing header and a typo/oversight
The header required for std::make_pair
is <utility>
. Your implementation may have defined it as a side-effect of including <map>
, but it's not portable to rely on that.
size_t pos
ought to be std::size_t pos
.
Test program
I managed to re-create some input from your sample output:
int main()
static auto const messages =
"<3>CTRL-EVENT-SCAN-STARTED ",
"<3>CTRL-EVENT-BSS-ADDED 0 00:00:00:00:00:00",
"<3>CTRL-EVENT-BSS-ADDED 1 00:00:00:00:00:01",
"<3>CTRL-EVENT-BSS-ADDED 2 00:00:00:00:00:02",
"<3>CTRL-EVENT-SCAN-RESULTS ",
"<3>WPS-AP-AVAILABLE ",
"<3>Trying to associate with 00:00:00:00:00:00 (SSID='my_ssid' freq=5745 MHz)",
"<3>Associated with 00:00:00:00:00:00",
"<3>CTRL-EVENT-SUBNET-STATUS-UPDATE status=0",
"<3>WPA: Key negotiation completed with 00:00:00:00:00:00 [PTK=CCMP GTK=TKIP]",
"<3>CTRL-EVENT-CONNECTED - Connection to 00:00:00:00:00:00 completed [id=1 id_str=]",
;
wpa_monitor monitor;
for (auto m: messages)
monitor.handle_wpa_message(m);
Naming
I avoid names that include their type, as it can be confusing if you later change that type. So I'd call the map mac_to_ssid
instead. That also has the benefit of being shorter, so easy to type and to read.
Extensibility
You'll find that a single large method gets harder to navigate as it gets bigger. As hinted in another answer, it makes sense to delegate to a bunch of simple methods that each handle a single case. You could iterate through a vector, but I'd stick to something simple like this:
class wpa_monitor
public:
bool handle_wpa_message(const std::string& message);
private:
std::map<std::string, std::string> mac_ssid_association_map = ;
bool handle_associate(const std::string& message);
bool handle_connected(const std::string& message);
bool handle_scanresult(const std::string& message);
;
bool wpa_monitor::handle_wpa_message(const std::string& message)
handle_connected(message)
Now we can look at each matcher in turn.
Refactor some common code
We repeatedly search for a string and take an interest in its end, like this:
auto pos = message.find("Trying to associate with ");
if (pos == std::string::npos)
return false;
std::string mac_substring = message.substr(pos + sizeof "Trying to associate with " - 1);
Obviously there's scope for error if we don't keep the two copies of the string in agreement. Instead, we can write a small function to do this:
static std::size_t find_end(const std::string& s, const std::string& substr)
auto pos = s.find(substr);
if (pos != std::string::npos)
pos += substr.size();
return pos;
That makes the caller simpler and more robust:
auto pos = find_end(message, "Trying to associate with ");
if (pos == std::string::npos)
return false;
auto const mac_substring = message.substr(pos);
Actually, that's not really the MAC substring, as it continues to the end of the string. We really want another helper, accepting a termination character:
static std::string find_in_delimiters(const std::string& s,
const std::string& open, const char close)
auto start = s.find(open);
if (start == std::string::npos)
return ;
start += open.size();
auto end = s.find(close, start);
if (end == std::string::npos)
return ;
return s.substr(start, end-start);
And we simplify the callers even more:
auto mac_addr = find_in_delimiters(message, "Trying to associate with ", ' ');
if (mac_addr.empty())
return false;
More on make_pair()
I get a failure to compile, as GCC seems to want rvalue references for make_pair<string,string>()
. I could resolve that with a std::move()
like this:
mac_ssid_association_map.insert( std::make_pair< std::string, std::string >( std::move(mac_addr), std::move(ssid) ) );
However, we can let template type deduction do its thing:
mac_ssid_association_map.insert(std::make_pair(std::move(mac_addr), std::move(ssid)));
Or even just:
mac_ssid_association_map.insert(std::move(mac_addr), std::move(ssid));
Use auto
instead of exact iterator types
Here's a place where changing the map type could be a maintenance headache:
std::map< std::string, std::string >::iterator it = mac_ssid_association_map.find( mac_addr );
We can use auto
, which also makes it more readable:
auto it = mac_ssid_association_map.find(mac_addr);
My version
#include <cstdio>
#include <string>
#include <map>
#include <utility>
#define DEBUG_PRINT std::printf
class wpa_monitor
public:
bool handle_wpa_message(const std::string& message);
private:
std::map<std::string, std::string> mac_to_ssid = ;
bool handle_associate(const std::string& message);
bool handle_connected(const std::string& message);
bool handle_scanresult(const std::string& message);
;
// Helper method - returns substring of s contained between
// open and close, or empty if not matched
static std::string find_in_delimiters(const std::string& s,
const std::string& open, const char close)
auto start = s.find(open);
if (start == s.npos)
return ;
start += open.size();
auto end = s.find(close, start);
if (end == s.npos)
return ;
return s.substr(start, end-start);
// Handlers for specific message types
bool wpa_monitor::handle_associate(const std::string& message)
auto mac_addr = find_in_delimiters(message, "Trying to associate with ", ' ');
if (mac_addr.empty())
return false;
auto ssid = find_in_delimiters(message, "SSID='", ''');
if (ssid.empty())
return false;
DEBUG_PRINT("Associating mac_address '%s' with ssid '%s'n", mac_addr.c_str(), ssid.c_str());
mac_to_ssid.insert(std::move(mac_addr), std::move(ssid));
return true;
bool wpa_monitor::handle_connected(const std::string& message)
auto mac_addr = find_in_delimiters(message, "CTRL-EVENT-CONNECTED - Connection to ", ' ');
if (mac_addr.empty())
return false;
auto it = mac_to_ssid.find(mac_addr);
if (it != mac_to_ssid.end())
DEBUG_PRINT("We are now connected to ssid '%s'n", it->second.c_str());
return true;
bool wpa_monitor::handle_scanresult(const std::string& message)
auto pos = message.find("CTRL-EVENT-SCAN-RESULTS");
if (pos == message.npos)
return false;
//scan results ready
DEBUG_PRINT("Scan results are readyn");
return true;
// Test program
int main()
static auto const messages =
"<3>CTRL-EVENT-SCAN-STARTED ",
"<3>CTRL-EVENT-BSS-ADDED 0 00:00:00:00:00:00",
"<3>CTRL-EVENT-BSS-ADDED 1 00:00:00:00:00:01",
"<3>CTRL-EVENT-BSS-ADDED 2 00:00:00:00:00:02",
"<3>CTRL-EVENT-SCAN-RESULTS ",
"<3>WPS-AP-AVAILABLE ",
"<3>Trying to associate with 00:00:00:00:00:00 (SSID='my_ssid' freq=5745 MHz)",
"<3>Associated with 00:00:00:00:00:00",
"<3>CTRL-EVENT-SUBNET-STATUS-UPDATE status=0",
"<3>WPA: Key negotiation completed with 00:00:00:00:00:00 [PTK=CCMP GTK=TKIP]",
"<3>CTRL-EVENT-CONNECTED - Connection to 00:00:00:00:00:00 completed [id=1 id_str=]",
;
wpa_monitor monitor;
for (auto m: messages)
monitor.handle_wpa_message(m);
Thanks for the thorough review and recommendations! I suppose I should have specified that I was using C++98 which may have been the reason for some of the cumbersome/different semantics in my code, however I did not want to limit answers to C++98 as I am learning modern C++ at the moment and pushing to get C++17 support in the next few months (not to mention a C++98 review would not be as useful to those who don't have my constraints). I am working in a heavily C oriented culture at the moment and it is very helpful to get C++ code reviews from the people in the C++ community.
â Nathan Owen
Feb 9 at 20:43
I tend to use C++11 as a baseline these days (on the one project where I'm stuck with C++03, I feel frustration), so much of my answer might not be relevant. OTOH, perhaps it provides you with some ammunition in any argument^W discussion over tooling - I'm believe the modern C++ is much more readable. The Refactor, Extensibility and Naming sections should still be relevant for any language version (and, indeed, other languages).
â Toby Speight
Feb 12 at 9:20
add a comment |Â
up vote
4
down vote
accepted
I'll make no comment on the use of C-style <stdio.h>
, since that's likely an artefact of the surrounding code, rather than something you'd have written yourself.
A missing header and a typo/oversight
The header required for std::make_pair
is <utility>
. Your implementation may have defined it as a side-effect of including <map>
, but it's not portable to rely on that.
size_t pos
ought to be std::size_t pos
.
Test program
I managed to re-create some input from your sample output:
int main()
static auto const messages =
"<3>CTRL-EVENT-SCAN-STARTED ",
"<3>CTRL-EVENT-BSS-ADDED 0 00:00:00:00:00:00",
"<3>CTRL-EVENT-BSS-ADDED 1 00:00:00:00:00:01",
"<3>CTRL-EVENT-BSS-ADDED 2 00:00:00:00:00:02",
"<3>CTRL-EVENT-SCAN-RESULTS ",
"<3>WPS-AP-AVAILABLE ",
"<3>Trying to associate with 00:00:00:00:00:00 (SSID='my_ssid' freq=5745 MHz)",
"<3>Associated with 00:00:00:00:00:00",
"<3>CTRL-EVENT-SUBNET-STATUS-UPDATE status=0",
"<3>WPA: Key negotiation completed with 00:00:00:00:00:00 [PTK=CCMP GTK=TKIP]",
"<3>CTRL-EVENT-CONNECTED - Connection to 00:00:00:00:00:00 completed [id=1 id_str=]",
;
wpa_monitor monitor;
for (auto m: messages)
monitor.handle_wpa_message(m);
Naming
I avoid names that include their type, as it can be confusing if you later change that type. So I'd call the map mac_to_ssid
instead. That also has the benefit of being shorter, so easy to type and to read.
Extensibility
You'll find that a single large method gets harder to navigate as it gets bigger. As hinted in another answer, it makes sense to delegate to a bunch of simple methods that each handle a single case. You could iterate through a vector, but I'd stick to something simple like this:
class wpa_monitor
public:
bool handle_wpa_message(const std::string& message);
private:
std::map<std::string, std::string> mac_ssid_association_map = ;
bool handle_associate(const std::string& message);
bool handle_connected(const std::string& message);
bool handle_scanresult(const std::string& message);
;
bool wpa_monitor::handle_wpa_message(const std::string& message)
handle_connected(message)
Now we can look at each matcher in turn.
Refactor some common code
We repeatedly search for a string and take an interest in its end, like this:
auto pos = message.find("Trying to associate with ");
if (pos == std::string::npos)
return false;
std::string mac_substring = message.substr(pos + sizeof "Trying to associate with " - 1);
Obviously there's scope for error if we don't keep the two copies of the string in agreement. Instead, we can write a small function to do this:
static std::size_t find_end(const std::string& s, const std::string& substr)
auto pos = s.find(substr);
if (pos != std::string::npos)
pos += substr.size();
return pos;
That makes the caller simpler and more robust:
auto pos = find_end(message, "Trying to associate with ");
if (pos == std::string::npos)
return false;
auto const mac_substring = message.substr(pos);
Actually, that's not really the MAC substring, as it continues to the end of the string. We really want another helper, accepting a termination character:
static std::string find_in_delimiters(const std::string& s,
const std::string& open, const char close)
auto start = s.find(open);
if (start == std::string::npos)
return ;
start += open.size();
auto end = s.find(close, start);
if (end == std::string::npos)
return ;
return s.substr(start, end-start);
And we simplify the callers even more:
auto mac_addr = find_in_delimiters(message, "Trying to associate with ", ' ');
if (mac_addr.empty())
return false;
More on make_pair()
I get a failure to compile, as GCC seems to want rvalue references for make_pair<string,string>()
. I could resolve that with a std::move()
like this:
mac_ssid_association_map.insert( std::make_pair< std::string, std::string >( std::move(mac_addr), std::move(ssid) ) );
However, we can let template type deduction do its thing:
mac_ssid_association_map.insert(std::make_pair(std::move(mac_addr), std::move(ssid)));
Or even just:
mac_ssid_association_map.insert(std::move(mac_addr), std::move(ssid));
Use auto
instead of exact iterator types
Here's a place where changing the map type could be a maintenance headache:
std::map< std::string, std::string >::iterator it = mac_ssid_association_map.find( mac_addr );
We can use auto
, which also makes it more readable:
auto it = mac_ssid_association_map.find(mac_addr);
My version
#include <cstdio>
#include <string>
#include <map>
#include <utility>
#define DEBUG_PRINT std::printf
class wpa_monitor
public:
bool handle_wpa_message(const std::string& message);
private:
std::map<std::string, std::string> mac_to_ssid = ;
bool handle_associate(const std::string& message);
bool handle_connected(const std::string& message);
bool handle_scanresult(const std::string& message);
;
// Helper method - returns substring of s contained between
// open and close, or empty if not matched
static std::string find_in_delimiters(const std::string& s,
const std::string& open, const char close)
auto start = s.find(open);
if (start == s.npos)
return ;
start += open.size();
auto end = s.find(close, start);
if (end == s.npos)
return ;
return s.substr(start, end-start);
// Handlers for specific message types
bool wpa_monitor::handle_associate(const std::string& message)
auto mac_addr = find_in_delimiters(message, "Trying to associate with ", ' ');
if (mac_addr.empty())
return false;
auto ssid = find_in_delimiters(message, "SSID='", ''');
if (ssid.empty())
return false;
DEBUG_PRINT("Associating mac_address '%s' with ssid '%s'n", mac_addr.c_str(), ssid.c_str());
mac_to_ssid.insert(std::move(mac_addr), std::move(ssid));
return true;
bool wpa_monitor::handle_connected(const std::string& message)
auto mac_addr = find_in_delimiters(message, "CTRL-EVENT-CONNECTED - Connection to ", ' ');
if (mac_addr.empty())
return false;
auto it = mac_to_ssid.find(mac_addr);
if (it != mac_to_ssid.end())
DEBUG_PRINT("We are now connected to ssid '%s'n", it->second.c_str());
return true;
bool wpa_monitor::handle_scanresult(const std::string& message)
auto pos = message.find("CTRL-EVENT-SCAN-RESULTS");
if (pos == message.npos)
return false;
//scan results ready
DEBUG_PRINT("Scan results are readyn");
return true;
// Test program
int main()
static auto const messages =
"<3>CTRL-EVENT-SCAN-STARTED ",
"<3>CTRL-EVENT-BSS-ADDED 0 00:00:00:00:00:00",
"<3>CTRL-EVENT-BSS-ADDED 1 00:00:00:00:00:01",
"<3>CTRL-EVENT-BSS-ADDED 2 00:00:00:00:00:02",
"<3>CTRL-EVENT-SCAN-RESULTS ",
"<3>WPS-AP-AVAILABLE ",
"<3>Trying to associate with 00:00:00:00:00:00 (SSID='my_ssid' freq=5745 MHz)",
"<3>Associated with 00:00:00:00:00:00",
"<3>CTRL-EVENT-SUBNET-STATUS-UPDATE status=0",
"<3>WPA: Key negotiation completed with 00:00:00:00:00:00 [PTK=CCMP GTK=TKIP]",
"<3>CTRL-EVENT-CONNECTED - Connection to 00:00:00:00:00:00 completed [id=1 id_str=]",
;
wpa_monitor monitor;
for (auto m: messages)
monitor.handle_wpa_message(m);
Thanks for the thorough review and recommendations! I suppose I should have specified that I was using C++98 which may have been the reason for some of the cumbersome/different semantics in my code, however I did not want to limit answers to C++98 as I am learning modern C++ at the moment and pushing to get C++17 support in the next few months (not to mention a C++98 review would not be as useful to those who don't have my constraints). I am working in a heavily C oriented culture at the moment and it is very helpful to get C++ code reviews from the people in the C++ community.
â Nathan Owen
Feb 9 at 20:43
I tend to use C++11 as a baseline these days (on the one project where I'm stuck with C++03, I feel frustration), so much of my answer might not be relevant. OTOH, perhaps it provides you with some ammunition in any argument^W discussion over tooling - I'm believe the modern C++ is much more readable. The Refactor, Extensibility and Naming sections should still be relevant for any language version (and, indeed, other languages).
â Toby Speight
Feb 12 at 9:20
add a comment |Â
up vote
4
down vote
accepted
up vote
4
down vote
accepted
I'll make no comment on the use of C-style <stdio.h>
, since that's likely an artefact of the surrounding code, rather than something you'd have written yourself.
A missing header and a typo/oversight
The header required for std::make_pair
is <utility>
. Your implementation may have defined it as a side-effect of including <map>
, but it's not portable to rely on that.
size_t pos
ought to be std::size_t pos
.
Test program
I managed to re-create some input from your sample output:
int main()
static auto const messages =
"<3>CTRL-EVENT-SCAN-STARTED ",
"<3>CTRL-EVENT-BSS-ADDED 0 00:00:00:00:00:00",
"<3>CTRL-EVENT-BSS-ADDED 1 00:00:00:00:00:01",
"<3>CTRL-EVENT-BSS-ADDED 2 00:00:00:00:00:02",
"<3>CTRL-EVENT-SCAN-RESULTS ",
"<3>WPS-AP-AVAILABLE ",
"<3>Trying to associate with 00:00:00:00:00:00 (SSID='my_ssid' freq=5745 MHz)",
"<3>Associated with 00:00:00:00:00:00",
"<3>CTRL-EVENT-SUBNET-STATUS-UPDATE status=0",
"<3>WPA: Key negotiation completed with 00:00:00:00:00:00 [PTK=CCMP GTK=TKIP]",
"<3>CTRL-EVENT-CONNECTED - Connection to 00:00:00:00:00:00 completed [id=1 id_str=]",
;
wpa_monitor monitor;
for (auto m: messages)
monitor.handle_wpa_message(m);
Naming
I avoid names that include their type, as it can be confusing if you later change that type. So I'd call the map mac_to_ssid
instead. That also has the benefit of being shorter, so easy to type and to read.
Extensibility
You'll find that a single large method gets harder to navigate as it gets bigger. As hinted in another answer, it makes sense to delegate to a bunch of simple methods that each handle a single case. You could iterate through a vector, but I'd stick to something simple like this:
class wpa_monitor
public:
bool handle_wpa_message(const std::string& message);
private:
std::map<std::string, std::string> mac_ssid_association_map = ;
bool handle_associate(const std::string& message);
bool handle_connected(const std::string& message);
bool handle_scanresult(const std::string& message);
;
bool wpa_monitor::handle_wpa_message(const std::string& message)
handle_connected(message)
Now we can look at each matcher in turn.
Refactor some common code
We repeatedly search for a string and take an interest in its end, like this:
auto pos = message.find("Trying to associate with ");
if (pos == std::string::npos)
return false;
std::string mac_substring = message.substr(pos + sizeof "Trying to associate with " - 1);
Obviously there's scope for error if we don't keep the two copies of the string in agreement. Instead, we can write a small function to do this:
static std::size_t find_end(const std::string& s, const std::string& substr)
auto pos = s.find(substr);
if (pos != std::string::npos)
pos += substr.size();
return pos;
That makes the caller simpler and more robust:
auto pos = find_end(message, "Trying to associate with ");
if (pos == std::string::npos)
return false;
auto const mac_substring = message.substr(pos);
Actually, that's not really the MAC substring, as it continues to the end of the string. We really want another helper, accepting a termination character:
static std::string find_in_delimiters(const std::string& s,
const std::string& open, const char close)
auto start = s.find(open);
if (start == std::string::npos)
return ;
start += open.size();
auto end = s.find(close, start);
if (end == std::string::npos)
return ;
return s.substr(start, end-start);
And we simplify the callers even more:
auto mac_addr = find_in_delimiters(message, "Trying to associate with ", ' ');
if (mac_addr.empty())
return false;
More on make_pair()
I get a failure to compile, as GCC seems to want rvalue references for make_pair<string,string>()
. I could resolve that with a std::move()
like this:
mac_ssid_association_map.insert( std::make_pair< std::string, std::string >( std::move(mac_addr), std::move(ssid) ) );
However, we can let template type deduction do its thing:
mac_ssid_association_map.insert(std::make_pair(std::move(mac_addr), std::move(ssid)));
Or even just:
mac_ssid_association_map.insert(std::move(mac_addr), std::move(ssid));
Use auto
instead of exact iterator types
Here's a place where changing the map type could be a maintenance headache:
std::map< std::string, std::string >::iterator it = mac_ssid_association_map.find( mac_addr );
We can use auto
, which also makes it more readable:
auto it = mac_ssid_association_map.find(mac_addr);
My version
#include <cstdio>
#include <string>
#include <map>
#include <utility>
#define DEBUG_PRINT std::printf
class wpa_monitor
public:
bool handle_wpa_message(const std::string& message);
private:
std::map<std::string, std::string> mac_to_ssid = ;
bool handle_associate(const std::string& message);
bool handle_connected(const std::string& message);
bool handle_scanresult(const std::string& message);
;
// Helper method - returns substring of s contained between
// open and close, or empty if not matched
static std::string find_in_delimiters(const std::string& s,
const std::string& open, const char close)
auto start = s.find(open);
if (start == s.npos)
return ;
start += open.size();
auto end = s.find(close, start);
if (end == s.npos)
return ;
return s.substr(start, end-start);
// Handlers for specific message types
bool wpa_monitor::handle_associate(const std::string& message)
auto mac_addr = find_in_delimiters(message, "Trying to associate with ", ' ');
if (mac_addr.empty())
return false;
auto ssid = find_in_delimiters(message, "SSID='", ''');
if (ssid.empty())
return false;
DEBUG_PRINT("Associating mac_address '%s' with ssid '%s'n", mac_addr.c_str(), ssid.c_str());
mac_to_ssid.insert(std::move(mac_addr), std::move(ssid));
return true;
bool wpa_monitor::handle_connected(const std::string& message)
auto mac_addr = find_in_delimiters(message, "CTRL-EVENT-CONNECTED - Connection to ", ' ');
if (mac_addr.empty())
return false;
auto it = mac_to_ssid.find(mac_addr);
if (it != mac_to_ssid.end())
DEBUG_PRINT("We are now connected to ssid '%s'n", it->second.c_str());
return true;
bool wpa_monitor::handle_scanresult(const std::string& message)
auto pos = message.find("CTRL-EVENT-SCAN-RESULTS");
if (pos == message.npos)
return false;
//scan results ready
DEBUG_PRINT("Scan results are readyn");
return true;
// Test program
int main()
static auto const messages =
"<3>CTRL-EVENT-SCAN-STARTED ",
"<3>CTRL-EVENT-BSS-ADDED 0 00:00:00:00:00:00",
"<3>CTRL-EVENT-BSS-ADDED 1 00:00:00:00:00:01",
"<3>CTRL-EVENT-BSS-ADDED 2 00:00:00:00:00:02",
"<3>CTRL-EVENT-SCAN-RESULTS ",
"<3>WPS-AP-AVAILABLE ",
"<3>Trying to associate with 00:00:00:00:00:00 (SSID='my_ssid' freq=5745 MHz)",
"<3>Associated with 00:00:00:00:00:00",
"<3>CTRL-EVENT-SUBNET-STATUS-UPDATE status=0",
"<3>WPA: Key negotiation completed with 00:00:00:00:00:00 [PTK=CCMP GTK=TKIP]",
"<3>CTRL-EVENT-CONNECTED - Connection to 00:00:00:00:00:00 completed [id=1 id_str=]",
;
wpa_monitor monitor;
for (auto m: messages)
monitor.handle_wpa_message(m);
I'll make no comment on the use of C-style <stdio.h>
, since that's likely an artefact of the surrounding code, rather than something you'd have written yourself.
A missing header and a typo/oversight
The header required for std::make_pair
is <utility>
. Your implementation may have defined it as a side-effect of including <map>
, but it's not portable to rely on that.
size_t pos
ought to be std::size_t pos
.
Test program
I managed to re-create some input from your sample output:
int main()
static auto const messages =
"<3>CTRL-EVENT-SCAN-STARTED ",
"<3>CTRL-EVENT-BSS-ADDED 0 00:00:00:00:00:00",
"<3>CTRL-EVENT-BSS-ADDED 1 00:00:00:00:00:01",
"<3>CTRL-EVENT-BSS-ADDED 2 00:00:00:00:00:02",
"<3>CTRL-EVENT-SCAN-RESULTS ",
"<3>WPS-AP-AVAILABLE ",
"<3>Trying to associate with 00:00:00:00:00:00 (SSID='my_ssid' freq=5745 MHz)",
"<3>Associated with 00:00:00:00:00:00",
"<3>CTRL-EVENT-SUBNET-STATUS-UPDATE status=0",
"<3>WPA: Key negotiation completed with 00:00:00:00:00:00 [PTK=CCMP GTK=TKIP]",
"<3>CTRL-EVENT-CONNECTED - Connection to 00:00:00:00:00:00 completed [id=1 id_str=]",
;
wpa_monitor monitor;
for (auto m: messages)
monitor.handle_wpa_message(m);
Naming
I avoid names that include their type, as it can be confusing if you later change that type. So I'd call the map mac_to_ssid
instead. That also has the benefit of being shorter, so easy to type and to read.
Extensibility
You'll find that a single large method gets harder to navigate as it gets bigger. As hinted in another answer, it makes sense to delegate to a bunch of simple methods that each handle a single case. You could iterate through a vector, but I'd stick to something simple like this:
class wpa_monitor
public:
bool handle_wpa_message(const std::string& message);
private:
std::map<std::string, std::string> mac_ssid_association_map = ;
bool handle_associate(const std::string& message);
bool handle_connected(const std::string& message);
bool handle_scanresult(const std::string& message);
;
bool wpa_monitor::handle_wpa_message(const std::string& message)
handle_connected(message)
Now we can look at each matcher in turn.
Refactor some common code
We repeatedly search for a string and take an interest in its end, like this:
auto pos = message.find("Trying to associate with ");
if (pos == std::string::npos)
return false;
std::string mac_substring = message.substr(pos + sizeof "Trying to associate with " - 1);
Obviously there's scope for error if we don't keep the two copies of the string in agreement. Instead, we can write a small function to do this:
static std::size_t find_end(const std::string& s, const std::string& substr)
auto pos = s.find(substr);
if (pos != std::string::npos)
pos += substr.size();
return pos;
That makes the caller simpler and more robust:
auto pos = find_end(message, "Trying to associate with ");
if (pos == std::string::npos)
return false;
auto const mac_substring = message.substr(pos);
Actually, that's not really the MAC substring, as it continues to the end of the string. We really want another helper, accepting a termination character:
static std::string find_in_delimiters(const std::string& s,
const std::string& open, const char close)
auto start = s.find(open);
if (start == std::string::npos)
return ;
start += open.size();
auto end = s.find(close, start);
if (end == std::string::npos)
return ;
return s.substr(start, end-start);
And we simplify the callers even more:
auto mac_addr = find_in_delimiters(message, "Trying to associate with ", ' ');
if (mac_addr.empty())
return false;
More on make_pair()
I get a failure to compile, as GCC seems to want rvalue references for make_pair<string,string>()
. I could resolve that with a std::move()
like this:
mac_ssid_association_map.insert( std::make_pair< std::string, std::string >( std::move(mac_addr), std::move(ssid) ) );
However, we can let template type deduction do its thing:
mac_ssid_association_map.insert(std::make_pair(std::move(mac_addr), std::move(ssid)));
Or even just:
mac_ssid_association_map.insert(std::move(mac_addr), std::move(ssid));
Use auto
instead of exact iterator types
Here's a place where changing the map type could be a maintenance headache:
std::map< std::string, std::string >::iterator it = mac_ssid_association_map.find( mac_addr );
We can use auto
, which also makes it more readable:
auto it = mac_ssid_association_map.find(mac_addr);
My version
#include <cstdio>
#include <string>
#include <map>
#include <utility>
#define DEBUG_PRINT std::printf
class wpa_monitor
public:
bool handle_wpa_message(const std::string& message);
private:
std::map<std::string, std::string> mac_to_ssid = ;
bool handle_associate(const std::string& message);
bool handle_connected(const std::string& message);
bool handle_scanresult(const std::string& message);
;
// Helper method - returns substring of s contained between
// open and close, or empty if not matched
static std::string find_in_delimiters(const std::string& s,
const std::string& open, const char close)
auto start = s.find(open);
if (start == s.npos)
return ;
start += open.size();
auto end = s.find(close, start);
if (end == s.npos)
return ;
return s.substr(start, end-start);
// Handlers for specific message types
bool wpa_monitor::handle_associate(const std::string& message)
auto mac_addr = find_in_delimiters(message, "Trying to associate with ", ' ');
if (mac_addr.empty())
return false;
auto ssid = find_in_delimiters(message, "SSID='", ''');
if (ssid.empty())
return false;
DEBUG_PRINT("Associating mac_address '%s' with ssid '%s'n", mac_addr.c_str(), ssid.c_str());
mac_to_ssid.insert(std::move(mac_addr), std::move(ssid));
return true;
bool wpa_monitor::handle_connected(const std::string& message)
auto mac_addr = find_in_delimiters(message, "CTRL-EVENT-CONNECTED - Connection to ", ' ');
if (mac_addr.empty())
return false;
auto it = mac_to_ssid.find(mac_addr);
if (it != mac_to_ssid.end())
DEBUG_PRINT("We are now connected to ssid '%s'n", it->second.c_str());
return true;
bool wpa_monitor::handle_scanresult(const std::string& message)
auto pos = message.find("CTRL-EVENT-SCAN-RESULTS");
if (pos == message.npos)
return false;
//scan results ready
DEBUG_PRINT("Scan results are readyn");
return true;
// Test program
int main()
static auto const messages =
"<3>CTRL-EVENT-SCAN-STARTED ",
"<3>CTRL-EVENT-BSS-ADDED 0 00:00:00:00:00:00",
"<3>CTRL-EVENT-BSS-ADDED 1 00:00:00:00:00:01",
"<3>CTRL-EVENT-BSS-ADDED 2 00:00:00:00:00:02",
"<3>CTRL-EVENT-SCAN-RESULTS ",
"<3>WPS-AP-AVAILABLE ",
"<3>Trying to associate with 00:00:00:00:00:00 (SSID='my_ssid' freq=5745 MHz)",
"<3>Associated with 00:00:00:00:00:00",
"<3>CTRL-EVENT-SUBNET-STATUS-UPDATE status=0",
"<3>WPA: Key negotiation completed with 00:00:00:00:00:00 [PTK=CCMP GTK=TKIP]",
"<3>CTRL-EVENT-CONNECTED - Connection to 00:00:00:00:00:00 completed [id=1 id_str=]",
;
wpa_monitor monitor;
for (auto m: messages)
monitor.handle_wpa_message(m);
edited Feb 9 at 11:49
answered Feb 9 at 10:54
Toby Speight
17.8k13490
17.8k13490
Thanks for the thorough review and recommendations! I suppose I should have specified that I was using C++98 which may have been the reason for some of the cumbersome/different semantics in my code, however I did not want to limit answers to C++98 as I am learning modern C++ at the moment and pushing to get C++17 support in the next few months (not to mention a C++98 review would not be as useful to those who don't have my constraints). I am working in a heavily C oriented culture at the moment and it is very helpful to get C++ code reviews from the people in the C++ community.
â Nathan Owen
Feb 9 at 20:43
I tend to use C++11 as a baseline these days (on the one project where I'm stuck with C++03, I feel frustration), so much of my answer might not be relevant. OTOH, perhaps it provides you with some ammunition in any argument^W discussion over tooling - I'm believe the modern C++ is much more readable. The Refactor, Extensibility and Naming sections should still be relevant for any language version (and, indeed, other languages).
â Toby Speight
Feb 12 at 9:20
add a comment |Â
Thanks for the thorough review and recommendations! I suppose I should have specified that I was using C++98 which may have been the reason for some of the cumbersome/different semantics in my code, however I did not want to limit answers to C++98 as I am learning modern C++ at the moment and pushing to get C++17 support in the next few months (not to mention a C++98 review would not be as useful to those who don't have my constraints). I am working in a heavily C oriented culture at the moment and it is very helpful to get C++ code reviews from the people in the C++ community.
â Nathan Owen
Feb 9 at 20:43
I tend to use C++11 as a baseline these days (on the one project where I'm stuck with C++03, I feel frustration), so much of my answer might not be relevant. OTOH, perhaps it provides you with some ammunition in any argument^W discussion over tooling - I'm believe the modern C++ is much more readable. The Refactor, Extensibility and Naming sections should still be relevant for any language version (and, indeed, other languages).
â Toby Speight
Feb 12 at 9:20
Thanks for the thorough review and recommendations! I suppose I should have specified that I was using C++98 which may have been the reason for some of the cumbersome/different semantics in my code, however I did not want to limit answers to C++98 as I am learning modern C++ at the moment and pushing to get C++17 support in the next few months (not to mention a C++98 review would not be as useful to those who don't have my constraints). I am working in a heavily C oriented culture at the moment and it is very helpful to get C++ code reviews from the people in the C++ community.
â Nathan Owen
Feb 9 at 20:43
Thanks for the thorough review and recommendations! I suppose I should have specified that I was using C++98 which may have been the reason for some of the cumbersome/different semantics in my code, however I did not want to limit answers to C++98 as I am learning modern C++ at the moment and pushing to get C++17 support in the next few months (not to mention a C++98 review would not be as useful to those who don't have my constraints). I am working in a heavily C oriented culture at the moment and it is very helpful to get C++ code reviews from the people in the C++ community.
â Nathan Owen
Feb 9 at 20:43
I tend to use C++11 as a baseline these days (on the one project where I'm stuck with C++03, I feel frustration), so much of my answer might not be relevant. OTOH, perhaps it provides you with some ammunition in any argument^W discussion over tooling - I'm believe the modern C++ is much more readable. The Refactor, Extensibility and Naming sections should still be relevant for any language version (and, indeed, other languages).
â Toby Speight
Feb 12 at 9:20
I tend to use C++11 as a baseline these days (on the one project where I'm stuck with C++03, I feel frustration), so much of my answer might not be relevant. OTOH, perhaps it provides you with some ammunition in any argument^W discussion over tooling - I'm believe the modern C++ is much more readable. The Refactor, Extensibility and Naming sections should still be relevant for any language version (and, indeed, other languages).
â Toby Speight
Feb 12 at 9:20
add a comment |Â
up vote
3
down vote
You're right to look for a more scalable implementation. From where I stand, the most simple one would be to associate, in a container, the strings you look for to identify a message and the function you then want to apply to the message. Something like:
/* given:
#include <string>
#include <map>
#include <algorithm>
#include <functional>
*/
void wpa_handler::handle_wpa_message( const std::string& message )
/* given, as class member:
std::map<std::string, std::function<void(const std::string&)>> message_dispatch;
// that you populate somewhere in the construction process:
message_dispatch["Trying to associate with "] = handle_function1;
message_dispatch["CTRL-EVENT-CONNECTED - Connection to "] = handle_function2;
// etc.
*/
auto handler = std::find_if(message_dispatch.begin(), message_dispatch.end(), [&message](auto&& pair)
return message.find(pair.first) != std::string::npos;
);
if (handler == message_dispatch.end())
// handle unknown message recieved -> exception?
else
handler->second(message);
Another, maybe yet simpler option, is to have a vector of your handling functions, and pass the message to each of them. It's then the handling function which is responsible for detecting if it shall handle that message or not. The problem is that you can't know if the message was handled, and by which handler, unless you implement a feedback mechanism.
The feedback mechanism could be as simple asreturn true;
in the handler. Then you'd havefor (auto& h: handlers) if (h.handle(message)) break;
â Toby Speight
Feb 8 at 10:00
add a comment |Â
up vote
3
down vote
You're right to look for a more scalable implementation. From where I stand, the most simple one would be to associate, in a container, the strings you look for to identify a message and the function you then want to apply to the message. Something like:
/* given:
#include <string>
#include <map>
#include <algorithm>
#include <functional>
*/
void wpa_handler::handle_wpa_message( const std::string& message )
/* given, as class member:
std::map<std::string, std::function<void(const std::string&)>> message_dispatch;
// that you populate somewhere in the construction process:
message_dispatch["Trying to associate with "] = handle_function1;
message_dispatch["CTRL-EVENT-CONNECTED - Connection to "] = handle_function2;
// etc.
*/
auto handler = std::find_if(message_dispatch.begin(), message_dispatch.end(), [&message](auto&& pair)
return message.find(pair.first) != std::string::npos;
);
if (handler == message_dispatch.end())
// handle unknown message recieved -> exception?
else
handler->second(message);
Another, maybe yet simpler option, is to have a vector of your handling functions, and pass the message to each of them. It's then the handling function which is responsible for detecting if it shall handle that message or not. The problem is that you can't know if the message was handled, and by which handler, unless you implement a feedback mechanism.
The feedback mechanism could be as simple asreturn true;
in the handler. Then you'd havefor (auto& h: handlers) if (h.handle(message)) break;
â Toby Speight
Feb 8 at 10:00
add a comment |Â
up vote
3
down vote
up vote
3
down vote
You're right to look for a more scalable implementation. From where I stand, the most simple one would be to associate, in a container, the strings you look for to identify a message and the function you then want to apply to the message. Something like:
/* given:
#include <string>
#include <map>
#include <algorithm>
#include <functional>
*/
void wpa_handler::handle_wpa_message( const std::string& message )
/* given, as class member:
std::map<std::string, std::function<void(const std::string&)>> message_dispatch;
// that you populate somewhere in the construction process:
message_dispatch["Trying to associate with "] = handle_function1;
message_dispatch["CTRL-EVENT-CONNECTED - Connection to "] = handle_function2;
// etc.
*/
auto handler = std::find_if(message_dispatch.begin(), message_dispatch.end(), [&message](auto&& pair)
return message.find(pair.first) != std::string::npos;
);
if (handler == message_dispatch.end())
// handle unknown message recieved -> exception?
else
handler->second(message);
Another, maybe yet simpler option, is to have a vector of your handling functions, and pass the message to each of them. It's then the handling function which is responsible for detecting if it shall handle that message or not. The problem is that you can't know if the message was handled, and by which handler, unless you implement a feedback mechanism.
You're right to look for a more scalable implementation. From where I stand, the most simple one would be to associate, in a container, the strings you look for to identify a message and the function you then want to apply to the message. Something like:
/* given:
#include <string>
#include <map>
#include <algorithm>
#include <functional>
*/
void wpa_handler::handle_wpa_message( const std::string& message )
/* given, as class member:
std::map<std::string, std::function<void(const std::string&)>> message_dispatch;
// that you populate somewhere in the construction process:
message_dispatch["Trying to associate with "] = handle_function1;
message_dispatch["CTRL-EVENT-CONNECTED - Connection to "] = handle_function2;
// etc.
*/
auto handler = std::find_if(message_dispatch.begin(), message_dispatch.end(), [&message](auto&& pair)
return message.find(pair.first) != std::string::npos;
);
if (handler == message_dispatch.end())
// handle unknown message recieved -> exception?
else
handler->second(message);
Another, maybe yet simpler option, is to have a vector of your handling functions, and pass the message to each of them. It's then the handling function which is responsible for detecting if it shall handle that message or not. The problem is that you can't know if the message was handled, and by which handler, unless you implement a feedback mechanism.
answered Feb 8 at 9:50
papagaga
2,799116
2,799116
The feedback mechanism could be as simple asreturn true;
in the handler. Then you'd havefor (auto& h: handlers) if (h.handle(message)) break;
â Toby Speight
Feb 8 at 10:00
add a comment |Â
The feedback mechanism could be as simple asreturn true;
in the handler. Then you'd havefor (auto& h: handlers) if (h.handle(message)) break;
â Toby Speight
Feb 8 at 10:00
The feedback mechanism could be as simple as
return true;
in the handler. Then you'd have for (auto& h: handlers) if (h.handle(message)) break;
â Toby Speight
Feb 8 at 10:00
The feedback mechanism could be as simple as
return true;
in the handler. Then you'd have for (auto& h: handlers) if (h.handle(message)) break;
â Toby Speight
Feb 8 at 10:00
add a comment |Â
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f187065%2fc-string-parser-for-wpa-supplicant%23new-answer', 'question_page');
);
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
1
You'll receive better reviews if you show a complete example. For example, I recommend that you edit to show the definition of
wpa_monitor
and/or the necessary#include
lines, and amain()
that shows how to call your function. It can really help reviewers if they are able to compile and run your program. A sample (anonymised) input would also help, if you can provide that.â Toby Speight
Feb 8 at 8:26
1
@TobySpeight I have edited it a bit more to show some of the dependencies along with a simplified class definition. Not too much more i can show without violating NDA in this case sadly (not to mention hooking this up to wpa_supplicant requires a lot of code which would not be related to my question). Perhaps next time I will take the time to make something synthetic.
â Nathan Owen
Feb 8 at 22:11
That's great - thanks for improving the question!
â Toby Speight
Feb 9 at 9:36