Skip to content

CDRIVER-4028 Print correct error message when DNS resolution fails #811

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 1, 2021
Merged

CDRIVER-4028 Print correct error message when DNS resolution fails #811

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
15ca424
Print correct error message when DNS resolution fails
alcaeus Jun 29, 2021
8f630aa
Drop duplicate case value
alcaeus Jun 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 21 additions & 3 deletions src/libmongoc/src/mongoc/mongoc-client.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,24 @@ typedef bool (*mongoc_rr_callback_t) (const char *service,
mongoc_rr_data_t *rr_data,
bson_error_t *error);

static const char *
_mongoc_hstrerror (int code)
{
switch (code) {
case HOST_NOT_FOUND:
return "The specified host is unknown.";
case NO_ADDRESS:
return "The requested name is valid but does not have an IP address.";
case NO_RECOVERY:
return "A nonrecoverable name server error occurred.";
case TRY_AGAIN:
return "A temporary error occurred on an authoritative name server. Try "
"again later.";
default:
return "An unknown error occurred.";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

static bool
srv_callback (const char *service,
ns_msg *ns_answer,
Expand Down Expand Up @@ -328,7 +346,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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

if (!_mongoc_host_list_from_hostport_with_err (
Expand Down Expand Up @@ -474,7 +492,7 @@ _mongoc_get_rr_search (const char *service,
DNS_ERROR ("Failed to look up %s record \"%s\": %s",
rr_type_name,
service,
strerror (h_errno));
_mongoc_hstrerror (h_errno));
}
} while (size >= buffer_size);

Expand All @@ -495,7 +513,7 @@ _mongoc_get_rr_search (const char *service,
i,
rr_type_name,
service,
strerror (h_errno));
_mongoc_hstrerror (h_errno));
}

/* Skip records that don't match the ones we requested. CDRIVER-3628 shows
Expand Down