C++ string parser for wpa_supplicant

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 ready\n" );
        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'

Answer 1

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)
{
    DEBUG_PRINT("WPA event: '%s'\n", message.c_str());

    return handle_associate(message)
        || handle_connected(message)
        || handle_scanresult(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 ready\n");
    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);
}

Answer 2

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.