[Asterisk-code-review] Add a negative DNS cache (asterisk[13])

[Joshua Colp]

Joshua Colp has posted comments on this change.

Change subject: Add a negative DNS cache
......................................................................


Patch Set 2: Code-Review-1

(8 comments)

I also think that the naming of the DNS cache API should be changed to explicitly state it is a negative cache API to avoid confusion.

https://gerrit.asterisk.org/#/c/1709/2/main/dns_cache.c
File main/dns_cache.c:

Line 33: #define MAX_NEGATIVE_TTL 10
This is a poor choice of a name for this.


Line 38: #define MAX_CACHE_ITEMS 256
I'd say raise the max some, on a heavy system there may be much more than this depending on circumstances.


Line 70: This task is responsible for checking the expiration of each
       :  * entry and removing the entry if it has expired.
This isn't quite true. It's for blatantly stale entries.


Line 79: static ast_mutex_t sched_lock;
Use AST_MUTEX_DEFINE_STATIC cause as it is you haven't initialized this.


Line 152: 	return time(NULL) > item->ttl ? CMP_MATCH : 0;
I'm not sure I agree with this. You really have two times:

1. The TTL of the entry itself according to the negative cache
2. When the entry was last accessed

If an entry is being frequently accessed then it's not stale and you shouldn't remove it. I think this should be a separate field.


Line 161: 	return dns_cache_scheduler_stop(0);
Add a comment explaining why this can't delete the item


Line 183: 	int size = strlen(name) + 1;
size_t


Line 192: 	ast_copy_string(item->name, name, size);
State that this is safe


-- 
To view, visit https://gerrit.asterisk.org/1709
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Icb9f8c7ec15eedd4701461bf865e8036a5732388
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-HasComments: Yes