Skip to content
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.

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

Fix CURLOPT_DNS_SHUFFLE_ADDRESSES #3110

Closed
wants to merge 1 commit into from

Conversation

dreckard
Copy link
Contributor

@dreckard dreckard commented Oct 7, 2018

Further error handling was added during merging of #1694, changing the return type of Curl_shuffle_addr from void to CURLcode. Unfortunately an incorrect check on the return type made its way in, causing Curl_cache_addr to always fail if CURLOPT_DNS_SHUFFLE_ADDRESSES is enabled.

The unit test for CURLOPT_DNS_SHUFFLE_ADDRESSES (1608) did not catch this because it does not call Curl_cache_addr, only verifying that:

  1. curl_easy_setopt succeeds
  2. Curl_shuffle_addr returns success
  3. The ordering of addresses actually changes

This PR just fixes the one return value comparison mentioned and things seem to work as expected after that. Let me know if you think it would be worth any test changes, but this can probably just be considered the one time introductory pain of a rarely used new option :)

@@ -455,7 +455,7 @@ Curl_cache_addr(struct Curl_easy *data,
/* shuffle addresses if requested */
if(data->set.dns_shuffle_addresses) {
CURLcode result = Curl_shuffle_addr(data, &addr);
if(!result)
if(result != CURLE_OK)
Copy link
Member

Choose a reason for hiding this comment

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

We prefer to use plain if(result) in the code for this check (used everywhere)! But otherwise it looks good.

@bagder bagder closed this in 3349a63 Oct 8, 2018
@bagder
Copy link
Member

bagder commented Oct 8, 2018

Thanks!

Getting a test case that covers this would of course be ideal, but I can't think of a good way to do it...

@lock lock bot locked as resolved and limited conversation to collaborators Jan 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants