HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

WinAPI code for DNS queries

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
winapifordnscodequeries

Problem

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
#include
#include
#include
#include
#include
#include // I keep specifying wstring and wcout. I could macro it, but I don't want to. Is it normal to specify w everywhere?
#include
#include

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 pNext)
{
switch (current->wType)
{
case DNS_TYPE_MX:
ss MX " Data.Mx.pNameExchange A " Data.A.IpAddress) AAAA " Data.AAAA.Ip6Address) PTR " Data.PTR.pNameHost Record type " wType pNext)
{
switch (current->wType)
{
case DNS_TYPE_MX:
if (current->Data.MX.wPreference 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 Data.A.IpAddress) Data.A.IpAddress);

DnsRAII results3;
if (0 == DnsQuery((PCWSTR)reverse.c_str(), DNS_TYPE_PTR, options, &dnsServers, &results3.p, NULL))
{
wcout Data.PTR.pNameHost << "\"\n";

wcout << "\nDone.\n";

Solution

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 begin(google, name, options);
DnsIterator end;

std::cout Data.MX.pNameExchange;


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

DnsIterator b2(google, name);
DnsIterator e2;

std::transform(b2, e2, 
    std::ostream_iterator(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.

Code Snippets

DnsRAII(const DnsRAII &p) = delete;
DnsRAII &operator=(const DnsRAII &) = delete;
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; }    
};
IPAddress google{8, 8, 8, 8};
DnsIterator(IPAddress server, std::string domain, DWORD options = DNS_QUERY_STANDARD) {
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;

Context

StackExchange Code Review Q#44291, answer score: 5

Revisions (0)

No revisions yet.