Support for ares getaddrinfo

[ki11roy]

This is a deeply reworked #112, though I have changed everything except maybe tests. It still lacks proper sorting (rfc and sortlist), but I hope to fix it later.
Update: sorting as a separate function available in #239

Summary of changes:

• ares_addrinfo structure, with allocating function ares__malloc_addrinfo/ares__append_addrinfo, the latter one links new addrinfo to existing one
• ares_sockaddr union; ares_process.c modified to use that union too
• ares_getaddrinfo function, first resolves port than looks into hosts file and than performs AAAA and A lookups for AF_UNSPEC and/or A/AAAA lookups for AF_INET/AF_INET6. The resulting list is not sorted. Also there are no aliases in hostent sense as they are represented via separate linked addrinfo structures.
  • lookup_service function, uses getservbyname_r to get numeric name of the service
    • getservbyname_r function, copied from getservbyport_r
  • fake_addrinfo function, allocates addrinfo if we have an ip address in host field (same as fake_hostent)
  • next_lookup function, for file and dns lookups
    • ares__get_addrinfo function gets ares_addrinfo from file, it differs a bit from similar function in gethostbyname
• ares_parse_a_reply/ares_parse_aaaa_reply both changed to parse into ares_addrinfo structure changed back in favour of private ares__parse_a/ares__parse_aaaa
• new tests, though there are no ares_parse_a_reply/ares_parse_aaaa_reply tests with ares_getaddrinfo yet
• manuals for all the public functions
• clang format file to keep things in order, though tests style is different

[coveralls]

Coverage increased (+0.1%) to 91.417% when pulling 302d91a on ki11roy:support_for_ares_getaddrinfo into 98857e3 on c-ares:master.

[bradh352]

@ki11roy: Changes have been made to public function prototypes. It breaks API and ABI compatibility with existing integrations that use those functions.

Can you elaborate on why this decision was made?

Otherwise I do like the fact that a new private structure is used, it helps with portability in my opinion (especially since I've done a bit of work with legacy systems like SCO).

I need some time to evaluate the code further to see how it differs from #112 by @ChristianAmmer

[ki11roy]

@bradh352 From one side ares_parse_a_reply/ares_parse_aaaa_reply look more like private and as I guess intended for some special cases.
From another side they parse into hostent structure already and not providing such ability for addrinfo would be a miss.
So nevertheless it’s public I believe it would be of help to have ability to parse into ares_addrinfo straight away.

Update:
In comparison to #112 there are no extra allocations and copies of hostent structures.

[ki11roy]

Reverted public function prototypes back to original.

[bradh352]

please re-base your changesets against current master now that the original ticket has been merged.

Thanks!

[bagder]

Don't merge, rebase (cleanup any problems) and force-push. Makes things much easier to review...

[ki11roy]

Don't merge, rebase (cleanup any problems) and force-push. Makes things much easier to review...

Oh, that was an intermediate commit, please ignore it. Squashed and cleaned up a bit.

[bradh352]

@ChristianAmmer care to review this PR as well?

[ChristianAmmer-zz]

I will review this Pull Request. I'm not sure how to start best because the changes are quite a lot and I think it's not wise to comment inline the code. I think it's good to start here by describing my first thoughts.

• The order how DNS suffixes are searched for T_A and T_AAAA queries will be changed. For example if AF_UNSPEC is requested, Adding feature ares_getaddrinfo #112 tries an suffix for both address families before trying the next suffix in the suffix search list. This PR changes the behavior by using the get_hostbyname behavior.
  This uses ares_search which iterates through the suffixes for one family (T_AAAA) and when it's finished it iterates through the suffixes for the other one (T_A). This has an negative effect which I tried to explain in Better AF_UNSPEC support for ares_gethostbyname #94.
• I understand the intention of ares__parse_a_reply with the extra ares_addrinfo argument to reduce an extra hostentmemory allocation. But for the user of the function it's not obvious how the function behaves for NULL vs. non NULL host and ai arguments.

For the rest I will have to look in more detail soon, but it's great to see features implemented like the file lookup, setting ai_canonname using service, ...

[ki11roy]

@ChristianAmmer

• Well, it is how gethostbynname works now and that was not my intention to change the method. It is better to change ares_search to get consistency.
  Also you did some manipulations with single/multi domain code which i think is a mistake, because getaddrinfo not working with multi domain queries and dot suffixes.

• It is is private now, so it decreases users count to those who understood. Nevertheless I will add the comment if it is not there yet.

[ChristianAmmer-zz]

Do you agree with me, that a suffix should be tried for both families before trying the next suffix in the search list?

• Well, it is how gethostbynname works now and that was not my intention to change the method. It is better to change ares_search to get consistency.

How do you intend to change ares_search?

Also you did some manipulations with single/multi domain code which i think is a mistake, because getaddrinfo not working with multi domain queries and dot suffixes.

The dot suffix specifies a FQDN and getaddrinfo can work with FQDN.
Also can you please describe in more detail where I did make these manipulations and what's my mistake with them.

• It is is private now, so it decreases users count to those who understood. Nevertheless I will add the comment if it is not there yet.

With user I mean also the c-ares developers. The interface is not self explaining, you have to read the implementation to know what happens with the result arguments host and ai if you set them NULL or non NULL. Also in my opinion the complexity of the function increases unnecessarily.

[ki11roy]

Do you agree with me, that a suffix should be tried for both families before trying the next suffix in the search list?

Agree, see below.

• Well, it is how gethostbynname works now and that was not my intention to change the method. It is better to change ares_search to get consistency.

How do you intend to change ares_search?

Something similar to adding ever_got_nodata flag, like ever_got_servfail:

        if (squery->status_as_is == ARES_ENOTFOUND && squery->ever_got_servfail) {
          end_squery(squery, ARES_ESERVFAIL, NULL, 0);
        }

So it is not fatal if we have some results. If there are no - handle it accordingly in the gethostbyname/getaddrinfo just like with ARES_ENODATA (note status == ARES_ESERVFAIL)

  else if ((status == ARES_ENODATA || status == ARES_ESERVFAIL || status == ARES_EBADRESP ||
            status == ARES_ETIMEOUT) && (hquery->sent_family == AF_INET6 &&
            hquery->want_family == AF_UNSPEC))
    {
      /* The AAAA query yielded no useful result.  Now look up an A instead. */
      hquery->sent_family = AF_INET;
      ares_search(hquery->channel, hquery->name, C_IN, T_A, host_callback,
                  hquery);
    }

Also you did some manipulations with single/multi domain code which i think is a mistake, because getaddrinfo not working with multi domain queries and dot suffixes.

The dot suffix specifies a FQDN and getaddrinfo can work with FQDN.

Right, it works. I have compared libc implementations of gethostbyname and getaddrinfo and it seems that I have misunderstood some docs, like this:

       If name doesn't end in a dot and the environment variable HOSTALIASES is
       set, the alias file pointed to by HOSTALIASES will first be searched
       for name (see hostname(7) for the file format).  The current domain
       and its parents are searched unless name ends in a dot.

is valid for getaddrinfo too, but not mentioned in the manual.

Also can you please describe in more detail where I did make these manipulations and what's my mistake with them.

It is not in sync with ares_search anymore, so you have missed some new code:

  /* Per RFC 7686, reject queries for ".onion" domain names with NXDOMAIN. */
  if (ares__is_onion_domain(name))
    {
      callback(arg, ARES_ENOTFOUND, 0, NULL);
      return;
    }

Copy-paste is (almost) always a bad thing so I think it is better to return back to ares_search as I did in this PR.

• It is is private now, so it decreases users count to those who understood. Nevertheless I will add the comment if it is not there yet.
  With user I mean also the c-ares developers. The interface is not self explaining, you have to read the implementation to know what happens with the result arguments host and ai if you set them NULL or non NULL. Also in my opinion the complexity of the function increases unnecessarily.

It is, but I doubt that separating that in two will get things easier. Though if you have ideas how to improve I will consider it.

[ki11roy]

@ChristianAmmer
I have investigated that issue from #94 and it seems that there is no problem at all. All the SERVFAIL requests will be skipped by default on TCP channel and UDP requests will be retried till success or failure. The only way to have SERVFAIL is to have ARES_FLAG_NOCHECKRESP flag set. This case can be fixed like I said in the message above, but I really see no reason for that. @bagder @bradh352 am I correct?

Flag description:

ARES_FLAG_NOCHECKRESP Do not discard responses with the SERVFAIL, NOTIMP, or REFUSED response code or responses whose questions don't match the questions in the request. Primarily useful for writing clients which might be used to test or debug name servers.

And the code:

  /* If we aren't passing through all error packets, discard packets
   * with SERVFAIL, NOTIMP, or REFUSED response codes.
   */
  if (!(channel->flags & ARES_FLAG_NOCHECKRESP))
    {
      if (rcode == SERVFAIL || rcode == NOTIMP || rcode == REFUSED)
        {
          skip_server(channel, query, whichserver);
          if (query->server == whichserver)
            next_server(channel, query, now);
          return;
        }
    }

And the test

TEST_P(MockTCPChannelTest, ServFailResponse) {
  DNSPacket rsp;
  rsp.set_response().set_aa()
    .add_question(new DNSQuestion("www.google.com", ns_t_a));
  rsp.set_rcode(ns_r_servfail);
  EXPECT_CALL(server_, OnRequest("www.google.com", ns_t_a))
    .WillOnce(SetReply(&server_, &rsp));
  HostResult result;
  ares_gethostbyname(channel_, "www.google.com.", AF_INET, HostCallback, &result);
  Process();
  EXPECT_TRUE(result.done_);
  // ARES_FLAG_NOCHECKRESP not set, so SERVFAIL consumed
  EXPECT_EQ(ARES_ECONNREFUSED, result.status_);
}

There are some tests with MockNoCheckRespChannelTest,
I’ve modified one to get ARES_ESERVFAIL:

/* experiments with the ARES_FLAG_NOCHECKRESP  */
TEST_P(MockNoCheckRespChannelTest, ServFailResponse) {
  DNSPacket rsp;
  rsp.set_response().set_aa()
    .add_question(new DNSQuestion("www.google.com", ns_t_a));
  rsp.set_rcode(ns_r_servfail);
  EXPECT_CALL(server_, OnRequest("www.google.com", ns_t_a))
    .WillOnce(SetReply(&server_, &rsp));
  HostResult result;
  ares_gethostbyname(channel_, "www.google.com.", AF_INET, HostCallback, &result);
  Process();
  EXPECT_TRUE(result.done_);
  EXPECT_EQ(ARES_ESERVFAIL, result.status_);
}

[ChristianAmmer-zz]

Regarding ares_search:

Copy-paste is (almost) always a bad thing so I think it is better to return back to ares_search as I did in this PR.

I did not copy-paste the ares_search code (so that's not an argument to return to ares_search), I tried to implement the functionality in a different way and I think I made it right. I removed complexity without making it harder to introduce additional requirements like the RFC 7686 (.onion) part you mentioned. With lesser complexity I mean:
Before we had ares_gethostbyname -> next_lookup -> ares_search -> ares_query ... with the queries host_query, search_query and qquery (one query to much to fit in my mind :-) ) each of them with a callback function. I decided to remove the ares_search layer and changed it to ares_getaddrinfo -> next_dns_lookup -> ares_query ....

How do you intend to change ares_search?

Something similar to adding ever_got_nodata flag, like ever_got_servfail:

But with an additional flag you will not solve the check-suffix-for-both-families-before-switch-to-next-suffix task. Which you already agreed with me.

Regarding ares_parse_a(aaa)_reply

Also in my opinion the complexity of the function increases unnecessarily.

It is, but I doubt that separating that in two will get things easier. Though if you have ideas how to improve I will consider it.

The functions need not to be changed at all. They have an output argument hostent which can trivially converted and added to an addrinfo variable. I do not see a benefit changing it the way you did. Your argument

in comparison to #112 there are no extra allocations and copies of hostent structures.

Is not the full truth because you make extra allocations and copies in ares__parse_a(aaa)_reply. While the hostent branch just reuses an addrs variable for the hostent result, the addrinfo branch you introduced copies addrs and deletes it later.

[ki11roy]

I would like to clarify if #94 is a real issue before discussing any further, see my comment above.

[ki11roy]

@bagder @bradh352 any comments? The code below works as expected.

using testing::_;

// the test relies on retries
TEST_P(MockUDPChannelTest, SearchDomainsServFailOnAAAAThenSuccess) {
  DNSPacket servfailrsp;
  servfailrsp.set_response().set_aa().set_rcode(ns_r_servfail)
    .add_question(new DNSQuestion("www.first.com", ns_t_aaaa));

  DNSPacket rspok;  
  rspok.set_response()
    .add_question(new DNSQuestion("www.first.com", ns_t_a))
    .add_answer(new DNSARR("www.first.com", 100, {1, 2, 3, 4}));

  EXPECT_CALL(server_, OnRequest("www.first.com", _))
    .WillOnce(SetReply(&server_, &servfailrsp)) 
    .WillOnce(SetReply(&server_, &servfailrsp)) // retry
    .WillOnce(SetReply(&server_, &rspok)) // wrong type, retry 
    .WillOnce(SetReply(&server_, &rspok));

  HostResult result;
  ares_gethostbyname(channel_, "www", AF_UNSPEC, HostCallback, &result);
  Process();
  EXPECT_TRUE(result.done_);
  EXPECT_EQ(ARES_SUCCESS, result.status_);
  std::stringstream ss;
  ss << result.host_;
  EXPECT_EQ("{'www.first.com' aliases=[] addrs=[1.2.3.4]}", ss.str());
}

[ki11roy]

I removed complexity without making it harder to introduce additional requirements like the RFC 7686 (.onion) part you mentioned. With lesser complexity I mean.

So how are you going to return this, and upcoming fixes, without copying them in two places?

The functions need not to be changed at all. They have an output argument hostent which can trivially converted and added to an addrinfo variable. I do not see a benefit changing it the way you did.

You are making an extra copy into completely different structure, while you can parse into the right place straight away. And that is not counting various ai_hints which can influence parsing too, for example canonical name.

But with an additional flag you will not solve the check-suffix-for-both-families-before-switch-to-next-suffix task. Which you already agreed with me.

And judging by the test above it works without any fixes even now and even with ares_gethostbyname. Or have I missed something? Please explain.

in comparison to #112 there are no extra allocations and copies of hostent structures.
Is not the full truth because you make extra allocations

Please show me exact lines and tell what makes you think of them as 'extra allocations'.

and copies in ares__parse_a(aaa)_reply.

Same.

While the hostent branch just reuses an addrs variable for the hostent result

Allocates, copies and then happily frees it after every add_to_addrinfo call:

      add_to_addrinfo(&hquery->ai, host);
      ares_free_hostent(host);

And looking into ares_free_hostent it seems how many? Five + address count allocations.

void ares_free_hostent(struct hostent *host)
{
  char **p;

  if (!host)
    return;

  ares_free((char *)(host->h_name));
  for (p = host->h_aliases; *p; p++)
    ares_free(*p);
  ares_free(host->h_aliases);
  ares_free(host->h_addr_list[0]); /* no matter if there is one or many entries,
                                 there is only one malloc for all of them */
  ares_free(host->h_addr_list);
  ares_free(host);
}

the addrinfo branch you introduced copies addrs and deletes it later.

While I agree about the copy, please justify your statement about deletion, with the particular lines.

[ChristianAmmer-zz]

So how are you going to return this, and upcoming fixes, without copying them in two places?

I'm thinking of a function which implements the new requirement and that one gets called from both places.
But the question is another: ares_search is not considered to make a T_A and T_AAAA query for one suffix, before switching to the next suffix. This is a limitation of the function and (in my opinion) it therefore should not be used for getaddrinfo. So how will you get this working with ares_search?

You are making an extra copy into completely different structure, while you can parse into the right place straight away. And that is not counting various ai_hints which can influence parsing too, for example canonical name.

ai_hints should not play a role when parsing the response. The canonical name is available from hostent. Only because we can add the result to an addrinfo structure and this saves us from one hostent allocation, that does not mean it's a good idea.

And judging by the test above it works without any fixes even now and even with ares_gethostbyname. Or have I missed something? Please explain.

The test above is changed by you, it probably tries "www.first.com" with the first DNS request, so no switch of the suffix is done anymore.

While I agree about the copy, please justify your statement about deletion, with the particular lines.

              /* Copy canonname in case we need it later */
              head_ai->ai_canonname = hostname;
              *ai = head_ai;
            }
           ares_free(addrs); // <-- here
          return ARES_SUCCESS;

The hostent branch in contrast reuses addrs for the result and makes no memcpy at all (just some assignments). This makes the hostent result efficient.

[ki11roy]

So how are you going to return this, and upcoming fixes, without copying them in two places?

I'm thinking of a function which implements the new requirement and that one gets called from both places.
But the question is another: ares_search is not considered to make a T_A and T_AAAA query for one suffix, before switching to the next suffix. This is a limitation of the function and (in my opinion) it therefore should not be used for getaddrinfo. So how will you get this working with ares_search?

I've explained how it works in the comments above. In short - it retries all the SERVFAILs and work with the suffixes too. There is an example which can't work according to your assumptions.

You are making an extra copy into completely different structure, while you can parse into the right place straight away. And that is not counting various ai_hints which can influence parsing too, for example canonical name.

ai_hints should not play a role when parsing the response. The canonical name is available from hostent. Only because we can add the result to an addrinfo structure and this saves us from one hostent allocation, that does not mean it's a good idea.

And judging by the test above it works without any fixes even now and even with ares_gethostbyname. Or have I missed something? Please explain.

The test above is changed by you,

It is written by me and works in trunk perfectly.

it probably tries "www.first.com" with the first DNS request, so no switch of the suffix is done anymore.

No it is not. Please explain how it works without referring to probabilities. It can't by your logic.

While I agree about the copy, please justify your statement about deletion, with the particular lines.

              /* Copy canonname in case we need it later */
              head_ai->ai_canonname = hostname;
              *ai = head_ai;
            }
           ares_free(addrs); // <-- here
          return ARES_SUCCESS;

The hostent branch in contrast reuses addrs for the result and makes no memcpy at all (just some assignments). This makes the hostent result efficient.

Can't see how more allocations and frees be possibly more efficient. Do you understand that this addresses are freed when freeing the hostent?
Let me explain all this step by step:
Hostname is a canonical name, which is always set into the hostent, and not always in addrinfo, only if you asked it via the hints. You are not using it in your code, as you don't have the hints.

Your code:

1. calling ares_parse
   1. allocates hostname
   2. allocates hostent
   3. allocates addresses
   4. allocates pointers to aliases
   5. allocates hostent's pointers to addresses (h_addr_list)
   6. copies hostname to hostent
   7. copies addresses
2. allocates addrinfo
3. allocates addresses
4. copies addresses into addrinfo (ignores all the rest)
5. frees hostent
   1. frees addresses
   2. frees pointers to addresses
   3. frees aliases
   4. frees canoname (hostname)

My code:

1. calling ares_parse
   1. allocates hostname
   2. allocates hostent
   3. allocates addresses
   4. allocates pointers to aliases
   5. allocates hostent's pointers to addresses (h_addr_list)
   6. allocates addrinfo
   7. copies hostname to addrinfo instead of copying into hostent
   8. copies addresses to addinfo instead of copying into hostent
   9. free addresses can't reuse as they are allocated with one malloc and we need them separated that step was in freeing hostent
2. allocates addrinfo moved into parse
3. allocates addresses it is done once in parse
4. copies addresses into addrinfo
   1. frees hostent
   2. frees addresses moved into parse
   3. frees pointers to aliases
   4. frees pointers to addresses
   5. frees canoname (hostname)
5. frees canoname (hostname) ** if it is not asked **

Code for the reference:

  status = ares__expand_name_for_response(aptr, abuf, alen, &hostname, &len);
  if (status != ARES_SUCCESS)
    return status;

...

  if (host)
    {
      /* Allocate addresses and aliases; ancount gives an upper bound for
         both. */
      addrs = ares_malloc(ancount * sizeof(struct in_addr));
      if (!addrs)
        {
          ares_free(hostname);
          return ARES_ENOMEM;
        }
      aliases = ares_malloc((ancount + 1) * sizeof(char *));
      if (!aliases)
        {
          ares_free(hostname);
          ares_free(addrs);
          return ARES_ENOMEM;
        }
    }

Well, we really can reuse addresses as we are controlling allocation and ares_free_addrinfo, but it involves deeper modification of parse function.

[ChristianAmmer-zz]

it probably tries "www.first.com" with the first DNS request, so no switch of the suffix is done anymore.

No it is not. Please explain how it works without referring to probabilities. It can't by your logic.

Ok, I tried your test with verbose output:

[ RUN      ] AddressFamilies/MockUDPChannelTest.SearchDomainsServFailOnAAAAThenSuccess/1
Configured IPv6 mock server with TCP socket 3 on port 5300 and UDP socket 4 on port 5300
Configured library with servers: [0000:0000:0000:0000:0000:0000:0000:0001]:5300
received UDP request REQ QRY RD  Q:{'www.first.com' IN AAAA} on port 5300
ProcessRequest(18901, 'www.first.com', AAAA)
sending reply RSP QRY AA SERVFAIL Q:{'www.first.com' IN AAAA} on port 5300
received UDP request REQ QRY RD  Q:{'www.first.com' IN AAAA} on port 5300
ProcessRequest(18901, 'www.first.com', AAAA)
sending reply RSP QRY AA SERVFAIL Q:{'www.first.com' IN AAAA} on port 5300
received UDP request REQ QRY RD  Q:{'www.first.com' IN AAAA} on port 5300
ProcessRequest(18901, 'www.first.com', AAAA)
sending reply RSP QRY NOERROR Q:{'www.first.com' IN A} A:{'www.first.com' IN A TTL=100 1.2.3.4} on port 5300
received UDP request REQ QRY RD  Q:{'www.first.com' IN A} on port 5300
ProcessRequest(23678, 'www.first.com', A)
sending reply RSP QRY NOERROR Q:{'www.first.com' IN A} A:{'www.first.com' IN A TTL=100 1.2.3.4} on port 5300
HostCallback({ARES_SUCCESS {'www.first.com' aliases=[] addrs=[1.2.3.4]}})
[       OK ] AddressFamilies/MockUDPChannelTest.SearchDomainsServFailOnAAAAThenSuccess/1 (6014 ms)

As you can see, it tries "www.first.com" with the first DNS request.

I'm not going to comment your list about allocations and copies. Without measuring it, your argument about efficiency is uncertain. But changing the function like you did makes it

• less readable: what happens to NULL vs non NULL arguments for host and ai
• less maintainable: changing the function implementation includes coping with two output arguments instead of one.

[ki11roy]

As you can see, it tries "www.first.com" with the first DNS request.

It tries and fails with the first request! And with the second and the third! And than it succeeds!

I'm not going to comment your list about allocations and copies.

I am addressing your assumptions that your code has less copies and allocations. That was your initial point:

Is not the full truth because you make extra allocations and copies in ares__parse_a(aaa)_reply. While the hostent branch just reuses an addrs variable for the hostent result, the addrinfo branch you introduced copies addrs and deletes it later.

The hostent branch in contrast reuses addrs for the result and makes no memcpy at all (just some assignments). This makes the hostent result efficient.

So I assume that was full truth and there are less allocations in my code. And my point was not about performance. It is about parsing into the right structure and supporting hints and cnames and other flags as I told before and less allocations as a consequence and a bonus.

[ChristianAmmer-zz]

It tries and fails with the first request! And with the second and the third! And than it succeeds!

It does not switch the suffixes as the name of the test states. Have a look at the other SearchDomain... tests.

I am addressing your assumptions that your code has less copies and allocations.

I did not made such an assumption.

[ki11roy]

It tries and fails with the first request! And with the second and the third! And than it succeeds!

It does not switch the suffixes as the name of the test states. Have a look at the other SearchDomain... tests.

Correct, the switching is not the task of this mechanism. So that returning us to the beginning:

But with an additional flag you will not solve the check-suffix-for-both-families-before-switch-to-next-suffix task. Which you already agreed with me.

So it will check suffix for both families as the test proves.

[ki11roy]

@bagder it seems that getaddrinfo is useless without proper sorting, which is a tricky and involves connecting to multiple addresses. I have partly adopted the version from https://android.googlesource.com/platform/bionic/+/2e23e29245aa42d0f9419187c94e72dba3888eef/libc/netbsd/net/getaddrinfo.c which seems compatible by license.
Two questions:

• is it ok to use the code
• will you need another PR

Update:
I have changed parse functions to simplify setting of canonical name and port and get rid of empty cname-only addrinfos. Canonical name goes into the first addrinfo as in the reference Linux getaddrinfo.

[ki11roy]

@bagder @bradh352 so what with the questions?

[bagder]

@ki11roy the license for that code is certainly with with us to use. If you use code straight from there you should make an effort to maintain that license blob.

I was a bit afraid that we needed sorting for it to be sensible, but I'm also glad that this is already discovered, realized and seemingly also soon fixed!

For me personally I think the sorting can go in as a separate PR, to make the review process slightly easier and reduce the total patch size of each PR. Also, since this is a new function that no existing user who would pull from github immediately would be using so having a short gap in time in the master branch with this "lack of feature" is not a concern.

[bradh352]

i'll review the comments and latest changeset this weekend. Any chance we can get the conflicts cleaned up before then?

[ki11roy]

i'll review the comments and latest changeset this weekend. Any chance we can get the conflicts cleaned up before then?

I'll try to fix it today, a bit later.

[ki11roy]

I have ported tests from #233 and commented out that one which wont work with the current ares_search wich I am using here. I think it is more like a bug somewhere between ares_gethostbyname, ares_search and ares_process and must be fixed somewhat similar with the 6bd2b89

[bradh352]

Just trying to take a peek at this, I see some comments regarding responses with no data results not properly continuing on, which is what I think #226 is trying to say, so you might have stumbled on the same bug? I haven't had time to try to reproduce myself.

I do agree the readability is reduced due to the wrappers for the parse a and aaaa replies and seems like a lot of additional code for no real gain over the initial implementation by @ChristianAmmer just copying the data from hostent over into addrinfo, or am I missing something and there is metadata that is missing?

[ki11roy]

@bradh352

Just trying to take a peek at this, I see some comments regarding responses with no data results not properly continuing on, which is what I think #226 is trying to say, so you might have stumbled on the same bug? I haven't had time to try to reproduce myself.

Could be, but I think it is another issue.

I do agree the readability is reduced due to the wrappers for the parse a and aaaa replies and seems like a lot of additional code for no real gain over the initial implementation by @ChristianAmmer just copying the data from hostent over into addrinfo, or am I missing something and there is metadata that is missing?

Well, initially I though about moving it into another file, like ares__parse_into_addrinfo and combine a and aaaa versions there. Yes, there are some fields which needed while parsing, port for example and canonical name. Moreover there is a TTL parameter which can be moved into ares_addrinfo so you wont't need ares_addrttl/ares_addr6ttl anymore. Will you agree on a separate file considering the above?

[bradh352]

Well, initially I though about moving it into another file, like ares__parse_into_addrinfo and combine a and aaaa versions there. Yes, there are some fields which needed while parsing, port for example and canonical name. Moreover there is a TTL parameter which can be moved into ares_addrinfo so you wont't need ares_addrttl/ares_addr6ttl anymore. Will you agree on a separate file considering the above?

I would personally love to have access to the TTL in the ares_addrinfo struct.

I think I can definitely get behind what you're saying, but I would like to have one additional thing done, and that would be to replace the existing ares_parse_a_reply() and ares_parse_aaaa_reply() with wrappers around your new ares__parse_into_addrinfo(). We don't have code that does similar things ... and since those are public functions, even if we choose not to use them internally, we need to keep them for compatibility.

[bradh352]

Just wanted to check in on the progress here. Thanks!

[ki11roy]

I want to split this PR into smaller ones, in the next couple of days.

[bradh352]

@k11roy just checking in ...

[ki11roy]

@bradh352 oh, sorry, looking into it

[bradh352]

@ki11roy hate to be a pain ... got other people opening issues on getaddrinfo() now that I think may be resolved by your solutions.

[ki11roy]

@bradh352 got a problem with CNAME TTL placement in ares_addrinfo structure. The possible solutions:

• add an extra ai_cname_ttl into ares_addrinfo.
• store cname_ttl between A/AAAA requests and change other ai_ttls afterwards accordingly.

The first one adds one integer per list element overhead, so the second one seems better. Will do the second variant.

[bradh352]

eh, 30 years ago I might be concerned about an additional integer.

These days though, I'd rather have more information in case needed. I think option #1 is better.

[ki11roy]

I have made #1 and it seems simpler indeed. Still have some tests to add and fix.

[bradh352]

great, thanks!

[bradh352]

@ki11roy just checking in

[ki11roy]

@bradh352 took some time to fix, please see #257

[bradh352]

#257 supersedes this