HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Parsing of a (Linux) netlink hotplug uevent packet

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
netlinkhotplugparsinglinuxueventpacket

Problem

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

#include
#include
#include
#include
#include

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

Solution

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

Code Snippets

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

Context

StackExchange Code Review Q#60398, answer score: 6

Revisions (0)

No revisions yet.