6
\$\begingroup\$

This is just some test code I'm doing to learn C++11. It compiles and runs consistently. But is anything wrong that will eventually (or under slightly different circumstances) fail?

Besides additional refactoring for sheer cleverness, are there any notable improvements that can be made to remove lines (like unnecessary type conversion chains)? Are there memory leaks or uninitialized memory? Some questions are noted in comments.

#include <iostream>
#include <fstream>
#include <memory>
#include <Windows.h>
#include <WinDNS.h>
#include <iomanip>
#include <string> // I keep specifying wstring and wcout.  I could macro it, but I don't want to.  Is it normal to specify w everywhere?
#include <sstream>
#include <exception>

using namespace std;

struct DnsRAII
{
    PDNS_RECORD p;

    DnsRAII() : p(NULL) { }
    ~DnsRAII() { if (p != NULL) DnsRecordListFree(p); }

private: // disallowing copy and assignment, right, because by default they would exist and if I allowed them I'd have to worry about shared references more, right?
    DnsRAII(const DnsRAII &p) { } 
    DnsRAII & operator= (const DnsRAII & p) { }
};

static wstring ipv4tos(IP4_ADDRESS addr)
{
    wstringstream ss;

    in_addr ip4;
    ip4.S_un.S_addr = addr;

    ss << to_wstring(ip4.S_un.S_un_b.s_b1) << "."
        << to_wstring(ip4.S_un.S_un_b.s_b2) << "."
        << to_wstring(ip4.S_un.S_un_b.s_b3) << "."
        << to_wstring(ip4.S_un.S_un_b.s_b4);

    return ss.str();
}

static wstring ipv4toptrq(IP4_ADDRESS addr)
{
    wstringstream ss;

    in_addr ip4;
    ip4.S_un.S_addr = addr;

    ss << to_wstring(ip4.S_un.S_un_b.s_b4) << "."
        << to_wstring(ip4.S_un.S_un_b.s_b3) << "."
        << to_wstring(ip4.S_un.S_un_b.s_b2) << "."
        << to_wstring(ip4.S_un.S_un_b.s_b1) << ".IN-ADDR.ARPA";

    return ss.str();
}

static wstring ipv6tos(IP6_ADDRESS addr)
{
    wstringstream ss;

    auto group = [&](int group) {
        if (!(addr.IP6Byte[group * 2] == 0 && addr.IP6Byte[group * 2] == addr.IP6Byte[(group * 2) + 1]))
            ss << hex << setfill(L'0')
            << setw(2) << (int)addr.IP6Byte[group * 2] << setw(2) << (int)addr.IP6Byte[(group * 2) + 1];
    };

    group(0); ss << ":";
    group(1); ss << ":";
    group(2); ss << ":";
    group(3); ss << ":";
    group(4); ss << ":";
    group(5); ss << ":";
    group(6); ss << ":";
    group(7); ss << ":";

    return ss.str();
}

static wstring rrtos(wstring &query, DnsRAII &results)
{
    wstringstream ss;

    for (auto current = results.p; current != NULL; current = current->pNext)
    {
        switch (current->wType)
        {
        case DNS_TYPE_MX:
            ss << query << " -> MX " << current->Data.Mx.pNameExchange << "\n";
            break;
        case DNS_TYPE_A:
            ss << query << " -> A " << ipv4tos(current->Data.A.IpAddress) << "\n";
            break;
        case DNS_TYPE_AAAA:
            ss << query << " -> AAAA " << ipv6tos(current->Data.AAAA.Ip6Address) << "\n";
            break;
        case DNS_TYPE_PTR:
            ss << query << " -> PTR " << current->Data.PTR.pNameHost << "\n";
            break;
        default:
            ss << query << " -> Record type " << current->wType << " unhandled\n";
            break;
        }
    }

    return ss.str();
}

static wstring topmx(DnsRAII &results)
{
    DNS_MX_DATA top;

    top.pNameExchange = 0;
    top.wPreference = ~0; // maxint - find ANYTHING lower than this

    for (auto current = results.p; current != NULL; current = current->pNext)
    {
        switch (current->wType)
        {
        case DNS_TYPE_MX:
            if (current->Data.MX.wPreference < top.wPreference)
                top = current->Data.Mx;
            break;
        default:
            break;
        }
    }

    if (top.pNameExchange != 0) return wstring(top.pNameExchange);

    throw exception("MX record not found");
}

int main(int argc, char **argv)
{
    cout << "Searching ... \n";

    auto options = DNS_QUERY_BYPASS_CACHE | DNS_QUERY_NO_LOCAL_NAME | DNS_QUERY_NO_HOSTS_FILE;

    in_addr dnsServer;
    dnsServer.S_un.S_un_b.s_b1 = 8; // Google's DNS 
    dnsServer.S_un.S_un_b.s_b2 = 8;
    dnsServer.S_un.S_un_b.s_b3 = 8;
    dnsServer.S_un.S_un_b.s_b4 = 8;

    IP4_ARRAY dnsServers;
    dnsServers.AddrCount = 1;
    dnsServers.AddrArray[0] = IP4_ADDRESS(dnsServer.S_un.S_addr); // I'm not sure I'm allocating this correctly

    auto name = wstring(L"stackoverflow.com");

    wcout << "\nDomain: " << name << " ... \n";

    DnsRAII results;
    if (0 == DnsQuery((PCWSTR)name.c_str(), DNS_TYPE_MX, options, &dnsServers, &results.p, NULL))
    {
        wcout << rrtos(name, results);
    }
    else
    {
        wcout << "Host not found";

        getchar();
        return 1;
    }

    wstring mx;

    try
    {
        mx = topmx(results);
    }
    catch (exception ex)
    {
        wcout << ex.what();

        getchar();
        return 1;
    }

    wcout << "\nMX: " << mx << " ... \n";

    DnsRAII results2;
    if (0 == DnsQuery((PCWSTR)mx.c_str(), DNS_TYPE_A, options, &dnsServers, &results2.p, NULL))
    {
        wcout << rrtos(mx, results2);
    }
    else
    {
        wcout << "Host not found";

        getchar();
        return 1;
    }

    wcout << "\nIP: " << ipv4tos(results2.p->Data.A.IpAddress) << " ... \n";

    wstring reverse = ipv4toptrq(results2.p->Data.A.IpAddress);

    DnsRAII results3;
    if (0 == DnsQuery((PCWSTR)reverse.c_str(), DNS_TYPE_PTR, options, &dnsServers, &results3.p, NULL))
    {
        wcout << rrtos(reverse, results3);
    }
    else
    {
        wcout << "PTR not found";

        getchar();
        return 1;
    }

    wcout << "\nSo \"" << name << "\" is run by \"" << results3.p->Data.PTR.pNameHost << "\"\n";

    wcout << "\nDone.\n";

    getchar();
    return 0;
}

Output is below:

Searching ...

Domain: stackoverflow.com ...
stackoverflow.com -> MX ASPMX2.GOOGLEMAIL.com
stackoverflow.com -> MX ASPMX3.GOOGLEMAIL.com
stackoverflow.com -> MX ASPMX.L.GOOGLE.com
stackoverflow.com -> MX ALT1.ASPMX.L.GOOGLE.com
stackoverflow.com -> MX ALT2.ASPMX.L.GOOGLE.com

MX: ASPMX.L.GOOGLE.com ...
ASPMX.L.GOOGLE.com -> A 173.194.68.26

IP: 173.194.68.26 ...
26.68.194.173.IN-ADDR.ARPA -> PTR qa-in-f26.1e100.net

So "stackoverflow.com" is run by "qa-in-f26.1e100.net"

Done.

("qa-in-f26.1e100.net" is a Google mail server)

\$\endgroup\$
2
  • \$\begingroup\$ Is this really working code? DnsRecordListFree is documented to take two parameters, but you only pass one in DnsRAII. \$\endgroup\$ Mar 15 '14 at 18:04
  • \$\begingroup\$ Yes it does work with only one. I think it's a default argument? \$\endgroup\$ Mar 15 '14 at 19:01
5
\$\begingroup\$

As it stands, the code is somewhat fragile. It only builds as a wide-character build (i.e., with UNICODE and/or _UNICODE defined). I'd prefer to see code explicitly define things that need to be wide characters as wide characters.

I'm also seeing the same problem with DnsFree that @Michael Urman mentioned in his comment: I need to pass a second parameter (DnsFreeRecordList) to DnsRecordListFree to get the compiler to accept the code.

I'm not overly fond of your DnsRAII class. First of all, I dislike the name. RAII is an implementation technique of which the client should be able to remain unaware. To the client, it should be something on the order of a DnsRecordCollection or (probably preferable) a DnsIterator.

Second, I don't like defining a private copy constructor and assignment operator to prevent copying and assignment. I'd prefer to either derive from Boost::noncopyable, or else (if you can use C++11) specify =delete for those two member functions:

DnsRAII(const DnsRAII &p) = delete;
DnsRAII &operator=(const DnsRAII &) = delete;

I'll repeat though: rather than changing the syntax for making those unavailable, I'd prefer to start with a different class. Lacking a reason to do otherwise, a DnsIterator seems like what I'd probably prefer.

To go with that, I think I'd create an IpAddress class with a few constructors to let the rest of the code create an IP address in a usable format a little more easily. As a first attempt at it, I'd consider something like this:

class IPAddress {
    IP4_ADDRESS addr;
public:
    IPAddress(char a, char b, char c, char d);
    IPAddress(IP4_ADDRESS addr) : addr(addr) {}
    explicit operator IP4_ADDRESS() const { return addr; }    
};

With this, the mainstream code to specify the IP Address of Google's server would looks something like:

IPAddress google{8, 8, 8, 8};

Then the DnsIterator class would take an instance of that (along with a domain to search for and an optional DWORD specifying search options):

DnsIterator(IPAddress server, std::string domain, DWORD options = DNS_QUERY_STANDARD) {

Using these, the complete code to retrieve and display the preferred MX record would look something like this:

auto options = DNS_QUERY_BYPASS_CACHE | DNS_QUERY_NO_LOCAL_NAME | DNS_QUERY_NO_HOSTS_FILE;

IPAddress google(8, 8, 8, 8);
std::string name{ "stackoverflow.com" };

DnsIterator<DNS_TYPE_MX> begin(google, name, options);
DnsIterator<DNS_TYPE_MX> end;


std::cout << std::min_element(begin, end, 
    [](DNS_RECORD const &a, DNS_RECORD const &b){ 
        return a.Data.MX.wPreference < b.Data.MX.wPreference; 
    })
    ->Data.MX.pNameExchange;

Likewise, listing all the A records for an address would look something like this:

DnsIterator<DNS_TYPE_A> b2(google, name);
DnsIterator<DNS_TYPE_A> e2;

std::transform(b2, e2, 
    std::ostream_iterator<std::string>(std::cout, "\n"), 
    [&name](DNS_RECORD const &r) { return rrtos(name, r); });

Of course, defining the classes to implement that would be a little extra work up-front, but honestly the amount of extra work is really quite minimal, and the payoff in simpler, neater code afterwards is pretty serious.

\$\endgroup\$
1
  • \$\begingroup\$ Wow, this is really meaningful feedback. Thanks! I'll work through it. \$\endgroup\$ Mar 24 '14 at 3:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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