3
\$\begingroup\$

This simple program uses a socket class I wrote, class socket, to retrieve a users pop3 emails and print to stdout. I would like feedback on the socket class and the code to download the pop3 emails. My main focus is on the simple state machine to go through the steps required to download the messages. It looks a bit ugly. There are probably better ways to do it.

main.cpp:

#include <iostream>
#include <algorithm> 
#include <sstream>  

#include "socket.hpp"

bool message_ok(int result, const char* buffer) {
    return result != -1 && result > 3 && strncmp(buffer, "+OK", 3) == 0;
}

int main(int argc, char* argv[]) {

    for (int i = 1; i < argc; ++i)
        std::cout << "param: " << i << ' ' << argv[i] << '\n';

    if (argc != 5) {
        std::cout << "Usage pop3puller hostname port username pop3password\n";
        return 0;
    }


    itl::socket sock;

    std::cout << "Attempting to open connection\n";

    int ret = sock.connect(argv[1], atoi(argv[2]));

    std::cout << "Connection " << (ret == 0 ? "successfully opened\n" : "failed!\n");

    const int MAX_BUFFER = 4096;
    char buffer[MAX_BUFFER+1] = { 0 };
    ret = sock.recv(buffer, MAX_BUFFER);

    std::for_each(buffer, buffer + ret, [](const char ch) { std::cout << ch; });

    // If pop3 server presents usual prompt, start process
    if (message_ok(ret, buffer)) {
        // send USER <username>
        std::string user("USER ");
        user += argv[3];
        user += "\r\n";
        ret = sock.send(user.c_str(), user.length());
        ret = sock.recv(buffer, MAX_BUFFER);

        if (message_ok(ret, buffer)) {
            // send PASS <password>
            std::string pw("PASS ");
            pw += argv[4];
            pw += "\r\n";
            ret = sock.send(pw.c_str(), pw.length());
            ret = sock.recv(buffer, MAX_BUFFER);
            std::for_each(buffer, buffer + ret, [](const char ch) { std::cout << ch; });

            if (message_ok(ret, buffer)) {
                // LIST to get number of messages
                ret = sock.send("LIST\r\n", strlen("LIST\r\n"));
                ret = sock.recv(buffer, MAX_BUFFER);
                std::for_each(buffer, buffer + ret, [](const char ch) { std::cout << ch; });

                if (message_ok(ret, buffer)) {
                    // +OK 2 messages(744416 octets)
                    // grab no. messages available
                    // discard +OK characters and whitespace
                    if (ret > 4) {
                        std::string msg(buffer + 4);
                        size_t start = msg.find_first_of("0123456789");
                        size_t end = msg.find_first_not_of("0123456789");

                        if (start != std::string::npos && end != std::string::npos) {
                            std::string number(buffer + 4 + start, buffer + 4 + end);
                            int num = atoi(number.c_str());

                            for (int i = 1; i <= num; ++i) {
                                std::stringstream strm;
                                strm << "RETR " << i << "\r\n";
                                ret = sock.send(strm.str().c_str(), strm.str().length());

                                // assume that if bytes receieved == max buffer, then more to come
                                // problem if message is EXACTLY MAX_BUFFER in size :(
                                // Consider getting size of each message and reading until this no. bytes
                                do
                                {
                                    ret = sock.recv(buffer, MAX_BUFFER);
                                    std::for_each(buffer, buffer + ret, [](const char ch) { std::cout << ch; });

                                } while (ret == MAX_BUFFER);
                            }
                        }
                    }
                }
            }
        }
    }

    sock.close();

    std::cout << "Connection now closed\n";
}

socket.hpp:

#ifndef SOCKET_HPP_INCLUDED_
#define SOCKET_HPP_INCLUDED_
/* blocking client socket class. */

namespace itl {

// forward declare pimpl
struct socketimpl;

class socket  
{
   public:
      socket();
      virtual ~socket();
      int close();
      int shutdown();
      int connect(const char* host, unsigned port);
      int send(const char* data, int length);
      int recv(char* buf, int len);

   private :
       int init();
       socketimpl *pimpl_;  // Handle object

      // No copies do not implement
      socket(const socket &rhs);
      socket &operator=(const socket &rhs);
};

}  // itl namespace

#endif  // SOCKET_HPP_INCLUDED_

winsocket.hpp:

#include "socket.hpp"

#ifdef WIN32

#ifdef _MSC_VER
#pragma warning(disable:4786)
#endif

#ifndef WIN32_LEAN_AND_MEAN
#define WIN32_LEAN_AND_MEAN  // required for winsock2.h
#endif

#include <winsock2.h>
#pragma comment(lib,"ws2_32.lib")  //winsock2 lib


#ifndef SOCKET  //because winsock2.h does not define it (winsock.h does)
#define SOCKET u_int
#endif

#ifndef INVALID_SOCKET
#define INVALID_SOCKET    ~(0)
#endif


#include <ws2tcpip.h>  // getaddrinfo

#include <iostream>

namespace itl {

// hide implementation details from header
struct socketimpl
{
public:
    SOCKET socket_ = INVALID_SOCKET;
};


socket::socket()
{
    pimpl_ = new socketimpl;
    init();
}

socket::~socket()
{
    if (pimpl_->socket_ != INVALID_SOCKET) {
        shutdown();
        // should call recv here?
        close();
    }

#ifdef WIN32
    WSACleanup();
#endif

    delete pimpl_;
    std::cout << "socket class dtor\n";
}

int socket::init() {
#ifdef WIN32
    WSADATA w = { 0 }; 
    WSAStartup(0x0202, &w);
    int error = WSAStartup(0x0202, &w);

    if (error)
    { // there was an error
        return error;
    }
    if (w.wVersion != 0x0202)
    { // wrong WinSock version!
        WSACleanup(); // unload ws2_32.dll
        return INVALID_SOCKET;
    }
#endif

    // is this a good idea?
    pimpl_->socket_ = ::socket(AF_INET, SOCK_STREAM, IPPROTO_TCP); // Create socket

    std::cout << (pimpl_->socket_ == INVALID_SOCKET ? "Invalid" : "Valid") << " socket created\n";

    return pimpl_->socket_ != INVALID_SOCKET;
}

int socket::close() {
    int ret = ::closesocket(pimpl_->socket_);
    pimpl_->socket_ = INVALID_SOCKET;
    return ret;
}

int socket::shutdown() {
    // SD_SEND says to server, we have no more data to send
    // server may respond with response data
    std::cout << "sending shutdown message to remote end\n";
    return ::shutdown(pimpl_->socket_, SD_SEND);
}

int socket::connect(const char* host, unsigned port) {

    struct addrinfo *result = NULL;
    struct addrinfo hints;

    struct sockaddr_in * target = NULL;
    memset(&hints, 0, sizeof(hints));
    hints.ai_family = AF_UNSPEC;
    hints.ai_socktype = SOCK_STREAM;
    hints.ai_protocol = IPPROTO_TCP;

    int ret = getaddrinfo(host, NULL, &hints, &result);
    if (ret != 0) {
        std::cout << "getaddrinfo failed with error: " << ret << '\n';
        WSACleanup();
        return 1;
    }

    // use first address if available
    if (result) {
        switch (result->ai_family) {
        case AF_INET:
            target = (struct sockaddr_in *) result->ai_addr;
            target->sin_port = htons(port);
            break;
        case AF_INET6:
            std::cout << "Not supporting IpV6 just yet\n";
            return INVALID_SOCKET;
            break;
        default:
            std::cout << "Not IPv4 or IPv6 - problem!\n";
            return INVALID_SOCKET;
            break;
        }
    }

    ret = ::connect(pimpl_->socket_, reinterpret_cast<sockaddr *>(target), sizeof(sockaddr));

    freeaddrinfo(result);

    return ret;
}

int socket::send(const char* data, int length) {
    return ::send(pimpl_->socket_, data, length, 0);
}


int socket::recv(char* buf, int len) {
    return ::recv(pimpl_->socket_, buf, len, 0);
}

#endif  // WIN32

} // namespace itl
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

I would organize this code somewhat differently. But, I'm going to start with some rather minor nits, and get to the more serious "stuff" later.

I'd start by adding an overload to your socket::send that takes an std::string as its argument so you can clean up the rest of your code a little bit:

int socket::send(std::string const &s) { 
    return send(s.str(), s.length());
}

This way when you need to send a string, it'll be just:

sock.send(s);

...instead of the current code like:

sock.send(s.str(), s.length());

Likewise, I'd add an overload of recv that takes an array by reference, so the compiler can determine the array's size without your having to pass it explicitly:

template <size_t N>
int recv(char (&array)[N]) { 
    return recv(array, N);
}

So when you need to receive some data, it'll just look like:

ret = sock.recv(buffer);

Then I'd rewrite msg_ok a bit. Right now it has:

return result != -1 && result > 3 && strncmp(buffer, "+OK", 3) == 0;

This appears to me to be logically redundant: a result that's greater than 3 can't possibly be equal to -1, so we might as well just eliminate the first test:

 return result > 3 && strncmp(buffer, "+OK", 3) == 0;

Then we get to the real meat of things: reorganizing the code. I'd start this by creating a vector of commands that need to be sent to the server (since they're all basically similar--strings to send to the server). Assuming we can use C++14, that can look something like this:

std::vector<std::string> commands { 
    {"USER "s + argv[3] + "\r\n"}, 
    {"PASS "s + argv[4] + "\r\n"},
    {"LIST\r\n")
};

[Without C++14, we need to change strings like "USER "s to std::string("USER ").]

Then I'd write a small engine to step through that vector, send each command to the server, and check the result:

for (auto const &c : commands) {
    sock.send(c);
    ret = sock.recv(buffer);
    if (!msg_ok(ret, buffer))
        throw std::runtime_error("Error retrieving mail");
    std::copy_n(buffer, ret, std::ostream_iterator<char>(std::cout));
}

This separates two things that really are fundamentally separate: the sequence of commands we need to send to the server to retrieve mail, and the sequence of operations we need to carry out on the client to send each command, and then check and display the results.

Another possibility that might be worth considering would be to have a vector of functions that built and processed commands on the fly. This would have the advantage of making it relatively easy to process the RETR commands in roughly the same way as the others.

As far as your socket class goes, I've commented elsewhere on a roughly similar design. I think those comments apply about equally here.

\$\endgroup\$
2
  • \$\begingroup\$ I would like to investigate the C++14 "USER "s feature. What is the name of this feature? any links? \$\endgroup\$
    – arcomber
    Oct 16, 2015 at 10:12
  • \$\begingroup\$ These are user defined literals. \$\endgroup\$ Oct 16, 2015 at 15:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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