CDRIVER-4028 Print correct error message when DNS resolution fails #811
Conversation
| return "A temporary error occurred on an authoritative name server. Try " | ||
| "again later."; | ||
| default: | ||
| return "An unknown error occurred."; |
There was a problem hiding this comment.
In practice, would we ever hit this case? If so, I wonder if it'd be helpful to somehow expose the integer value in the error message relayed up the chain. I realize we can't print the value of code here since it'd require allocating a string, but perhaps we can do this in the calling context.
If it's unlikely we'd hit this case, I'm fine leaving it as-is.
There was a problem hiding this comment.
I was thinking of it but avoided it because it would require allocating memory. Either way, even if we hit this scenario, it would be difficult for us to do anything as the value given would not make any sense to us (since it isn't documented). Even with the code, we'd most likely have to resort to tracing or other means to find out what's going on so I opted for this message.
| @@ -328,7 +347,7 @@ srv_callback (const char *service, | |||
| if (size < 1) { | |||
| DNS_ERROR ("Invalid record in SRV answer for \"%s\": \"%s\"", | |||
| service, | |||
| strerror (h_errno)); | |||
| _mongoc_hstrerror (h_errno)); | |||
There was a problem hiding this comment.
I'm trusting that h_errno is always set by this point. I do find it odd that we rely on it being declared and reference it here even if both DNS APIs used above (which would set this) are unavailable. Or would libmongoc never compile without at least one of those available?
I left some more general comments about using h_errno in CDRIVER-4028, but that's outside the scope of this PR.
There was a problem hiding this comment.
Quoting from the resolver(3) man page:
In the case of an error return from res_nquery(), res_query(), res_nsearch(), res_search(), res_nquerydomain(), or res_querydomain(), the global variable h_errno (see gethostbyname(3)) can be consulted to determine the cause of the error.
| @@ -306,7 +306,6 @@ _mongoc_hstrerror (int code) | |||
| case HOST_NOT_FOUND: | |||
| return "The specified host is unknown."; | |||
| case NO_ADDRESS: | |||
| case NO_DATA: | |||
There was a problem hiding this comment.
Is this guaranteed to have the same value as NO_ADDRESS? I assume you caught this in some CI warning?
There was a problem hiding this comment.
Yes. I added both constants to ensure we're grabbing it, but CI complained about the case being duplicated. I'd argue that using one of these should be sufficient, and the 50:50 joker picked NO_DATA for elimination. I don't anticipate trouble from this, but I suppose a last resort to deal with either constant not being defined would be to hardcode the value (4) and be done with it.
|
Cherry-picked to r1.17 in e1be09a. |
CDRIVER-4028
I'll backport the resulting commit to r1.17 after merging.