patterncppMinor
Sending email using libcurl
Viewed 0 times
libcurlsendingusingemail
Problem
I developed a function to send email based on this. Does this code have problems? Also, I'm not sure if this code is thread-safe because of the global
```
#include
#include
static std::string payloadText[11];
std::string dateTimeNow();
std::string generateMessageId();
void setPayloadText(const std::string &to,
const std::string &from,
const std::string &cc,
const std::string &nameFrom,
const std::string &subject,
const std::string &body)
{
payloadText[ 0] = "Date: " + dateTimeNow();
payloadText[ 1] = "To: \r\n";
payloadText[ 2] = "From: (" + nameFrom + ")\r\n";
payloadText[ 3] = "Cc: (" + nameFrom + ")\r\n";
payloadText[ 4] = "Message-ID: \r\n";
payloadText[ 5] = "Subject: " + subject + "\r\n";
payloadText[ 6] = "\r\n";
payloadText[ 7] = body + "\r\n";
payloadText[ 8] = "\r\n";
payloadText[ 9] = "\r\n"; // "It could be a lot of lines, could be MIME encoded, whatever.\r\n";
payloadText[10] = "\r\n"; // "Check RFC5322.\r\n";
}
std::string dateTimeNow()
{
static const char *DAY_NAMES [] = { "Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat" };
static const char *MONTH_NAMES[] = { "Jan", "Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec" };
const int RFC1123_TIME_LEN = 29;
time_t t;
struct tm tm;
std::string ret;
ret.resize(RFC1123_TIME_LEN);
time(&t);
gmtime_s(&tm, &t);
strftime(&ret[0], RFC1123_TIME_LEN + 1, "---, %d --- %Y %H:%M:%S GMT", &tm);
memcpy(&ret[0], DAY_NAMES [tm.tm_wday], 3);
memcpy(&ret[8], MONTH_NAMES[tm.tm_mon], 3);
return ret;
}
std::string generateMessageId()
{
const int MESSAGE_ID_LEN = 37;
time_t t;
struct tm tm;
std::string ret;
ret.resize(15);
time(&t);
gmtime_s(&tm, &t);
strftime(const_cast(ret.c_str()),
MESSAGE_ID_LEN,
"%Y%m%d%H%
payloadText variable.```
#include
#include
static std::string payloadText[11];
std::string dateTimeNow();
std::string generateMessageId();
void setPayloadText(const std::string &to,
const std::string &from,
const std::string &cc,
const std::string &nameFrom,
const std::string &subject,
const std::string &body)
{
payloadText[ 0] = "Date: " + dateTimeNow();
payloadText[ 1] = "To: \r\n";
payloadText[ 2] = "From: (" + nameFrom + ")\r\n";
payloadText[ 3] = "Cc: (" + nameFrom + ")\r\n";
payloadText[ 4] = "Message-ID: \r\n";
payloadText[ 5] = "Subject: " + subject + "\r\n";
payloadText[ 6] = "\r\n";
payloadText[ 7] = body + "\r\n";
payloadText[ 8] = "\r\n";
payloadText[ 9] = "\r\n"; // "It could be a lot of lines, could be MIME encoded, whatever.\r\n";
payloadText[10] = "\r\n"; // "Check RFC5322.\r\n";
}
std::string dateTimeNow()
{
static const char *DAY_NAMES [] = { "Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat" };
static const char *MONTH_NAMES[] = { "Jan", "Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec" };
const int RFC1123_TIME_LEN = 29;
time_t t;
struct tm tm;
std::string ret;
ret.resize(RFC1123_TIME_LEN);
time(&t);
gmtime_s(&tm, &t);
strftime(&ret[0], RFC1123_TIME_LEN + 1, "---, %d --- %Y %H:%M:%S GMT", &tm);
memcpy(&ret[0], DAY_NAMES [tm.tm_wday], 3);
memcpy(&ret[8], MONTH_NAMES[tm.tm_mon], 3);
return ret;
}
std::string generateMessageId()
{
const int MESSAGE_ID_LEN = 37;
time_t t;
struct tm tm;
std::string ret;
ret.resize(15);
time(&t);
gmtime_s(&tm, &t);
strftime(const_cast(ret.c_str()),
MESSAGE_ID_LEN,
"%Y%m%d%H%
Solution
Here are some things that may help you improve your code.
Use required
The code invokes
First, as noted in http://en.cppreference.com/w/c/chrono/gmtime :
As with all bounds-checked functions,
It's also not guaranteed be thread safe, so you might want to use the POSIX
Don't write C code in C++
This is basically C code written with a little bit of C++. It could be greatly improved by using more of a C++ approach, as by using a class to represent an email and having most of the functions implemented as member functions. For example, I did a fairly simple refactoring of this code and came up with this class interface:
Fix the bugs
The code does not currently format a message correctly because there is no
Use C++11 version of
If you set the
Eliminate global variables
There is absolutely no reason that
The reworked
And finally, to use it within
(Note that this assumes that
Then the options to be able to use these together would look like this:
Use
Instead of this:
You could use this:
Use required
#includesThe code invokes
gmtime_s, memcpy and strlen but doesn't have the required headers that define those. The code should include these lines:#include
#include First, as noted in http://en.cppreference.com/w/c/chrono/gmtime :
As with all bounds-checked functions,
gmtime_s is only guaranteed to be available if __STDC_LIB_EXT1__ is defined by the implementation and if the user defines __STDC_WANT_LIB_EXT1__ to the integer constant 1 before including time.h.It's also not guaranteed be thread safe, so you might want to use the POSIX
gmtime_r instead.Don't write C code in C++
This is basically C code written with a little bit of C++. It could be greatly improved by using more of a C++ approach, as by using a class to represent an email and having most of the functions implemented as member functions. For example, I did a fairly simple refactoring of this code and came up with this class interface:
class email
{
public:
email(const std::string &to,
const std::string &from,
const std::string &nameFrom,
const std::string &subject,
const std::string &body,
const std::string &cc = ""
);
std::string dateTimeNow() const;
CURLcode send(const std::string &url,
const std::string &username,
const std::string &password);
private:
// data
std::string to_, from_, cc_, nameFrom_, subject_, body_;
// functions
std::string setPayloadText();
std::string generateMessageId() const;
// static functions
static size_t payload_source(void *ptr, size_t size, size_t nmemb, void *userp);
};Fix the bugs
The code does not currently format a message correctly because there is no
"\r\n" after the date string. It also does not work if there is no "CC" address. Please read the relevant RFCs (e.g. RFC 5322) to understand how an email is properly formatted. For example, the time string format is old. Instead of using the time zone abbreviation "GMT", the preferred format is to use the timezone offset. For that reason, you could just as easily use localtime rather than gmtime.Use C++11 version of
strftimeIf you set the
locale appropriately, you can eliminate all of the day and month parts from dateTimeNow() and just use this:std::string dateTimeNow()
{
const int RFC5322_TIME_LEN = 32;
time_t t;
struct tm *tm;
std::string ret;
ret.resize(RFC5322_TIME_LEN);
time(&t);
tm = localtime(&t);
strftime(&ret[0], RFC5322_TIME_LEN, "%a, %d %b %Y %H:%M:%S %z", tm);
return ret;
}Eliminate global variables
There is absolutely no reason that
payloadText needs to be 11 strings and no reason that it needs to be global. The last parameter of the payload_source is intended to be used for user data. One way to do that would be to define your own class like this:class stringdata {
public:
std::string msg;
size_t bytesleft;
stringdata(std::string &&m)
: msg{m}, bytesleft{msg.size()}
{}
stringdata(std::string &m) = delete;
};The reworked
payload_source would be this:static size_t payload_source(void *ptr, size_t size, size_t nmemb, void *userp)
{
stringdata *text = reinterpret_cast(userp);
if ((size == 0) || (nmemb == 0) || ((size*nmemb) bytesleft == 0)) {
return 0;
}
if ((nmemb * size) >= text->msg.size()) {
text->bytesleft = 0;
return text->msg.copy(reinterpret_cast(ptr), text->msg.size());
}
return 0;
}And finally, to use it within
sendEmail we can initialize the data like this:stringdata textdata{setPayloadText(to, from, cc, nameFrom, subject, body)};(Note that this assumes that
setPayloadText is rewritten to return a single std::string -- it's a fairly trivial rewrite.)Then the options to be able to use these together would look like this:
curl_easy_setopt(curl, CURLOPT_READFUNCTION, payload_source);
curl_easy_setopt(curl, CURLOPT_READDATA, &textdata);Use
iostreams rather than printfInstead of this:
if (ret != CURLE_OK) {
fprintf(stderr, "curl_easy_perform() failed: %s\n", curl_easy_strerror(ret));
}You could use this:
if (ret != CURLE_OK) {
std::cerr << "curl_easy_perform() failed: "
<< curl_easy_strerror(ret)
<< "\n";
}Code Snippets
#include <ctime>
#include <cstring>class email
{
public:
email(const std::string &to,
const std::string &from,
const std::string &nameFrom,
const std::string &subject,
const std::string &body,
const std::string &cc = ""
);
std::string dateTimeNow() const;
CURLcode send(const std::string &url,
const std::string &username,
const std::string &password);
private:
// data
std::string to_, from_, cc_, nameFrom_, subject_, body_;
// functions
std::string setPayloadText();
std::string generateMessageId() const;
// static functions
static size_t payload_source(void *ptr, size_t size, size_t nmemb, void *userp);
};std::string dateTimeNow()
{
const int RFC5322_TIME_LEN = 32;
time_t t;
struct tm *tm;
std::string ret;
ret.resize(RFC5322_TIME_LEN);
time(&t);
tm = localtime(&t);
strftime(&ret[0], RFC5322_TIME_LEN, "%a, %d %b %Y %H:%M:%S %z", tm);
return ret;
}class stringdata {
public:
std::string msg;
size_t bytesleft;
stringdata(std::string &&m)
: msg{m}, bytesleft{msg.size()}
{}
stringdata(std::string &m) = delete;
};static size_t payload_source(void *ptr, size_t size, size_t nmemb, void *userp)
{
stringdata *text = reinterpret_cast<stringdata *>(userp);
if ((size == 0) || (nmemb == 0) || ((size*nmemb) < 1) || (text->bytesleft == 0)) {
return 0;
}
if ((nmemb * size) >= text->msg.size()) {
text->bytesleft = 0;
return text->msg.copy(reinterpret_cast<char *>(ptr), text->msg.size());
}
return 0;
}Context
StackExchange Code Review Q#139784, answer score: 6
Revisions (0)
No revisions yet.