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
base: master
from

Conversation

Projects
None yet
2 participants
@dreckard
Contributor

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 :)

@bagder

bagder approved these changes Oct 7, 2018

@@ -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)

This comment has been minimized.

@bagder

bagder Oct 7, 2018

Member

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

This comment has been minimized.

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment