Throw h_errno specific exceptions after res_query errors.

[dminuoso]

resolv currently ignores h_errno and only produces a generic user error when res_query fails.

This PR forwards h_errno as different DnsException constructors when h_errno is available.

[dminuoso]

See #4

[lyokha]

@dminuoso not sure why I cannot write comments right inside code review (probably, I don't have permission). I wanted to propose replacing the 4 lines with throw in file src/Network/DNS.hs (throw Dns...) by throwIO Dns... as throwIO is preferable in IO context.

[andreasabel]

@dminuoso not sure why I cannot write comments right inside code review

@lyokha Please try again after accepting the invitation.

[lyokha]

Did you mean throwIO here and in other 3 cases below? throwIO is preferable in the IO context.

[lyokha]

Similar messages (res_send(3) failed and res_mkquery(3) failed have no space between the function name and the man section tag (3), consider removing the space here for consistency.

[lyokha]

There is more space for improvements. Note that h_errno is not reliable in multithreading environment as it's global. A better check against the same errors as h_errno provides is field res_h_errno inside struct __res_state. The status gets copied simultaneously in both places. The struct is availlable in the n-style API (i.e. res_nquery). Below are changes to demonstrate how to use this:

File configure.ac

diff --git a/configure.ac b/configure.ac
index 93942e2..2c735fa 100644
--- a/configure.ac
+++ b/configure.ac
@@ -30,6 +30,10 @@ AC_CHECK_DECLS([res_query, res_nquery], [], [], [[
 #include <resolv.h>
 ]])
 
+AC_CHECK_DECLS([h_errno], [], [], [[
+#include <netdb.h>
+]])
+
 dnl ----------------------------------------------------------------------------
 
 RESOLV_SEARCH_LIBS([res_query],[res_query(0,0,0,0,0)],[resolv bind],[EXTRA_LIBS="$EXTRA_LIBS $ac_lib"],[
@@ -71,6 +75,17 @@ fi
 
 fi
 
+AC_CHECK_MEMBERS([struct __res_state.res_h_errno],[],[],[[
+#include <sys/types.h>
+#ifdef HAVE_NETINET_IN_H
+# include <netinet/in.h>
+#endif
+#ifdef HAVE_ARPA_NAMESER_H
+# include <arpa/nameser.h>
+#endif
+#include <resolv.h>
+]])
+
 AC_MSG_CHECKING([which DNS api to use])
 
 case "x$USE_RES_NQUERY" in

Check that field res_h_errno exists (it exists in glibc 23 years already).

File cbits/hs_resolv.h

diff --git a/cbits/hs_resolv.h b/cbits/hs_resolv.h
index 9e2aa32..5bf1eea 100644
--- a/cbits/hs_resolv.h
+++ b/cbits/hs_resolv.h
@@ -9,6 +9,10 @@
 # include <netinet/in.h>
 #endif
 
+#if defined(HAVE_DECL_H_ERRNO)
+# include <netdb.h>
+#endif
+
 #if defined(HAVE_ARPA_NAMESER_H)
 # include <arpa/nameser.h>
 #endif
@@ -86,6 +90,48 @@ hs_res_close(struct __res_state *s)
   res_nclose(s);
 }
 
+#if defined(HAVE_STRUCT___RES_STATE_RES_H_ERRNO)
+
+inline static int
+hs_get_h_errno(struct __res_state *s)
+{
+  assert(s);
+
+  switch(s->res_h_errno)
+  {
+    case HOST_NOT_FOUND: return 1;
+    case NO_DATA: return 2;
+    case NO_RECOVERY: return 3;
+    case TRY_AGAIN: return 4;
+    default:  return -1;
+  }
+}
+
+#elif defined(HAVE_DECL_H_ERRNO)
+
+inline static int
+hs_get_h_errno(struct __res_state *s)
+{
+  switch(h_errno)
+  {
+    case HOST_NOT_FOUND: return 1;
+    case NO_DATA: return 2;
+    case NO_RECOVERY: return 3;
+    case TRY_AGAIN: return 4;
+    default:  return -1;
+  }
+}
+
+#else
+
+inline static int
+hs_get_h_errno(struct __res_state *s)
+{
+  return -1;
+}
+
+#endif
+
 #else
 
 /* use non-reentrant API */
@@ -139,6 +185,31 @@ hs_res_close(void *s)
 {
 }
 
+#if defined(HAVE_DECL_H_ERRNO)
+
+inline static int
+hs_get_h_errno(void *s)
+{
+  switch(h_errno)
+  {
+    case HOST_NOT_FOUND: return 1;
+    case NO_DATA: return 2;
+    case NO_RECOVERY: return 3;
+    case TRY_AGAIN: return 4;
+    default:  return -1;
+  }
+}
+
+#else
+
+inline static int
+hs_get_h_errno(void *s)
+{
+  return -1;
+}
+
+#endif
+
 #endif
 
 #endif /* HS_RESOLV_H */

Note that hs_get_h_errno() accepts one argument now. The argument is a pointer to struct __res_state.

File src/Network/DNS/FFI.hs

diff --git a/src/Network/DNS/FFI.hs b/src/Network/DNS/FFI.hs
index 04e895d..8c4bd2e 100644
--- a/src/Network/DNS/FFI.hs
+++ b/src/Network/DNS/FFI.hs
@@ -87,3 +87,6 @@ foreign import capi safe "hs_resolv.h hs_res_mkquery" c_res_mkquery :: Ptr CResS
 -- void hs_res_close(void *);
 foreign import capi safe "hs_resolv.h hs_res_close" c_res_close :: Ptr CResState -> IO ()
 
+-- void *get_h_errno(void *s, int c, size_t n);
+foreign import capi unsafe "hs_resolv.h hs_get_h_errno" c_get_h_errno :: Ptr CResState -> IO CInt
+

File src/Network/DNS.hs

diff --git a/src/Network/DNS.hs b/src/Network/DNS.hs
index ab24be0..96b80b4 100644
--- a/src/Network/DNS.hs
+++ b/src/Network/DNS.hs
@@ -98,18 +98,22 @@ import           Compat
 import           Network.DNS.FFI
 import           Network.DNS.Message
 
--- | Exception thrown in case of errors while encoding or decoding into a 'Msg'.
+-- | Exception thrown in case of errors while resolving or encoding/decoding into a 'Msg'.
 --
 -- @since 0.1.1.0
 data DnsException = DnsEncodeException
                   | DnsDecodeException
+                  | DnsHostNotFound -- ^ No such domain (authoritative)
+                  | DnsNoData       -- ^ No record for requested type
+                  | DnsNoRecovery   -- ^ Non recoverable errors, REFUSED, NOTIMP
+                  | DnsTryAgain     -- ^ No such domain (non-authoritative) or SERVERFAIL
                   deriving (Show, Typeable)
 
 instance Exception DnsException
 
 -- | Send a query via @res_query(3)@ and decode its response into a 'Msg'
 --
--- Throws 'DnsException' in case of encoding or decoding errors. May throw other IO exceptions in case of network errors.
+-- Throws 'DnsException' in case of resolving or encoding/decoding errors. May throw other IO exceptions in case of network errors.
 --
 -- === Example
 --
@@ -158,7 +162,13 @@ queryRaw (Class cls) (Name name) qtype = withCResState $ \stptr ->
                     unless (errno == eOK) $
                         throwErrno "res_query"
 
-                    fail "res_query(3) failed"
+                    h_errno <- c_get_h_errno stptr
+                    case h_errno of
+                        1 -> throwIO DnsHostNotFound
+                        2 -> throwIO DnsNoData
+                        3 -> throwIO DnsNoRecovery
+                        4 -> throwIO DnsTryAgain
+                        _ -> fail "res_query(3) failed"
 
                 BS.packCStringLen (resptr, fromIntegral reslen)

This patch was made against changes in another branch, so it's hardly can be applied directly in your branch.

[andreasabel]

Now with #12 merged, this PR needs some conflict resolution.

[lyokha]

Yes, the conflict should arise in DNS.hs in function queryRaw. Resolution seems to be simple: replace line

fail "res_query(3) failed"

by the new hunk from the original patch and indent the new text correctly.

[lyokha]

After merge i would replace throw by throwIO in that same hunk, and remove extra space between res_query and (3) because there are no space in similar messages in the code.

I would also consider getting state from field __res_state.res_h_errno because this is more reliable and more natural (see my comments above).

[lyokha]

@andreasabel I can try to resolve conflicts

[lyokha]

Ah, the conflict is in DNS/FFI.hs. Then it should be even simpler.

[andreasabel]

Please add @since 0.2.0.0 to the new API elements and add them to the CHANGELOG.

[andreasabel]

@lyokha

About the changelog. Should it contain other changes, e.g. the fix of memory leaks?

Yes, it should list all the changes that are relevant for users of this package.
If you are done with all the changes you want to release, you can make a PR with the CHANGELOG and I will have a look!

[andreasabel]

I added the @since info I requested. Works if placed on a separated new line.

[andreasabel]

@lyokha: I tried to remove the amount of cut&paste code by using a new macro __HS_GET_H_ERRNO. Please review (& approve if you agree).

Hang on, found something else...

Ok, further reduced cut&paste.

[lyokha]

Looks good!

[andreasabel]

It is a bit unsettling that there are no tests for this new feature.
But I guess testing would involve making some real DNS requests.
The current testsuite does not seem to make such, it just tests some internal invariants.

[lyokha]

Just made a manual test.

{-# LANGUAGE OverloadedStrings #-}

module Main where

import Network.DNS
import Control.Exception

main :: IO ()
main = handle (\e -> putStrLn $ "Caught: " ++ show (e :: DnsException)) $
    queryA (Name "www.auuaauuaoogle.com") >>= print

$ dist/build/resolv_test/resolv_test
Caught: DnsHostNotFound

So this works at first glance. I have more issues to discuss:

1. fail in IO throws exceptions of type IOError, we are stealing them in favour of our custom DnsException type. This makes error handling on the user side harder because now we have approximately equal (?) probabilities of exceptions of both kinds. Before this fix we only had encoding/decoding exceptions other than IOError.
2. hs_get_errno does not test the success case (when h_errno is 0), it means that it can be used only in negative context, i.e. when processing an error: if used in other contexts, it returns -1 as if an error occurred. Not sure if this is a big issue, but it must be kept in mind in case of refactoring.

[lyokha]

Btw, DNS query with some bogus name can be used to test DnsHostNotFound (until someone registers such a name though).

[andreasabel]

2. hs_get_errno does not test the success case (when h_errno is 0), it means that it can be used only in negative context, i.e. when processing an error: if used in other contexts, it returns -1 as if an error occurred.

Good catch. This is indeed not a good design and can lead to accidential errors. It should be patched to return 0 if no error occurred.

1. fail in IO throws exceptions of type IOError, we are stealing them in favour of our custom DnsException type

Hard for me to judge the implications of this. Should be documented in the CHANGELOG.
Or do you think this behavior should be changed?

Btw, DNS query with some bogus name can be used to test DnsHostNotFound (until someone registers such a name though).

This sounds like a test to this extent could be added. DNS query should be cheap enough to do in CI.
One could also add a test for a successful query such as google.com.

[lyokha]

Hard for me to judge the implications of this. Should be documented in the CHANGELOG. Or do you think this behavior should be changed?

I just recalled how I used this in a custom application:

    !addrs <- handle (\e -> if isUserError e &&
                                    "(res_query(3) failed)" `T.isSuffixOf` T.pack (show e)
                                then do

It wasn't simple as well :) Now, if I want to catch all exceptions (modulo encoding/decoding), I must use catches rather than simply handle. OTOH, this ugly check against the string literal goes away.

The changelog could be more precise:

Check the value of `h_errno` on failures of `res_nquery()` and throw an appropriate exception.

to

Check the value of `h_errno` on failures of `res_nquery()` and throw an appropriate exception of type `DnsException`.

Btw, DNS query with some bogus name can be used to test DnsHostNotFound (until someone registers such a name though).

This sounds like a test to this extent could be added. DNS query should be cheap enough to do in CI. One could also add a test for a successful query such as google.com.

Now I hesitate. Tests cannot control DNS system settings on the user side, and thus get no reliable.

[andreasabel]

OTOH, this ugly check against the string literal goes away.

Yeah, this is definitely an improvement!

Tests cannot control DNS system settings on the user side, and thus get no reliable.

Ok, but maybe it is sufficient that tests run correctly in the CI and in vanilla user environments.

[lyokha]

@andreasabel, don't you mind against the following change in the changelog?

diff --git a/ChangeLog.md b/ChangeLog.md
index af36318..6eb29b1 100644
--- a/ChangeLog.md
+++ b/ChangeLog.md
@@ -10,7 +10,10 @@ _2023-03-xx, Alexey Radkov and Andreas Abel_
   (PR [#16](https://github.com/haskell-hvr/resolv/pull/16).)
 * Fix memory leaks due to missing `res_nclose()` after each `res_ninit()` call.
   (PR [#12](https://github.com/haskell-hvr/resolv/pull/12).)
-* Check the value of `h_errno` on failures of `res_nquery()` and throw an appropriate exception.
+* Check the value of `h_errno` on failures of `res_nquery()` and throw an
+  appropriate exception of type `DnsException` built with one of new
+  constructors `DnsHostNotFound`, `DnsNoData`, `DnsNoRecovery`, or `DnsTryAgain`.
+  Note that previously such exceptions were thrown by `fail` and had type `IOError`.
   (PR [#17](https://github.com/haskell-hvr/resolv/pull/17).)
 * Suppress configure warning on option `--with-compiler` passed by Cabal.
   (PR [#21](https://github.com/haskell-hvr/resolv/pull/21).)

Any suggestions?

[andreasabel]

It should be patched to return 0 if no error occurred.

I did this now. Please check!

Changelog looks good!

[lyokha]

Yes, this is exactly what I meant! The case when HAVE_H_ERRNO not defined is more subtle (now it always returns -1) but I guess it won't appear nowadays in real world.

[lyokha]

I committed updates in the changelog in the branch I used before.

[andreasabel]

Yes, this is exactly what I meant! The case when HAVE_H_ERRNO not defined is more subtle (now it always returns -1) but I guess it won't appear nowadays in real world.

Yes looking at the call site, it is only called if there was an error indeed: https://github.com/haskell-hvr/resolv/pull/17/files#diff-7f2d6596edf47f19c2c00ce2c44fb33f4695a697116995962c5f10284eafb324R174-R178

So, strictly speaking, my change to preserve 0 was unnecessary. However, it is not wrong and looks more robust.

I think we can merge now.