C++ string parser for wpa_supplicant

The name of the pictureThe name of the pictureThe name of the pictureClash 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'






share|improve this question

















  • 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







  • 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
















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'






share|improve this question

















  • 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







  • 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












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'






share|improve this question













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'








share|improve this question












share|improve this question




share|improve this question








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 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




    @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




    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




    @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










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);






share|improve this answer























  • 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

















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.






share|improve this answer





















  • 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











Your Answer




StackExchange.ifUsing("editor", function ()
return StackExchange.using("mathjaxEditing", function ()
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix)
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
);
);
, "mathjax-editing");

StackExchange.ifUsing("editor", function ()
StackExchange.using("externalEditor", function ()
StackExchange.using("snippets", function ()
StackExchange.snippets.init();
);
);
, "code-snippets");

StackExchange.ready(function()
var channelOptions =
tags: "".split(" "),
id: "196"
;
initTagRenderer("".split(" "), "".split(" "), channelOptions);

StackExchange.using("externalEditor", function()
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled)
StackExchange.using("snippets", function()
createEditor();
);

else
createEditor();

);

function createEditor()
StackExchange.prepareEditor(
heartbeatType: 'answer',
convertImagesToLinks: false,
noModals: false,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
);



);








 

draft saved


draft discarded


















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






























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);






share|improve this answer























  • 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














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);






share|improve this answer























  • 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












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);






share|improve this answer















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);







share|improve this answer















share|improve this answer



share|improve this answer








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
















  • 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












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.






share|improve this answer





















  • 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















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.






share|improve this answer





















  • 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













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.






share|improve this answer













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.







share|improve this answer













share|improve this answer



share|improve this answer











answered Feb 8 at 9:50









papagaga

2,799116




2,799116











  • 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
















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













 

draft saved


draft discarded


























 


draft saved


draft discarded














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













































































Popular posts from this blog

Chat program with C++ and SFML

Function to Return a JSON Like Objects Using VBA Collections and Arrays

Will my employers contract hold up in court?