Parsing of a (Linux) netlink hotplug uevent packet

Question

The netlink service is used, among other things, to notify userspace about hotplug events (e.g. "a device has been connected"). These events come in the form of packets, which follow the following format, explained here:

The netlink packet contains a set of null terminated text lines. The first line of the netlink packet combines the $ACTION and $DEVPATH values, separated by an @ (at sign). Each line after the first contains a KEYWORD=VALUE pair defining a hotplug event variable.

I have made an "SSCCE" of the code I use to parse these kinds of packets, and posted it on Ideone.com. I'd like a review on potential risks and bad practices.

#include <cstdlib>

#include <vector>
#include <unordered_map>
#include <string>
#include <algorithm>
#include <iostream>


int main (void)
{
    /* Even if a std::string was used, a string literal would not work, because
    the fake packet contains '\0'. */
    const std::vector<char> msg = {
        'a','d','d','@','/','s','y','s','/','d','e','v','i','c','e','s','\0',
        'S','U','B','S','Y','S','T','E','M','=','b','l','o','c','k','\0','D',
        'E','V','T','Y','P','E','=','p','a','r','t','i','t','i','o','n','\0'
    };

    std::unordered_map<std::string , std::string> hotplug_event_variables;

    auto it1 = std::find( msg.begin() , msg.end() , '@' );
    if ( it1 != msg.end() )
    {
        /* As per the kernel documentation, the piece before the '@' token is
        the implicit "ACTION" event variable. */
        hotplug_event_variables.emplace( std::piecewise_construct , std::forward_as_tuple("ACTION") , std::forward_as_tuple(msg.begin(),it1) );

        /* It points to the '@'; increment it. */
        ++it1;

        auto it2 = std::find( it1 , msg.end() , '\0' );
        if ( it2 != msg.end() )
        {
            /* As with the previous event variable, DEVPATH is implicit. */
            hotplug_event_variables.emplace( std::piecewise_construct , std::forward_as_tuple("DEVPATH") , std::forward_as_tuple(it1,it2) );

            /* It points to the '\0'; increment it. */
            ++it2;

            /*
             * An example packet:
             *
             * Pass of the loop
             *                   |$ACTION@$DEVPATH0·····=·····0······=······0|
             *                           ^^       ^^    ^     ^^     ^      ^
             *                          it1      it2    |     ||     |      |
             * #1                                it3   it4   it5     |      |
             * #2                                             it3   it4    it5
            */
            for ( auto it3 = it2 , it4 = std::find( it2 , msg.end() , '=' ) , it5 = std::find( it4+1 , msg.end() , '\0') ; it3 != msg.end() , it4 != msg.end() , it5 != msg.end() ; it3 = it5+1 , it4 = std::find( it3 , msg.end() , '=' ) , it5 = std::find( it4 , msg.end() , '\0' ) )
            {
                /* While playing with the fake packet, I found that empty pairs
                could made it into the map. */
                if ( std::distance(it3,it4) > 0 && std::distance(it4+1,it5) > 0 )
                {
                    hotplug_event_variables.emplace( std::piecewise_construct , std::forward_as_tuple(it3,it4) , std::forward_as_tuple(it4+1,it5) );
                }
            }

            /*
             * Expected output:
             *
             * "ACTION"    = "add"
             * "DEVPATH"   = "/sys/devices"
             * "SUBSYSTEM" = "block"
             * "DEVTYPE"   = "partition"
            */
            for ( const auto & x : hotplug_event_variables )
            {
                std::cout << '"' << x.first << "\" = \"" << x.second << "\"\n";
            }
        }
    }

    return EXIT_SUCCESS;
}

More information about netlink may be found at the netlink(7) man page.

Answer 1

• The empty line after the first #include is a little odd. They can all be kept together and still be grouped by type (system, library, and local).

  Also, consider ordering these in some way for organization, such as alphabetically.

• void parameters are not needed on C++, unlike in C.

• C++11 now supports list initialization, which you've mostly done here with msg. You can omit the = to make full use of this.

• This for loop is way too long horizontally:

  for ( auto it3 = it2 , it4 = std::find( it2 , msg.end() , '=' ) , it5 = std::find( it4+1 , msg.end() , '\0') ; it3 != msg.end() , it4 != msg.end() , it5 != msg.end() ; it3 = it5+1 , it4 = std::find( it3 , msg.end() , '=' ) , it5 = std::find( it4 , msg.end() , '\0' ) )
  

  You may be better off just using a while loop and making the calls to std::find() within the body of the loop. Another option would be to wrap this line, but that may also look ugly.

Answer 2

I have a couple of things to add to what @Jamal already said:

• First, about this comment:

  /* Even if a std::string was used, a string literal would not work, because
  the fake packet contains '\0'. */
  

  It's true that a string literal would not work, but you can still use a std::string instead of a std::vector and your wode will work without any other change.

• When possible, try to use std::begin and std::end instead of the member methods begin and end. When writing generic code, it helps to handle containers that do not have these methods work with the global functions (for example C arrays or std::valarray).

• You return from your program with return EXIT_SUCCESS;. It should return 0 with most of the known implementations. If you don't plan to return any error codes from your program, you may as well forget about returning anything from main and let your compiler automagically return 0; at the end of main (it does so if there are no return statements in main). That may even allow you not to include <cstdlib> at all.

• Generally speaking, you're doing with loops operations that could be abstracted into functions. For example, I would have created a split function so split a string on some characters. I would have split the string of \0 characters then on = characters when needed. That would probably produce code more readable than a bunch of loops on iterators. Think about it :)