Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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.

share|improve this question
    
I've rolled this back as it invalidated my answer. The original embedded code should be retained after receiving answers. –  Jamal Aug 18 at 19:10
    
@Jamal I'm sorry. For the record, the changes were reflecting your suggestion about the vector initialization (i.e. removing the '=' sign) and renaming the iterators in the loop - there was no need for it3, and the others were renamed for consistency. –  Kalrish Aug 18 at 19:14
3  
That's alright, but be sure to read the Help Center since you're new here. The applicable section is at the very bottom. –  Jamal Aug 18 at 19:15
    
I probably would also use a vector. But I am not sure I follow your logic of not using a std::string. It is quite reasonable to put '\0' into a std::string. –  Loki Astari Aug 18 at 21:59
    
@LokiAstari I haven't used a std::string because the constructor would stop scanning the string literal when it found the first '\0' character. Edit: Oh, sorry, I have just noticed what you meant (list initialization). In real code, I used std::vector because, after all, I was reading "raw" bytes from the netlink socket. But yes, it would have worked as well. –  Kalrish Aug 20 at 12:37

2 Answers 2

up vote 5 down vote accepted
  • 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.

share|improve this answer
1  
Thank you! About the inclusion directives, I purposedly put C++ standard headers first, then STL headers, and lastly other headers. I didn't know about your second nor your third point. Finally, as for the loop, I'll give it a look. –  Kalrish Aug 18 at 19:11
    
@Kalrish: Ah, I hadn't noticed the inclusion thing here. It's mostly up to you, but in many cases, library directives (C and C++) are kept together. –  Jamal Aug 18 at 19:13

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

share|improve this answer
    
I agree with you and Loki Astari (see the comment above) with regards to the std::string, although I still believe that std::vector is semantically more correct in this context. As for std::begin and std::end, I will use them - thanks! The return matter is not very important here, because this was just an "SSCCE", but I agree that it would have simplified the code snippet. Lastly, about functions, I avoided them to avoid copies, but I will look at it again, because it is indeed ugly :). –  Kalrish Aug 20 at 13:38

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.