Wrapping Curl, an HTTP client

Question

I have written an HTTP client wrapped the libcurl. It should be able to do HTTP get/post with string/map param, with cookies and proxy.

Can somebody review the code? B.T.W., I'm not sure the way pass a map into HTTP header is correct, maybe I should remove these two interface

CURLcode do_http_post(std::string post_url, std::map<std::string, std::string> map_param, void* user_data);

and

void set_http_header(std::map<std::string, std::string> map_param);.

curl_wrapper.h

#ifndef CURL_WRAPPER
#define CURL_WRAPPER
#include <string>
#include <exception>
#include "curl.h"
#include <map>
#define DATA_MAX_LEN CURL_MAX_WRITE_SIZE*30
struct client_data;

class curl_client{
public:
    curl_client();
    ~curl_client();
    CURLcode do_http_get(std::string get_url, void* user_data);
    CURLcode do_http_post(std::string post_url, std::map<std::string, std::string> map_param, void* user_data);
    CURLcode do_http_post(std::string post_url, std::string post_fields, void* user_data);
    void set_http_header(std::string header_param);
    void set_http_header(std::map<std::string, std::string> map_param);
    void set_http_cookie(std::string cookie_file);
    void set_http_proxy(std::string proxy_url);

private:
    static size_t write_data( char *ptr, size_t size, size_t nmemb, void *user_data);
    void set_common_opt(std::string url, void* user_data);
    void set_post_fields(std::string post_fields);
    void set_post_fields(std::map<std::string, std::string> map_param);
    CURLcode perform();

private:
    CURL* curl_;
    CURLcode res_;
    curl_slist* p_header_list_;
};

struct client_data {
    client_data() {
        size = DATA_MAX_LEN;
        used = 0;
        buf = new char[DATA_MAX_LEN];
    }
    ~client_data(){
        if (buf!=nullptr){
            delete[] buf;
        }
    }
    char *buf;
    int size;
    int used;
};


#endif

curl_wrapper.cpp

#include "curl_wrapper.h"

curl_client::curl_client(){
    curl_ = curl_easy_init();
    p_header_list_ = nullptr;
}

curl_client::~curl_client(){
    curl_easy_cleanup(curl_);
    curl_slist_free_all(p_header_list_);
}

CURLcode curl_client::do_http_get(std::string get_url, void* user_data){
    set_common_opt(get_url, user_data);
    return perform();
}

CURLcode curl_client::do_http_post(std::string post_url, std::map<std::string, std::string> map_param, void* user_data){
    set_common_opt(post_url, user_data);
    set_post_fields(map_param);
    return perform();
}

CURLcode curl_client::do_http_post(std::string post_url, std::string post_fields, void* user_data){
    set_common_opt(post_url, user_data);
    set_post_fields(post_fields);
    return perform();
}

void curl_client::set_common_opt(std::string url, void* user_data){
    curl_easy_setopt(curl_, CURLOPT_URL, url.c_str());  
    curl_easy_setopt(curl_, CURLOPT_WRITEFUNCTION, curl_client::write_data);  
    curl_easy_setopt(curl_, CURLOPT_WRITEDATA, user_data);  
    curl_easy_setopt(curl_, CURLOPT_HEADER, 1); 
    curl_easy_setopt(curl_, CURLOPT_FOLLOWLOCATION, 1);  
    // not verify host and ca for https
    curl_easy_setopt(curl_, CURLOPT_SSL_VERIFYPEER, 0);
    curl_easy_setopt(curl_, CURLOPT_SSL_VERIFYHOST, 0);
#ifdef DEBUG
    curl_easy_setopt(curl_, CURLOPT_VERBOSE, 1);  
#endif
}

void curl_client::set_post_fields(std::map<std::string, std::string> map_param){
    std::string post_fields;
    std::map<std::string, std::string>::iterator it;
    for (it = map_param.begin(); it!=map_param.end(); ++it){
        if (it!=map_param.begin()){
            post_fields += "&";
        }
        post_fields += it->first + "=" + it->second;
    }
    set_post_fields(post_fields);  
}

void curl_client::set_post_fields(std::string post_fields){
    curl_easy_setopt(curl_, CURLOPT_POSTFIELDS, post_fields.c_str()); 
    curl_easy_setopt(curl_, CURLOPT_POST, 1);  
}

void curl_client::set_http_header(std::string header_param){
    if (p_header_list_){
        curl_slist_free_all(p_header_list_);
        p_header_list_ = nullptr;
    }
    p_header_list_ = curl_slist_append(p_header_list_, header_param.c_str());
    curl_easy_setopt(curl_, CURLOPT_HTTPHEADER, p_header_list_);
}

void curl_client::set_http_header(std::map<std::string, std::string> map_param){

    std::string header_param;
    std::map<std::string, std::string>::iterator it;
    for (it = map_param.begin(); it!=map_param.end(); ++it){
        if (it!=map_param.begin()){
            header_param += "&";
        }
        header_param += it->first + "=" + it->second;
    }
    set_http_header(header_param);
}

void curl_client::set_http_cookie(std::string cookie_file) {
    curl_easy_setopt(curl_, CURLOPT_COOKIEJAR, cookie_file.c_str());//save to
    curl_easy_setopt(curl_, CURLOPT_COOKIEFILE, cookie_file.c_str());//read from
}

void curl_client::set_http_proxy(std::string proxy_url){
    curl_easy_setopt(curl_, CURLOPT_PROXY, proxy_url.c_str());
}

size_t curl_client::write_data( char *ptr, size_t size, size_t nmemb, void *user_data){
    client_data *the_buf = (client_data *)user_data;
    int bytes_passed_in = size * nmemb;
    int bytes_written = 0;
    if (the_buf->used + bytes_passed_in < DATA_MAX_LEN){
        memcpy(the_buf->buf + the_buf->used, ptr, bytes_passed_in);
        the_buf->used += bytes_passed_in;
        *(the_buf->buf + the_buf->used) = 0;
        bytes_written = bytes_passed_in;
    }else {
        memcpy(the_buf->buf + the_buf->used, ptr, DATA_MAX_LEN - the_buf->used - 1);
        bytes_written = DATA_MAX_LEN - the_buf->used - 1;
        the_buf->used = DATA_MAX_LEN;
        *(the_buf->buf + DATA_MAX_LEN - 1) = 0;
        // here libcurl will signal an error for intact data written.
    }
    return bytes_written;
}

CURLcode curl_client::perform(){
    if (curl_) {
        res_ = CURL_LAST;
        try {
            res_ = curl_easy_perform(curl_);
        } catch (std::exception e){
        }
        return res_;
    }
}

An HTTP get test case:

 void curl_get_googleplay(){
        curl_client curl;
        client_data user_data;
        CURLcode res = curl.do_http_get(std::string("play.google.com/store/apps/details?id=com.teamviewer.teamviewer.market.mobile"), &user_data);
    }

Answer 1

Looking at your header file:

• Replace the #define DATA_MAX_LEN with a static const variable; Do the same with any other constant that is #define-d.

• Pass more complex parameters by const reference, to avoid making a copy (urls, parameters maps, etc).

• You define client_data structure for (I assume) receiving the results; If it is strongly typed, why do you pass it in by void*? This just allows client code to call your API in a way that will corrupt your code (e.g. a client might decide to pass there a pointer to a std::vector<uint8_t> instead).

• You do not use RAII and smart pointers (you probably should)

• After looking at your code, it is still unclear to me, if I would be able to use it to read the HTTP response headers (e.g. "I want to know if the response came with the "Cache-Control" header specified, and what was it's value").

• Consider returning the result data as a return value (instead as an output parameter) and raising an exception in case you get a HTTP error. This would allow you to specify other error conditions as well.

• Just looking at the interface (not the implementation) I have no idea how your class will behave if I call with invalid parameters.

Code:

curl_client curl;
client_data user_data;
curl.set_http_cookie("\\"); // does this throw? What does it throw?

Looking at your implementation file:

• Your perform function calls curl_easy_perform (a C function) inside a try/catch block for std::exception. Exceptions are a C++ thing (i.e. curl_easy_perform will never throw an exception -- that's why it's using return codes to signal errors).

• You are using C-style casts; Don't! (see my point above about removing the void* code).

• There is no reason at all to use raw pointer and memcpy. Consider using std::vector instead (it will be safer, more efficient, exception-safe and already tested).

• set_post_fields concatenates strings into a different string, in a loop. Consider using a std::ostringstream instead.