Throwing a DnsException rather than calling fail

[chrisdone]

Would you accept a PR that replaced the calls to fail such as these with e.g. throwIO ResQueryFailed?

I'd like to use this API for my SMTP server and I'd like to check SPF records (via TXT and SPF), it'd be nice to do a catch of type DnsException to handle all DNS-resolving issues.

Presently if I use a domain that doesn't have a TXT record, the failure isn't the best:

> queryTXT (Network.DNS.Name "chrisdone.com")
*** Exception: user error (res_query(3) failed)

In the case of a failed query, I'd mark incoming email as spam. So that's a fine case, other cases, I might want to log as an issue. At present I'd have to match on the strings.

Should be a quick update. And any existing code using this lib would (hopefully) get a non-exhaustive pattern match warning when upgrading. But it could also be a major version bump with a CHANGELOG warning.

[hvr]

indeed, that's a part that's lacking; I didn't get to map dns errors to exceptions as I ran out of time figuring out how to do this properly...

The trouble is that e.g. res_query(3) simply returns -1 in case of failures, and at that point you have to figure out what kind of error you had. But you seem to be happy with having a general catch-all exception constructor added to DnsException (which doesn't necessarily tell you why res_query failed)?

[chrisdone]

At least for my use-case, I really need to know whether I got an error because there's simply no record and all other procedures went correctly, versus there's an actual problem resolving (and there might be a valid TXT/SPF record and an innocent emailer). It's true that a generic FailedForSomeReason exception wouldn't let me distinguish, and might end up marking an email as spam unintentionally.

I'll go away and do a bit of research into the errono types of problems and report back, then we can decide with more information. I'll also check what the RFC on SPF says about DNS resolving failures.

EDIT: I suppose another API would return Maybe a record (indicating: all fine, but no actual record of that type), but I don't want to mess with your existing API. A catch around the call is fine for me.

[chrisdone]

Good news! I found the following documentation from Oracle of all places: https://docs.oracle.com/cd/E19683-01/816-0214/6m6nf1p39/index.html#hstrerror,%20herror

#define NETDB_INTERNAL -1 /* see errno */  
#define NETDB_SUCCESS  0  /* no problem */ 
#define HOST_NOT_FOUND 1  /* Authoritative Answer Host not found */  
#define TRY_AGAIN      2  /* Non-Authoritative not found, or SERVFAIL */ 
#define NO_RECOVERY    3  /* Non-Recoverable: FORMERR, REFUSED, NOTIMP*/ 
#define NO_DATA        4  /* Valid name, no data for requested type */   

I added information to the string message, here:

-                fail "res_query(3) failed"
+                fail ("res_query failed [errno=" ++ show (let Errno i =errno
+                                                        in i)++ ", reslen=" ++ show reslen ++ "]")

But I found that the stderr number always remained 0:

Prelude Compat Network.DNS Network.DNS.FFI Network.DNS.Message> queryTXT (Name "chrisdone.com")
hs_res_query: s->res_h_errno = 4
*** Exception: user error (res_query failed [errno=0, reslen=-1])

That doesn't make sense, but actually while looking through the man
page: http://man7.org/linux/man-pages/man3/resolver.3.html

The API consists of a set of more modern, reentrant functions and an
older set of nonreentrant functions that have been superseded. The
traditional resolver interfaces such as res_init() and res_query()
use some static (global) state stored in the _res structure,
rendering these functions non-thread-safe. BIND 8.2 introduced a set
of new interfaces res_ninit(), res_nquery(), and so on, which take a
res_state as their first argument, so you can use a per-thread
resolver state.

And I looked inside the implementation of res_nquery
https://github.com/lattera/glibc/blob/master/resolv/res_query.c#L219

And you can see that it actually sets a field in the state object,
rathe than errno:

RES_SET_H_ERRNO(statp, NO_RECOVERY);

Indeed, that's what it says on the Oracle page:

The variables statp->res_h_errno and _res.res_h_errno and external
variable h_errno are set whenever an error occurs during a resolver
operation.

So, making this change in hs_resolv.h,

 hs_res_query(struct __res_state *s, const char *dname, int class, int type, unsigned char *answer, int anslen)
 {
   assert(s);
-
-  return res_nquery(s, dname, class, type, answer, anslen);
+  int i = res_nquery(s, dname, class, type, answer, anslen);
+  printf("hs_res_query: s->res_h_errno = %d\n", s->res_h_errno);
+  return i;
 }

We now get this output:

Prelude Compat Network.DNS Network.DNS.FFI Network.DNS.Message> queryTXT (Name "chrisdone.com")
hs_res_query: s->res_h_errno = 4
*** Exception: user error (res_query failed [errno=0, reslen=-1]

Hurrah! 4 means "no data for the requested type", just what I was
looking for.

So I think we can make a sum type for the above set of error cases,
and then fallback to the errno value on -1 (#define NETDB_INTERNAL -1 /* see errno */). Something like:

data DnsException
  = NetdbInternalError Int -- ^ see errno 
  | HostNotFound       -- ^ Authoritative Answer Host not found 
  | TryAgain           -- ^ Non-Authoritative not found, or SERVFAIL
  | NoRecovery         -- ^ Non-Recoverable: FORMERR, REFUSED, NOTIMP
  | NoData             -- ^ Valid name, no data for requested type
  | MessageSizeOverflow
  | ResInitFailed
  | ResMkQueryFailed
  | DnsEncodeException
  | DnsDecodeException

What do you think? The last three are just adding cases from your
code.

[hvr]

I guess that would work; it'll be fun to check how much Solaris, AIX (does have a global h_errno documented at least in their man-pages), *BSD, Darwin, et al diverge and adapt the autoconf scripting... ;-)

I'm starting to wonder if there would be any benefit to have e.g. a separate DnsQueryException data type to partition the exceptions according to the library call.

[hvr]

The other consideration worth paying attention to is to see how to reconcile this w/ a potential execption type in https://hackage.haskell.org/package/windns-0.1.0.0/docs/Network-DNS.html

[hvr]

@chrisdone ...so, I think we should only turn errno /= 0-failures into exceptions, as the other error-cases are supposed to have the error-information as part of the ordinary response data (in the RCODE header field), except for the high-level query{A,TXT,..} entry points, which are trickier, as they simply filter the "good" responses, and error data structures are simply ignored/filtered out. I'd have to investigate if it's possible to have a non-zero RCODE combined w/ partial valid responses.

[chrisdone]

I'm just coming back to looking at adding spam checking to my mail server again.

I think we should only turn errno /= 0-failures into exceptions

So for this example,

Prelude Compat Network.DNS Network.DNS.FFI Network.DNS.Message> queryTXT (Name "chrisdone.com")
hs_res_query: s->res_h_errno = 4
*** Exception: user error (res_query failed [errno=0, reslen=-1]

What way of calling the module would you recommend? I don't mind calling query directly, provided it gives all info in a first-class manner.

[chrisdone]

Actually, I've found libspf2 which has packages in standard distributions and seems to be used by sendmail etc. No need to do the work manually!