[PATCH] dns: realloc address buffer after each packet

[PATCH] dns: realloc address buffer after each packet

[Josef Bacik]

Sometimes DNS responses come in slower than we poll for them which can lead us
to process multiple DNS packets which overflows the addresses array.  So instead
realloc the array each time to make sure we are accounting for any answers we
currently have in the address array.  We also move the caching of the addresses
outside of the recv hook so we can be sure to cache all the responses at once
instead of one packet at a time.  Thanks,

Signed-off-by: Josef Bacik <jbacik@fb.com>
---
 grub-core/net/dns.c | 65 ++++++++++++++++++++++++++++-------------------------
 1 file changed, 34 insertions(+), 31 deletions(-)

diff --git a/grub-core/net/dns.c b/grub-core/net/dns.c
index 89741dd..d8258c6 100644
--- a/grub-core/net/dns.c
+++ b/grub-core/net/dns.c
@@ -109,11 +109,10 @@ struct recv_data
 {
   grub_size_t *naddresses;
   struct grub_net_network_level_address **addresses;
-  int cache;
   grub_uint16_t id;
+  grub_uint32_t ttl;
   int dns_err;
   char *name;
-  const char *oname;
   int stop;
 };
 
@@ -230,6 +229,7 @@ recv_hook (grub_net_udp_socket_t sock __attribute__ ((unused)),
 	   struct grub_net_buff *nb,
 	   void *data_)
 {
+  struct grub_net_network_level_address *addresses;
   struct dns_header *head;
   struct recv_data *data = data_;
   int i, j;
@@ -276,14 +276,17 @@ recv_hook (grub_net_udp_socket_t sock __attribute__ ((unused)),
       ptr++;
       ptr += 4;
     }
-  *data->addresses = grub_malloc (sizeof ((*data->addresses)[0])
-				 * grub_be_to_cpu16 (head->ancount));
-  if (!*data->addresses)
+  addresses = grub_realloc (*data->addresses,
+			    sizeof ((*data->addresses)[0])
+			    * (grub_be_to_cpu16 (head->ancount)
+			       + *data->naddresses));
+  if (!addresses)
     {
       grub_errno = GRUB_ERR_NONE;
       grub_netbuff_free (nb);
       return GRUB_ERR_NONE;
     }
+  *data->addresses = addresses;
   reparse_ptr = ptr;
  reparse:
   for (i = 0, ptr = reparse_ptr; i < grub_be_to_cpu16 (head->ancount); i++)
@@ -386,31 +389,6 @@ recv_hook (grub_net_udp_socket_t sock __attribute__ ((unused)),
 	}
       ptr += length;
     }
-  if (ttl_all && *data->naddresses && data->cache)
-    {
-      int h;
-      grub_dprintf ("dns", "caching for %d seconds\n", ttl_all);
-      h = hash (data->oname);
-      grub_free (dns_cache[h].name);
-      dns_cache[h].name = 0;
-      grub_free (dns_cache[h].addresses);
-      dns_cache[h].addresses = 0;
-      dns_cache[h].name = grub_strdup (data->oname);
-      dns_cache[h].naddresses = *data->naddresses;
-      dns_cache[h].addresses = grub_malloc (*data->naddresses
-					    * sizeof (dns_cache[h].addresses[0]));
-      dns_cache[h].limit_time = grub_get_time_ms () + 1000 * ttl_all;
-      if (!dns_cache[h].addresses || !dns_cache[h].name)
-	{
-	  grub_free (dns_cache[h].name);
-	  dns_cache[h].name = 0;
-	  grub_free (dns_cache[h].addresses);
-	  dns_cache[h].addresses = 0;
-	}
-      grub_memcpy (dns_cache[h].addresses, *data->addresses,
-		   *data->naddresses
-		   * sizeof (dns_cache[h].addresses[0]));
-    }
   grub_netbuff_free (nb);
   grub_free (redirect_save);
   return GRUB_ERR_NONE;
@@ -435,7 +413,7 @@ grub_net_dns_lookup (const char *name,
   grub_uint8_t *qtypeptr;
   grub_err_t err = GRUB_ERR_NONE;
   struct recv_data data = {naddresses, addresses, cache,
-			   grub_cpu_to_be16 (id++), 0, 0, name, 0};
+			   grub_cpu_to_be16 (id++), ~0U, 0, 0};
   grub_uint8_t *nbd;
   grub_size_t try_server = 0;
 
@@ -602,6 +580,31 @@ grub_net_dns_lookup (const char *name,
   
   grub_free (sockets);
 
+  if (data.ttl && *data.naddresses && cache)
+    {
+      int h;
+      grub_dprintf ("dns", "caching for %d seconds\n", data.ttl);
+      h = hash (name);
+      grub_free (dns_cache[h].name);
+      dns_cache[h].name = 0;
+      grub_free (dns_cache[h].addresses);
+      dns_cache[h].addresses = 0;
+      dns_cache[h].name = grub_strdup (name);
+      dns_cache[h].naddresses = *data.naddresses;
+      dns_cache[h].addresses = grub_malloc (*data.naddresses
+					    * sizeof (dns_cache[h].addresses[0]));
+      dns_cache[h].limit_time = grub_get_time_ms () + 1000 * data.ttl;
+      if (!dns_cache[h].addresses || !dns_cache[h].name)
+	{
+	  grub_free (dns_cache[h].name);
+	  dns_cache[h].name = 0;
+	  grub_free (dns_cache[h].addresses);
+	  dns_cache[h].addresses = 0;
+	}
+      grub_memcpy (dns_cache[h].addresses, *data.addresses,
+		   *data.naddresses
+		   * sizeof (dns_cache[h].addresses[0]));
+    }
   if (*data.naddresses)
     return GRUB_ERR_NONE;
   if (data.dns_err)
-- 
2.5.0

Re: [PATCH] dns: realloc address buffer after each packet

[Andrei Borzenkov]

On Wed, Feb 24, 2016 at 10:11 PM, Josef Bacik <jbacik@fb.com> wrote:
> Sometimes DNS responses come in slower than we poll for them which can lead us
> to process multiple DNS packets which overflows the addresses array.  So instead
> realloc the array each time to make sure we are accounting for any answers we
> currently have in the address array.  We also move the caching of the addresses
> outside of the recv hook so we can be sure to cache all the responses at once
> instead of one packet at a time.  Thanks,
>

This still does not address the problem that we stop waiting for
further packets as soon as we get any response, so we still depend on
delivery order to get correct record.

What about following

- send both A and AAAA query concurrently if requested
- keep track of both requests (i.e. have data.id[2] and data.addresses[2])
- reset request ID (or otherwise mark it as "received") as soon as we
got reply. Note that reply may contain no addresses - NXDOMAIN is
prerfectly valid - so condition should not be "got any record of type
A or AAAA" as it is now but rather simply "got reply to request with
id XX". This also allows us to implement negative caching at some
point :)
- return both A and AAAA results separately to grub_net_dns_lookup()
- combine them in grub_net_dns_lookup() depending on preference - i.e.
put either A or AAAA first in final result.

This seems to cover all issues so far - we do not wait too long, we
are guaranteed to get both A and AAAA if we request them and we return
them in proper order for further processing.

Do I miss something?

> Signed-off-by: Josef Bacik <jbacik@fb.com>
> ---
>  grub-core/net/dns.c | 65 ++++++++++++++++++++++++++++-------------------------
>  1 file changed, 34 insertions(+), 31 deletions(-)
>
> diff --git a/grub-core/net/dns.c b/grub-core/net/dns.c
> index 89741dd..d8258c6 100644
> --- a/grub-core/net/dns.c
> +++ b/grub-core/net/dns.c
> @@ -109,11 +109,10 @@ struct recv_data
>  {
>    grub_size_t *naddresses;
>    struct grub_net_network_level_address **addresses;
> -  int cache;
>    grub_uint16_t id;
> +  grub_uint32_t ttl;
>    int dns_err;
>    char *name;
> -  const char *oname;
>    int stop;
>  };
>
> @@ -230,6 +229,7 @@ recv_hook (grub_net_udp_socket_t sock __attribute__ ((unused)),
>            struct grub_net_buff *nb,
>            void *data_)
>  {
> +  struct grub_net_network_level_address *addresses;
>    struct dns_header *head;
>    struct recv_data *data = data_;
>    int i, j;
> @@ -276,14 +276,17 @@ recv_hook (grub_net_udp_socket_t sock __attribute__ ((unused)),
>        ptr++;
>        ptr += 4;
>      }
> -  *data->addresses = grub_malloc (sizeof ((*data->addresses)[0])
> -                                * grub_be_to_cpu16 (head->ancount));
> -  if (!*data->addresses)
> +  addresses = grub_realloc (*data->addresses,
> +                           sizeof ((*data->addresses)[0])
> +                           * (grub_be_to_cpu16 (head->ancount)
> +                              + *data->naddresses));
> +  if (!addresses)
>      {
>        grub_errno = GRUB_ERR_NONE;
>        grub_netbuff_free (nb);
>        return GRUB_ERR_NONE;
>      }
> +  *data->addresses = addresses;
>    reparse_ptr = ptr;
>   reparse:
>    for (i = 0, ptr = reparse_ptr; i < grub_be_to_cpu16 (head->ancount); i++)
> @@ -386,31 +389,6 @@ recv_hook (grub_net_udp_socket_t sock __attribute__ ((unused)),
>         }
>        ptr += length;
>      }
> -  if (ttl_all && *data->naddresses && data->cache)
> -    {
> -      int h;
> -      grub_dprintf ("dns", "caching for %d seconds\n", ttl_all);
> -      h = hash (data->oname);
> -      grub_free (dns_cache[h].name);
> -      dns_cache[h].name = 0;
> -      grub_free (dns_cache[h].addresses);
> -      dns_cache[h].addresses = 0;
> -      dns_cache[h].name = grub_strdup (data->oname);
> -      dns_cache[h].naddresses = *data->naddresses;
> -      dns_cache[h].addresses = grub_malloc (*data->naddresses
> -                                           * sizeof (dns_cache[h].addresses[0]));
> -      dns_cache[h].limit_time = grub_get_time_ms () + 1000 * ttl_all;
> -      if (!dns_cache[h].addresses || !dns_cache[h].name)
> -       {
> -         grub_free (dns_cache[h].name);
> -         dns_cache[h].name = 0;
> -         grub_free (dns_cache[h].addresses);
> -         dns_cache[h].addresses = 0;
> -       }
> -      grub_memcpy (dns_cache[h].addresses, *data->addresses,
> -                  *data->naddresses
> -                  * sizeof (dns_cache[h].addresses[0]));
> -    }
>    grub_netbuff_free (nb);
>    grub_free (redirect_save);
>    return GRUB_ERR_NONE;
> @@ -435,7 +413,7 @@ grub_net_dns_lookup (const char *name,
>    grub_uint8_t *qtypeptr;
>    grub_err_t err = GRUB_ERR_NONE;
>    struct recv_data data = {naddresses, addresses, cache,
> -                          grub_cpu_to_be16 (id++), 0, 0, name, 0};
> +                          grub_cpu_to_be16 (id++), ~0U, 0, 0};
>    grub_uint8_t *nbd;
>    grub_size_t try_server = 0;
>
> @@ -602,6 +580,31 @@ grub_net_dns_lookup (const char *name,
>
>    grub_free (sockets);
>
> +  if (data.ttl && *data.naddresses && cache)
> +    {
> +      int h;
> +      grub_dprintf ("dns", "caching for %d seconds\n", data.ttl);
> +      h = hash (name);
> +      grub_free (dns_cache[h].name);
> +      dns_cache[h].name = 0;
> +      grub_free (dns_cache[h].addresses);
> +      dns_cache[h].addresses = 0;
> +      dns_cache[h].name = grub_strdup (name);
> +      dns_cache[h].naddresses = *data.naddresses;
> +      dns_cache[h].addresses = grub_malloc (*data.naddresses
> +                                           * sizeof (dns_cache[h].addresses[0]));
> +      dns_cache[h].limit_time = grub_get_time_ms () + 1000 * data.ttl;
> +      if (!dns_cache[h].addresses || !dns_cache[h].name)
> +       {
> +         grub_free (dns_cache[h].name);
> +         dns_cache[h].name = 0;
> +         grub_free (dns_cache[h].addresses);
> +         dns_cache[h].addresses = 0;
> +       }
> +      grub_memcpy (dns_cache[h].addresses, *data.addresses,
> +                  *data.naddresses
> +                  * sizeof (dns_cache[h].addresses[0]));
> +    }
>    if (*data.naddresses)
>      return GRUB_ERR_NONE;
>    if (data.dns_err)
> --
> 2.5.0
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] dns: realloc address buffer after each packet

[Josef Bacik]

On 02/26/2016 05:22 AM, Andrei Borzenkov wrote:
> On Wed, Feb 24, 2016 at 10:11 PM, Josef Bacik <jbacik@fb.com> wrote:
>> Sometimes DNS responses come in slower than we poll for them which can lead us
>> to process multiple DNS packets which overflows the addresses array.  So instead
>> realloc the array each time to make sure we are accounting for any answers we
>> currently have in the address array.  We also move the caching of the addresses
>> outside of the recv hook so we can be sure to cache all the responses at once
>> instead of one packet at a time.  Thanks,
>>
>
> This still does not address the problem that we stop waiting for
> further packets as soon as we get any response, so we still depend on
> delivery order to get correct record.
>
> What about following
>
> - send both A and AAAA query concurrently if requested
> - keep track of both requests (i.e. have data.id[2] and data.addresses[2])
> - reset request ID (or otherwise mark it as "received") as soon as we
> got reply. Note that reply may contain no addresses - NXDOMAIN is
> prerfectly valid - so condition should not be "got any record of type
> A or AAAA" as it is now but rather simply "got reply to request with
> id XX". This also allows us to implement negative caching at some
> point :)

So we check the rcode so a NXDOMAIN response will just be discarded, we 
don't have to worry about this case.

> - return both A and AAAA results separately to grub_net_dns_lookup()
> - combine them in grub_net_dns_lookup() depending on preference - i.e.
> put either A or AAAA first in final result.
>
> This seems to cover all issues so far - we do not wait too long, we
> are guaranteed to get both A and AAAA if we request them and we return
> them in proper order for further processing.
>
> Do I miss something?
>

So my patch previous to this one changes it so DNS servers we get from 
dhcp are bound to either ipv4 or ipv6, so the only way we get 
PREFER_IPV* is if an admin sets it.  So now the only time we'll get both 
an A _and_ an AAAA record is if we have both types of interfaces on the 
system, so it won't matter which answer is first as we'll be able to 
send traffic to either.  I just side-stepped the issue by making it so 
that (at least automatically) we aren't asking for both records unless 
we are sure we have an interface to go along with both types of records. 
  Thanks,

Josef

Re: [PATCH] dns: realloc address buffer after each packet

[Andrei Borzenkov]

26.02.2016 16:52, Josef Bacik пишет:
> On 02/26/2016 05:22 AM, Andrei Borzenkov wrote:
>> On Wed, Feb 24, 2016 at 10:11 PM, Josef Bacik <jbacik@fb.com> wrote:
>>> Sometimes DNS responses come in slower than we poll for them which
>>> can lead us
>>> to process multiple DNS packets which overflows the addresses array. 
>>> So instead
>>> realloc the array each time to make sure we are accounting for any
>>> answers we
>>> currently have in the address array.  We also move the caching of the
>>> addresses
>>> outside of the recv hook so we can be sure to cache all the responses
>>> at once
>>> instead of one packet at a time.  Thanks,
>>>
>>
>> This still does not address the problem that we stop waiting for
>> further packets as soon as we get any response, so we still depend on
>> delivery order to get correct record.
>>
>> What about following
>>
>> - send both A and AAAA query concurrently if requested
>> - keep track of both requests (i.e. have data.id[2] and
>> data.addresses[2])
>> - reset request ID (or otherwise mark it as "received") as soon as we
>> got reply. Note that reply may contain no addresses - NXDOMAIN is
>> prerfectly valid - so condition should not be "got any record of type
>> A or AAAA" as it is now but rather simply "got reply to request with
>> id XX". This also allows us to implement negative caching at some
>> point :)
> 
> So we check the rcode so a NXDOMAIN response will just be discarded, we
> don't have to worry about this case.
> 
>> - return both A and AAAA results separately to grub_net_dns_lookup()
>> - combine them in grub_net_dns_lookup() depending on preference - i.e.
>> put either A or AAAA first in final result.
>>
>> This seems to cover all issues so far - we do not wait too long, we
>> are guaranteed to get both A and AAAA if we request them and we return
>> them in proper order for further processing.
>>
>> Do I miss something?
>>
> 
> So my patch previous to this one changes it so DNS servers we get from
> dhcp are bound to either ipv4 or ipv6, so the only way we get
> PREFER_IPV* is if an admin sets it.

In this case I do not follow why you want to collect multiple answers in
the first place. The only reason for me was to make sure we have both A
and AAAA replies; otherwise every reply packet is supposed to contain
exactly the same content. Following this logic the right thing to do is
to just drop all subsequent duplicated replies.

>  So now the only time we'll get both
> an A _and_ an AAAA record is if we have both types of interfaces on the
> system, so it won't matter which answer is first as we'll be able to
> send traffic to either.

User can set preference using net_add_dns --prefer-ipvX | --only-ipvX.
We should respect it.

>  I just side-stepped the issue by making it so
> that (at least automatically) we aren't asking for both records unless
> we are sure we have an interface to go along with both types of records.

I'll reply to your other patch.

Re: [PATCH] dns: realloc address buffer after each packet

[Josef Bacik]

On 02/27/2016 12:39 PM, Andrei Borzenkov wrote:
> 26.02.2016 16:52, Josef Bacik пишет:
>> On 02/26/2016 05:22 AM, Andrei Borzenkov wrote:
>>> On Wed, Feb 24, 2016 at 10:11 PM, Josef Bacik <jbacik@fb.com> wrote:
>>>> Sometimes DNS responses come in slower than we poll for them which
>>>> can lead us
>>>> to process multiple DNS packets which overflows the addresses array.
>>>> So instead
>>>> realloc the array each time to make sure we are accounting for any
>>>> answers we
>>>> currently have in the address array.  We also move the caching of the
>>>> addresses
>>>> outside of the recv hook so we can be sure to cache all the responses
>>>> at once
>>>> instead of one packet at a time.  Thanks,
>>>>
>>>
>>> This still does not address the problem that we stop waiting for
>>> further packets as soon as we get any response, so we still depend on
>>> delivery order to get correct record.
>>>
>>> What about following
>>>
>>> - send both A and AAAA query concurrently if requested
>>> - keep track of both requests (i.e. have data.id[2] and
>>> data.addresses[2])
>>> - reset request ID (or otherwise mark it as "received") as soon as we
>>> got reply. Note that reply may contain no addresses - NXDOMAIN is
>>> prerfectly valid - so condition should not be "got any record of type
>>> A or AAAA" as it is now but rather simply "got reply to request with
>>> id XX". This also allows us to implement negative caching at some
>>> point :)
>>
>> So we check the rcode so a NXDOMAIN response will just be discarded, we
>> don't have to worry about this case.
>>
>>> - return both A and AAAA results separately to grub_net_dns_lookup()
>>> - combine them in grub_net_dns_lookup() depending on preference - i.e.
>>> put either A or AAAA first in final result.
>>>
>>> This seems to cover all issues so far - we do not wait too long, we
>>> are guaranteed to get both A and AAAA if we request them and we return
>>> them in proper order for further processing.
>>>
>>> Do I miss something?
>>>
>>
>> So my patch previous to this one changes it so DNS servers we get from
>> dhcp are bound to either ipv4 or ipv6, so the only way we get
>> PREFER_IPV* is if an admin sets it.
>
> In this case I do not follow why you want to collect multiple answers in
> the first place. The only reason for me was to make sure we have both A
> and AAAA replies; otherwise every reply packet is supposed to contain
> exactly the same content. Following this logic the right thing to do is
> to just drop all subsequent duplicated replies.

I don't want to collect multiple answers.  We bail as soon as we get an 
answer we care about.  If a user sets --prefer-ipv6 then they must have 
an environment that also gives them an ipv4 address so they can handle 
an A record response.  I want to make sure that we only get both an A 
and AAAA record when we have both kinds of ipvX interfaces, and if not 
we bail as soon as we get one we want.

>
>>   So now the only time we'll get both
>> an A _and_ an AAAA record is if we have both types of interfaces on the
>> system, so it won't matter which answer is first as we'll be able to
>> send traffic to either.
>
> User can set preference using net_add_dns --prefer-ipvX | --only-ipvX.
> We should respect it.
>

And we still will, prefer means we try but we don't make guarantees and 
the --only will work as it has always worked.  Thanks,

Josef

Re: [PATCH] dns: realloc address buffer after each packet

[Andrei Borzenkov]

01.03.2016 19:38, Josef Bacik пишет:
> On 02/27/2016 12:39 PM, Andrei Borzenkov wrote:
>> 26.02.2016 16:52, Josef Bacik пишет:
>>> On 02/26/2016 05:22 AM, Andrei Borzenkov wrote:
>>>> On Wed, Feb 24, 2016 at 10:11 PM, Josef Bacik <jbacik@fb.com> wrote:
>>>>> Sometimes DNS responses come in slower than we poll for them which
>>>>> can lead us
>>>>> to process multiple DNS packets which overflows the addresses array.
>>>>> So instead
>>>>> realloc the array each time to make sure we are accounting for any
>>>>> answers we
>>>>> currently have in the address array.  We also move the caching of the
>>>>> addresses
>>>>> outside of the recv hook so we can be sure to cache all the responses
>>>>> at once
>>>>> instead of one packet at a time.  Thanks,
>>>>>
>>>>
>>>> This still does not address the problem that we stop waiting for
>>>> further packets as soon as we get any response, so we still depend on
>>>> delivery order to get correct record.
>>>>
>>>> What about following
>>>>
>>>> - send both A and AAAA query concurrently if requested
>>>> - keep track of both requests (i.e. have data.id[2] and
>>>> data.addresses[2])
>>>> - reset request ID (or otherwise mark it as "received") as soon as we
>>>> got reply. Note that reply may contain no addresses - NXDOMAIN is
>>>> prerfectly valid - so condition should not be "got any record of type
>>>> A or AAAA" as it is now but rather simply "got reply to request with
>>>> id XX". This also allows us to implement negative caching at some
>>>> point :)
>>>
>>> So we check the rcode so a NXDOMAIN response will just be discarded, we
>>> don't have to worry about this case.
>>>
>>>> - return both A and AAAA results separately to grub_net_dns_lookup()
>>>> - combine them in grub_net_dns_lookup() depending on preference - i.e.
>>>> put either A or AAAA first in final result.
>>>>
>>>> This seems to cover all issues so far - we do not wait too long, we
>>>> are guaranteed to get both A and AAAA if we request them and we return
>>>> them in proper order for further processing.
>>>>
>>>> Do I miss something?
>>>>
>>>
>>> So my patch previous to this one changes it so DNS servers we get from
>>> dhcp are bound to either ipv4 or ipv6, so the only way we get
>>> PREFER_IPV* is if an admin sets it.
>>
>> In this case I do not follow why you want to collect multiple answers in
>> the first place. The only reason for me was to make sure we have both A
>> and AAAA replies; otherwise every reply packet is supposed to contain
>> exactly the same content. Following this logic the right thing to do is
>> to just drop all subsequent duplicated replies.
> 
> I don't want to collect multiple answers.  We bail as soon as we get an
> answer we care about. 

No, we do not. In DNS receive hook we set stop condition and bail out
when grub_net_poll_cards() checks it. But it checks it only after going
through all active cards. Multiple DNS servers may be reachable through
different cards so we can have multiple packets from different servers
in the same iteration, before stop condition is checked. And even with
single active card we may still get multiple packets even from single
server in case replies were delayed because receive_packets() consumes
multiple packets if present.

> If a user sets --prefer-ipv6 then they must have
> an environment that also gives them an ipv4 address so they can handle
> an A record response.  I want to make sure that we only get both an A
> and AAAA record when we have both kinds of ipvX interfaces, and if not
> we bail as soon as we get one we want.
> 
>>
>>>   So now the only time we'll get both
>>> an A _and_ an AAAA record is if we have both types of interfaces on the
>>> system, so it won't matter which answer is first as we'll be able to
>>> send traffic to either.
>>
>> User can set preference using net_add_dns --prefer-ipvX | --only-ipvX.
>> We should respect it.
>>
> 
> And we still will, prefer means we try but we don't make guarantees and
> the --only will work as it has always worked.  Thanks,
> 
> Josef

Re: [PATCH] dns: realloc address buffer after each packet

[Josef Bacik]

On 03/01/2016 02:22 PM, Andrei Borzenkov wrote:
> 01.03.2016 19:38, Josef Bacik пишет:
>> On 02/27/2016 12:39 PM, Andrei Borzenkov wrote:
>>> 26.02.2016 16:52, Josef Bacik пишет:
>>>> On 02/26/2016 05:22 AM, Andrei Borzenkov wrote:
>>>>> On Wed, Feb 24, 2016 at 10:11 PM, Josef Bacik <jbacik@fb.com> wrote:
>>>>>> Sometimes DNS responses come in slower than we poll for them which
>>>>>> can lead us
>>>>>> to process multiple DNS packets which overflows the addresses array.
>>>>>> So instead
>>>>>> realloc the array each time to make sure we are accounting for any
>>>>>> answers we
>>>>>> currently have in the address array.  We also move the caching of the
>>>>>> addresses
>>>>>> outside of the recv hook so we can be sure to cache all the responses
>>>>>> at once
>>>>>> instead of one packet at a time.  Thanks,
>>>>>>
>>>>>
>>>>> This still does not address the problem that we stop waiting for
>>>>> further packets as soon as we get any response, so we still depend on
>>>>> delivery order to get correct record.
>>>>>
>>>>> What about following
>>>>>
>>>>> - send both A and AAAA query concurrently if requested
>>>>> - keep track of both requests (i.e. have data.id[2] and
>>>>> data.addresses[2])
>>>>> - reset request ID (or otherwise mark it as "received") as soon as we
>>>>> got reply. Note that reply may contain no addresses - NXDOMAIN is
>>>>> prerfectly valid - so condition should not be "got any record of type
>>>>> A or AAAA" as it is now but rather simply "got reply to request with
>>>>> id XX". This also allows us to implement negative caching at some
>>>>> point :)
>>>>
>>>> So we check the rcode so a NXDOMAIN response will just be discarded, we
>>>> don't have to worry about this case.
>>>>
>>>>> - return both A and AAAA results separately to grub_net_dns_lookup()
>>>>> - combine them in grub_net_dns_lookup() depending on preference - i.e.
>>>>> put either A or AAAA first in final result.
>>>>>
>>>>> This seems to cover all issues so far - we do not wait too long, we
>>>>> are guaranteed to get both A and AAAA if we request them and we return
>>>>> them in proper order for further processing.
>>>>>
>>>>> Do I miss something?
>>>>>
>>>>
>>>> So my patch previous to this one changes it so DNS servers we get from
>>>> dhcp are bound to either ipv4 or ipv6, so the only way we get
>>>> PREFER_IPV* is if an admin sets it.
>>>
>>> In this case I do not follow why you want to collect multiple answers in
>>> the first place. The only reason for me was to make sure we have both A
>>> and AAAA replies; otherwise every reply packet is supposed to contain
>>> exactly the same content. Following this logic the right thing to do is
>>> to just drop all subsequent duplicated replies.
>>
>> I don't want to collect multiple answers.  We bail as soon as we get an
>> answer we care about.
>
> No, we do not. In DNS receive hook we set stop condition and bail out
> when grub_net_poll_cards() checks it. But it checks it only after going
> through all active cards. Multiple DNS servers may be reachable through
> different cards so we can have multiple packets from different servers
> in the same iteration, before stop condition is checked. And even with
> single active card we may still get multiple packets even from single
> server in case replies were delayed because receive_packets() consumes
> multiple packets if present.
>

Sigh sorry I had my old patch in my head where I a) didn't add things to 
the cache if we had a preference and b) stopped once we set data->stop. 
  I'll fix this up.  Thanks,

Josef