IntroA couple of weeks ago I finished a Python implementation of a multithreaded port checker and I wasn't quite happy with the result I've got (speed). I needed it to be faster, so I've built another one using CPP (and a bit of C). Here is the Python code, if anyone wants to see it.At the moment, I didn't implemented the threading module for my script because I'd like to get some advice on what would be the wisest choice (while asking for an answer which adds this part is forbidden here, I wouldn't mind seeing an example of it)DescriptionThe program takes as command line arguments the following:a text file which contains one domain name per linethe port number for which the above domains will be checkednumber of threadsthe timeouta text file where the domains which have the port open will be written (along with their IP address) e.g: google.com:172.217.22.46What the program is currently doing is to translate each domain into an IP address, and check if the given port is opened or not. If it's opened, write the domain:ip to an output file.The program can be compiled and run using the below commands:g++ port_checker.cpp -o checker./checker domains.txt 80 2 2 output.txtCode#include <iostream>#include <fstream>#include <vector>#include <sys/socket.h>#include <netdb.h>#include <arpa/inet.h>#include <unistd.h>std::vector<std::string> get_domains_from_file(std::string domains_file) {    std::vector<std::string> domains_array;    std::ifstream file(domains_file);    std::string domain;    if (!file.is_open()) {        std::cerr << Unable to open domains file! << std::endl;        std::exit(-1);    }    while (file >> domain) {        domains_array.push_back(domain);    }    return domains_array;}int main(int argc, char *argv[]) {    struct hostent *h;    struct sockaddr_in servaddr;    int sd, rval;    if (argc != 6) {        std::cerr << The number of arguments should be 5 e.g: ./checker domains.txt [port] [threads] [timeout] output.txt << std::endl;        std::exit(-1);    }    std::string domains_file = argv[1];    std::string output_file  = argv[5];    int port                 = atoi(argv[2]);    int threads              = atoi(argv[3]);    int timeout              = atoi(argv[4]);    std::vector<std::string> domains = get_domains_from_file(domains_file);    std::ofstream myfile;    myfile.open(output_file);    for (int i = 0; i < domains.size(); i++) {        if ((h=gethostbyname(domains[i].c_str())) == NULL) {            std::cerr << Error when using gethostbyname << std::endl;            std::exit(-1);        }        // std::cout << inet_ntoa(*((struct in_addr *)h->h_addr)) << std::endl;        sd = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP);        if (sd == -1) {            std::cout << Error when trying to create socket ! << std::endl;            std::exit(-1);        }        memset(&servaddr, 0, sizeof(servaddr));        servaddr.sin_family = AF_INET;        servaddr.sin_port = htons(port);        memcpy(&servaddr.sin_addr, h -> h_addr, h -> h_length);        rval = connect(sd, (struct sockaddr *) &servaddr, sizeof(servaddr));        if (rval == -1) {            // std::cout << Port is closed for domain:  << domains[i] << std::endl;            continue;        }        else {            myfile << domains[i] << : << inet_ntoa(*((struct in_addr *)h->h_addr))  << std::endl;        }        close(sd);    }    return 0;}What I'd like the most to get out of this review is:how can I improve the performance ? (that's what concerns me first)is there any other way of checking for an opened port ?how can my code be improved in terms of readability / best practices ?NOTE: Bear that this is my first C++ program in a very long time, so make sure you add as much information as you can in your review.
Simple and effective port checker in C++
c++;performance;networking;socket;tcp
Checking TCP ports and multi-threadingYour performance should be fine for a sequential program. Another way to check whether the TCP port is open is by just sending the initial SYN and wait for the ACK. That way, you don't have to do the complete TCP handshake. However, that also means that you have to implement that part of the TCP protocol yourself. Also, some platforms don't allow using raw IP sockets for that for restricted users, which can pose another problem.However, you cannot implement multi-threading at the moment, at least not over your current for loop. gethostbyname is not thread-safe. You have to use another function there, otherwise you will end up with data races and therefore undefined behaviour. GNU provides gethostbyaddr_r, which is an extension of the POSIX standard.That being said, let's have a look at your current code.Proper includes#include <fstream>#include <vector>#include <sys/socket.h>#include <netdb.h>#include <arpa/inet.h>#include <unistd.h>You're missing several includes. cout and cerr need <iostream>, memcpy and memset need <cstring>, exit needs cstdlib.Be more user friendlyWe can use your get_domains_from_file as a case-study:std::vector<std::string> get_domains_from_file(std::string domains_file) {    std::vector<std::string> domains_array;    std::ifstream file(domains_file);    std::string domain;    if (!file.is_open()) {        std::cerr << Unable to open domains file! << std::endl;        std::exit(-1);    }    while (file >> domain) {        domains_array.push_back(domain);    }    return domains_array;}At the moment, you simply exit(-1) whenever you encounter a problem. I see two problems there:The exit code doesn't yield any information, apart from it worked or it failedYou cannot recover the error, for example if you want to keep running even if you failed to create a socket for the 6000th domain.Consider alternative input and output streamsAlso, since we're currently in UNIX land, your program isn't very pipe-friendly. One can only get the domains from a file, but in a real-world scenario, it's likely that you have some logs which you want to check, e.g.grep acme /var/log/attacks | ./domain-checker 80 4 2We can at least make your program pipe-ready, if we use an istream here:std::vector<std::string> get_domains_from_stream(std::istream & in) {    std::vector<std::string> domains_array;    std::string domain;    while (in >> domain) {        domains_array.push_back(domain);    }    return domains_array;}You can implement your other function with get_domains_from_stream:std::vector<std::string> get_domains_from_file(std::string domains_file) {    std::ifstream file(domains_file);    if (!file.is_open()) {        std::cerr << Unable to open domains file! << std::endl;        return {};    } else {        return get_domains_from_stream(file);    }}Note that I've returned an empty vector in this case. It is more or less a point of preference here, whether to return an empty resource, throw an exception, or do something else. But from a user's point of view, the I've specified an empty file, whoops and I've specified a non-existing file, whoops are the same kind of error: the user gave the wrong argument to your program.Better usage message and input handlingWhile we're at error messages, you could improve your argument usage error message:std::cerr << The number of arguments should be 5 e.g:           << arg[0] <<  domains.txt [port] [threads] [timeout] output.txt           << std::endl;You have no idea what the application is called on the user's PC, so just use arg[0] for that.Speaking of arguments, how does your program work with invalid input?$ ./a.out domains.txt abc def ghi blarg.outNo error message, no exception. That's due to atoi, which simply returns 0. Prefer std::stoi instead, which throws an exception if something is off:struct hostent *h;             // hm, what are these?struct sockaddr_in servaddr;   // where do you use them?int sd, rval;                  // what are these ints?const std::string domains_file = argv[1];const std::string output_file  = argv[5];const int port                 = std::stoi(argv[2]);const int threads              = std::stoi(argv[3]);const int timeout              = std::stoi(argv[4]);By the way, my criticism about the input also holds for your output. cout is usually fine for output. If a user wants to write to a file, they can just use redirection:$ grep acme.evil | ./checker 80 4 2 > evil.ipLimit the scope of your variables as much as possibleStill in the same section of your program, you define int sd, int rval, struct hostent *h; and other's far too early. You always try to limit the scope of your variables. Therefore, get rid of them at that point. Also, use const a little bit more often to ensure that you don't change things. It will also help the compiler to optimize your program in several cases. Not necessarily in your current one, but it's usually a good idea.The for-loopNow, after above discussion, let's have a look at your for loop. Since you use C++11, consider a range-based for-loop:for(const auto & domain : domains){    struct hostent * h = gethostbyname(domain.c_str());I don't think a std::exit is appropriate here. A small typo in the first line of your domain.txt shouldn't prevent the rest from getting checked. However, a little bit more information would be great:    if (h == nullptr) {        std::cerr << Error when using gethostbyname:                   << hstrerror(h_errno)                  <<  (on domain:  << domain << )                  << std::endl;        continue;    }Note that hstrerror and h_errno are considered obsolete, but so is gesthostbyname.A resource leakYou forgot to close sd if rval is -1. That can lead to a too many handles error, and you cannot continue to scan all those ports. One way to fix that would be not to use continue:if(rval != -1){    myfile << domains[i] << : << inet_ntoa(*reinterpret_cast<struct in_addr *>(h->h_addr))  << std::endl;}close(sd);However, that's also error prone. Enter RAII:class socket_wrapper {public:    explicit socket_wrapper(int fd) : m_fd(fd) {}    ~socket_wrapper(){         if(m_fd != -1) ::close(m_fd);    }    socket_wrapper() = delete;    socket_wrapper(const socket_wrapper &) = delete;    socket_wrapper & operator=(const socket_wrapper &) = delete;    socket_wrapper(socket_wrapper && other) : m_fd(other.m_fd) {        other.m_fd = -1;    }    socket_wrapper & operator=(socket_wrapper && other){        this->close();        m_fd = other.m_fd;        other.m_fd = -1;    }    bool is_valid() const { return m_fd != -1; }    void close() {        ::close(m_fd);        m_fd = -1;    }    int fd() const { return m_fd; }private:    int m_fd;};This is basically a wrapper around an int. However, the most important part is that it will automatically call close on that int, so you cannot forget it accidentally. This will make your code more robust. Note that the move operations are just a small bonus and for this an overkill. You probably won't need them. Still, it's important to forbid copying that wrapper. We end up with:    const auto sd = static_cast<socket_wrapper>(socket(PF_INET, SOCK_STREAM, IPPROTO_TCP));    if (sd.is_valid()) {        std::cout << Error when trying to create socket ! << std::endl;        std::exit(-1);    }    memset(&servaddr, 0, sizeof(servaddr));    servaddr.sin_family = AF_INET;    servaddr.sin_port = htons(port);    memcpy(&servaddr.sin_addr, h -> h_addr, h -> h_length);    rval = connect(sd.fd(), reinterpret_cast<struct sockaddr *>(&servaddr), sizeof(servaddr));    if (rval == -1) {        // std::cout << Port is closed for domain:  << domains[i] << std::endl;        continue;    }            myfile << domains[i] << : << inet_ntoa(*reinterpret_cast<struct in_addr *>(h->h_addr))  << std::endl;We don't have to worry that we forgot to close our socket, since it will now get closed automatically. Note that the generated code will be pretty much the same as your current one. It's just safer.Use the proper castYou probably saw it already in the code above. Use the proper cast. A C-style cast should be the last resort.A remark on performanceThere are two bottlenecks in your program:Getting the address info for a domain name.Connecting to the given address.Note that you can get address info on the other domains while you're connecting. So for a first implementation of multi-threading, you could get the address info in one thread, and connect in another. Connect them via a thread-safe, closable queue.However, due to its blocking nature, the connect call may block for some time if the port isn't open. If you want to stay in POSIX land, you have to resort to multiple threads connecting at once. A worker pool might be suitable for this. Certain libraries like Boost.ASIO provide you asynchronous IO operations, which make this easier. Boost also provides thread-safe lockfree queues, so that you don't have to implement them yourself, if you're really racing for the last bit of performance.But as always: before you start to optimize, profile your program. For a sequential program, there's not really anything else you can do at the moment. You read a single domain name, you get its network address, you try to connect, and you remember the result. And that's exactly what you did.