Hi Baptiste,
On Wed, May 24, 2017 at 11:34:31AM +0200, Baptiste wrote:
> Over the last few weeks, I entirely reworked the internal resolver of
> HAProxy to make it more flexible.
> The main driver for this is to add more features related to DNS use-cases
> (SRV records, scale in / scale out a backend, DNS converter, etc...) and
> also to make it more efficient internaly and more friendly with DNS servers
> ;)
(...)
I have a few comments :
1) The following construct to copy single characters :
memcpy(&str[len], "#", 1);
len += 1;
is better done this way :
str[len++] = '#';
I'm not changing it, it's not in a performance critical path.
2) patch 8 introduces a one-byte overflow in dns_cache_key() :
+ if (len + hostname_dn_len > size)
+ return NULL;
+ memcpy(&str[len], hostname_dn, hostname_dn_len);
+ len += hostname_dn_len;
Here len may be equal to size.
+ str[len] = '\0';
and here it's overwritten.
+ return buf;
I'm changing it this way :
+ if (len + hostname_dn_len + 1 > size) // +1 for trailing zero
+ return NULL;
3) in patch 10, there's a strange construct :
+ return_DNS_UPD_SRVIP_NOT_FOUND:
+ list_for_each_entry(record, &dns_p->answer_list, list) {
+ /* move the first record to the end of the list, for internal
round robin */
+ if (record) {
+ LIST_DEL(&record->list);
+ LIST_ADDQ(&dns_p->answer_list, &record->list);
+ break;
+ }
+ }
+ return DNS_UPD_SRVIP_NOT_FOUND;
I *think* you did this only to be sure to have a first element, but
that's not appropriate. A few points to keep in mind :
- list_for_each_entry() must not be used in cases where the element
is deleted. list_for_each_entry_safe() must be used for this. It
just happens to work in your case because you quit after the first
change, but this is very bug-prone.
- "if (record)" will always be true since it's the item carrying the
list you stopped on.
So I'm not totally sure about the intent there. I'm leaving it as-is
for now but I'd prefer it if you could send an update for this one. If
you just want to delete the first record and put it at the end only
when it exists, you can for example do this :
if (!LIST_ISEMPTY(&dns_p->answer_list)) {
struct list *first = dns_p->anwser_list->n;
LIST_DEL(first);
LIST_ADDQ(&dns_p->answer_list, first);
}
4) in patch 11, dns_dump_config() is not used anymore and looks like
debugging code as it dumps pointers. Once all the changes are done,
don't forget either to kill it, or to enclose it between #ifdef if
you prefer to keep it around.
By the way, I don't want to be nit-picking, but :
+ /* generates a query id */
+ i = 0;
+ do {
+ query_id = dns_rnd16();
+ /* we do try only 100 times to find a free query id */
+ if (i++ > 100) {
(...)
That's 101 times and not 100 :-)
Otherwise at first glance it looks good, I've merged your series.
thanks!
Willy
