Parsing httpline

Question

I have an HTTP server class (.hpp & cpp). I am trying to improve reserve data from the socket class because I have s.getline() to get HTTP call. My s.RecvData() gets data line by line in an infinite loop to build HTTP struct.

This is my old code:

while (1) {
    line = s.RecvData();
    //httpline  = s.Getline();
    if (line.empty()) break;
    //find location of tab"\t"
    int location_tab_char = line.find_first_of(" ");
    int loacation_End_chars = line.find_first_of("\r\n");
    if (loacation_End_chars == 0)
    {
        break;
    }
    if ("Host:" == line.substr(0, location_tab_char))
    {
        req.hostName_ = line.substr(0, loacation_End_chars);
    }
    else if ("Connection:" == line.substr(0, location_tab_char))
    {
        req.conn_ = line.substr(0, loacation_End_chars);
    }
    else    if ("Accept:" == line.substr(0, location_tab_char))
    {
        req.accept_ = line.substr(0, loacation_End_chars);
    }
    else if ("Accept-Language:" == line.substr(0, location_tab_char))
    {
        req.acceptLanguage_ = line.substr(0, loacation_End_chars);
    }
    else if ("Accept-Encoding:" == line.substr(0, location_tab_char))
    {
        req.acceptEncoding_ = line.substr(0, loacation_End_chars);
    }
    else if ("User-Agent:" == line.substr(0, location_tab_char))
    {
        req.userAgent_ = line.substr(0, loacation_End_chars);
    }
}

I just need a way to improve it because getting data from the socket line by line is too much calling. Also, when I pass to struct, I pass this string "Host: 169.254.80.80:8080"

"Host: 169.254.80.80:8080\r\nConnection: keep-alive\r\nAccept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,/;q=0.8\r\nUser-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36\r\nAccept-Encoding: gzip,deflate,sdch\r\n Accept-Language: en-US,en;q=0.8,ar;q=0.6 ";

You should go for regular expressions to parse the header. You can check out other HTTP server's source code to see how they implement that. (with regex, I guess). — edmz
– edmz, Commented Apr 17, 2014 at 9:30

utnapistim · Accepted Answer · 2014-04-17 10:03:20Z

Your code can be made smaller and without repeated constructs, by mapping your keys to values in a map:

// #include <map>

std::map<std::string,std::string> headers;

while(true) { // better use true instead of 1

    line = s.RecvData();

    if (line.empty() || line == "\r\n\r\n")
        break;
    auto key_value_sep = line.find(":"); // do not split by space

    // you could stop here in case of an exception in the data
    // if(0 == key_value_sep || std::string::npos == key_value_sep)
    //     throw std::runtime_error{"..."}; // or break, or whatever

    // line is a single line; as such, end chars location not needed
    // int loacation_End_chars = line.find_first_of("\r\n");

    auto key = line.substr(0, key_value_separator);

    auto value = line.substr(key_value_separator + 2, // skip ": "
                             line.size() - key.size() - 4); // size less key,
                                                            // separator and
                                                            // "\r\n"
    // you could break here in case of an exception in the data
    // if (value.empty() || key.empty()) || ...)
    //     throw std::runtime_error{"..."}; // or break, or whatever

    // set header 
    headers[key] += value;
}

// set headers (or use directly from map)
req.hostName_ = headers["Host"];
req.conn_ = headers["Connection"];
req.accept_ = headers["Accept"];
req.acceptLanguage_ = headers["Accept-Language"];
req.acceptEncoding_ = headers["Accept-Encoding"];
req.userAgent_ = headers["User-Agent"];

Morwenn · Accepted Answer · 2014-04-17 20:24:01Z

First of all, a few notes about style:

You probably have a typo in loacation_End_chars. I think that you meant location_End_chars instead.
Also, you should be consistent with location_tab_char and use snake_case everywhere without any capital. Therefore, loacation_End_chars should actually be named location_end_chars.

Your program may compute line.substr(0, location_tab_char) an awful lot of time if it has to "User-Agent:", which may be extremely inefficient. It is bad generally bad to repeat code when it could be avoided. Here, a solution would be to compute line.substr(0, location_tab_char) and line.substr(0, location_end_char) only once before your conditions:

std::string first = line.substr(0, location_tab_char);
std::string second = line.substr(0, location_end_chars);
if ("Host:" == first)
{
    req.hostName_ = second;
}
else if ("Connection:" == first)
{
    req.conn_ = second;
}
// etc...

Unfortunately, I have no idea what would be good names for these variables, so I named them first and second, but you should find a relevant and meaninful name for them instead.

Another way to simplify your code would be to map directly the strings to the fields of your struct thanks to a std::reference_wrapper:

std::map<std::string, std::reference_wrapper<std::string>> mapping = {
    { "Host:", req.hostName_ },
    { "Connection:", req.conn_ },
    { "Accept:", req.accept_ },
    { "Accept-Language:", req.acceptLanguage_ },
    { "Accept-Encoding:", req.acceptEncoding_ },
    { "User-Agent:", req.userAgent_ },    
};

Then your code would become:

while (true) {
    line = s.RecvData();
    //httpline  = s.Getline();
    if (line.empty()) break;
    //find location of tab"\t"
    int location_tab_char = line.find_first_of(" ");
    int location_end_chars = line.find_first_of("\r\n");
    if (location_end_chars == 0)
    {
        break;
    }

    auto key = line.substr(0, location_tab_char);
    auto value = line.substr(0, location_end_chars);
    auto it = mapping.find(key);
    if (it != mapping.end())
    {
        it->second.get() = value;
    }
}

Some additional notes (thanks to @utnapistim comment):

std::map::operator[] invokes the default constructor of the value type. However, std::reference_wrapper does not have a default constructor, that's why I used the method at instead (contrary to operator[], at cannot create an element, that's why we need to check for the existence of the element beforehand with find).
Of course, you header instance needs to live in a greater scope than the std::map instance, otherwise, trying to access the elements of mapping will trigger undefined behavior.

A note about the std::reference_wrapper use: make sure the header you are mapping has a bigger scope than the map (otherwise you end up with UB). Also, the use of std::reference_wrapper in a map imposes that you always check for existence of the value before assignment (as you did - which is non-obvious unless you know the behavior of std::map::operator[]). — utnapistim
– utnapistim, Commented Apr 17, 2014 at 10:38
@utnapistim Yeah, std::map::operator[] always crashes with std::reference_wrapper since it does not have a default constructor. I will add a note about that, thanks :) — Morwenn
– Morwenn, Commented Apr 17, 2014 at 11:51

Stack Exchange Network

Parsing httpline

2 Answers 2

You must log in to answer this question.

Hot Network Questions

Parsing httpline

2 Answers 2

You must log in to answer this question.

Related

Hot Network Questions