Skip to content

Commit

Permalink
Merge pull request #1322 from pi-hole/fix/negative_replies
Browse files Browse the repository at this point in the history 
Fix query counts
  • Loading branch information
@yubiuser
yubiuser authored Apr 2, 2022
2 parents 392ff15 + 8734b60 commit 4f6f6ca
Show file tree
Hide file tree
Showing 4 changed files with 65 additions and 53 deletions.
10 changes: 7 additions & 3 deletions src/api/api.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,9 +121,13 @@ void getStats(const int *sock)
ssend(*sock, "dns_queries_all_types %i\n", sumalltypes);

// Send individual reply type counters
ssend(*sock, "reply_NODATA %i\nreply_NXDOMAIN %i\nreply_CNAME %i\nreply_IP %i\n",
counters->reply[REPLY_NODATA], counters->reply[REPLY_NXDOMAIN],
counters->reply[REPLY_CNAME], counters->reply[REPLY_IP]);
int sumallreplies = 0;
for(enum reply_type reply = REPLY_UNKNOWN; reply < QUERY_REPLY_MAX; reply++)
{
ssend(*sock, "reply_%s %i\n", get_query_reply_str(reply), counters->reply[reply]);
sumallreplies += counters->reply[reply];
}
ssend(*sock, "dns_queries_all_replies %i\n", sumallreplies);
ssend(*sock, "privacy_level %i\n", config.privacylevel);
}
else
Expand Down
12 changes: 9 additions & 3 deletions src/database/query-table.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -867,7 +867,14 @@ void DB_read_queries(void)
if(type < 100)
{
// Mapped query type
query->type = type;
if(type >= TYPE_A && type < TYPE_MAX)
query->type = type;
else
{
// Invalid query type
logg("DB warn: Query type %d is invalid.", type);
continue;
}
}
else
{
Expand Down Expand Up @@ -900,8 +907,7 @@ void DB_read_queries(void)
client->lastQuery = queryTimeStamp;

// Handle type counters
if(type >= TYPE_A && type < TYPE_MAX)
counters->querytype[type-1]++;
counters->querytype[query->type-1]++;

// Update overTime data
overTime[timeidx].total++;
Expand Down
71 changes: 31 additions & 40 deletions src/dnsmasq_interface.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@
#include "database/message-table.h"

// Private prototypes
static const char *reply_status_str[QUERY_REPLY_MAX+1];
static void print_flags(const unsigned int flags);
#define query_set_reply(flags, type, addr, query, response) _query_set_reply(flags, type, addr, query, response, __FILE__, __LINE__)
static void _query_set_reply(const unsigned int flags, const enum reply_type reply, const union all_addr *addr, queriesData* query,
Expand Down Expand Up @@ -664,9 +663,6 @@ bool _FTL_new_query(const unsigned int flags, const char *name,
id, queryID, short_path(file), line);
}

// Update counters
counters->querytype[querytype-1]++;

// Update overTime
const unsigned int timeidx = getOverTimeID(querytimestamp);

Expand Down Expand Up @@ -720,6 +716,7 @@ bool _FTL_new_query(const unsigned int flags, const char *name,
query->flags.response_calculated = false;
// Initialize reply type
query->reply = REPLY_UNKNOWN;
counters->reply[REPLY_UNKNOWN]++;
// Store DNSSEC result for this domain
query->dnssec = DNSSEC_UNSPECIFIED;
// Every domain is insecure in the beginning. It can get secure or bogus
Expand Down Expand Up @@ -755,6 +752,9 @@ bool _FTL_new_query(const unsigned int flags, const char *name,
client->lastQuery = querytimestamp;
client->numQueriesARP++;

// Update counters
counters->querytype[querytype-1]++;

// Process interface information of client (if available)
// Skip interface name length 1 to skip "-". No real interface should
// have a name with a length of 1...
Expand Down Expand Up @@ -1449,7 +1449,7 @@ static bool _FTL_check_blocking(int queryID, int domainID, int clientID, const c
logg("Blocking %s as %s is %s", domainstr, blockedDomain, blockingreason);

if(force_next_DNS_reply != 0)
logg("Forcing next reply to %s", reply_status_str[force_next_DNS_reply]);
logg("Forcing next reply to %s", get_query_reply_str(force_next_DNS_reply));
}
else if(db_okay)
{
Expand Down Expand Up @@ -2570,95 +2570,86 @@ void print_flags(const unsigned int flags)
free(flagstr);
}

static const char *reply_status_str[QUERY_REPLY_MAX+1] = {
"UNKNOWN",
"NODATA",
"NXDOMAIN",
"CNAME",
"IP",
"DOMAIN",
"RRNAME",
"SERVFAIL",
"REFUSED",
"NOTIMP",
"OTHER",
"DNSSEC",
"NONE",
"BLOB",
"MAX"
};

static void _query_set_reply(const unsigned int flags, const enum reply_type reply,
const union all_addr *addr,
queriesData *query, const struct timeval response,
const char *file, const int line)
{
enum reply_type new_reply = REPLY_UNKNOWN;
// If reply is set, we use it directly instead of interpreting the flags
if(reply != 0)
{
query->reply = reply;
new_reply = reply;
}
// else: Iterate through possible values by analyzing both the flags and the addr bits
else if(flags & F_NEG ||
(flags & F_NOERR && !(flags & (F_IPV4 | F_IPV6))) || // <-- FTL_make_answer() when no A or AAAA is added
force_next_DNS_reply == REPLY_NXDOMAIN ||
force_next_DNS_reply == REPLY_NODATA)
(flags & F_NOERR && !(flags & (F_IPV4 | F_IPV6))) || // <-- FTL_make_answer() when no A or AAAA is added
force_next_DNS_reply == REPLY_NXDOMAIN ||
force_next_DNS_reply == REPLY_NODATA)
{
if(flags & F_NXDOMAIN || force_next_DNS_reply == REPLY_NXDOMAIN)
// NXDOMAIN
query->reply = REPLY_NXDOMAIN;
new_reply = REPLY_NXDOMAIN;
else
// NODATA(-IPv6)
query->reply = REPLY_NODATA;
new_reply = REPLY_NODATA;
}
else if(flags & F_CNAME)
// <CNAME>
query->reply = REPLY_CNAME;
new_reply = REPLY_CNAME;
else if(flags & F_REVERSE)
// reserve lookup
query->reply = REPLY_DOMAIN;
new_reply = REPLY_DOMAIN;
else if(flags & F_RRNAME)
// TXT query
query->reply = REPLY_RRNAME;
new_reply = REPLY_RRNAME;
else if((flags & F_RCODE && addr != NULL) || force_next_DNS_reply == REPLY_REFUSED)
{
if((addr != NULL && addr->log.rcode == REFUSED)
|| force_next_DNS_reply == REPLY_REFUSED )
{
// REFUSED query
query->reply = REPLY_REFUSED;
new_reply = REPLY_REFUSED;
}
else if(addr != NULL && addr->log.rcode == SERVFAIL)
{
// SERVFAIL query
query->reply = REPLY_SERVFAIL;
new_reply = REPLY_SERVFAIL;
}
}
else if(flags & F_KEYTAG)
query->reply = REPLY_DNSSEC;
new_reply = REPLY_DNSSEC;
else if(force_next_DNS_reply == REPLY_NONE)
{
query->reply = REPLY_NONE;
new_reply = REPLY_NONE;
}
else if(flags & (F_IPV4 | F_IPV6))
{
// IP address
query->reply = REPLY_IP;
new_reply = REPLY_IP;
}
else
{
// Other binary, possibly proprietry, data
query->reply = REPLY_BLOB;
new_reply = REPLY_BLOB;
}

if(config.debug & DEBUG_QUERIES)
{
const char *path = short_path(file);
logg("Set reply to %s (%d) in %s:%d", reply_status_str[query->reply], query->reply,
logg("Set reply to %s (%d) in %s:%d", get_query_reply_str(query->reply), query->reply,
path, line);
if(query->reply != REPLY_UNKNOWN && query->reply != new_reply)
logg("Reply of query %i was %s now changing to %s", query->id,
get_query_reply_str(query->reply), get_query_reply_str(new_reply));
}

counters->reply[query->reply]++;
// Subtract from old reply counter
counters->reply[query->reply]--;
// Add to new reply counter
counters->reply[new_reply]++;
// Store reply type
query->reply = new_reply;

// Save response time
// Skipped internally if already computed
Expand Down
25 changes: 18 additions & 7 deletions test/test_suite.bats
View file
Original file line number Diff line number Diff line change
Expand Up @@ -412,13 +412,24 @@
#[[ ${lines[8]} == "clients_ever_seen 8" ]]
#[[ ${lines[9]} == "unique_clients 8" ]]
[[ ${lines[10]} == "dns_queries_all_types 47" ]]
[[ ${lines[11]} == "reply_NODATA 0" ]]
[[ ${lines[12]} == "reply_NXDOMAIN 1" ]]
[[ ${lines[13]} == "reply_CNAME 5" ]]
[[ ${lines[14]} == "reply_IP 23" ]]
[[ ${lines[15]} == "privacy_level 0" ]]
[[ ${lines[16]} == "status enabled" ]]
[[ ${lines[17]} == "" ]]
[[ ${lines[11]} == "reply_UNKNOWN 0" ]]
[[ ${lines[12]} == "reply_NODATA 0" ]]
[[ ${lines[13]} == "reply_NXDOMAIN 1" ]]
[[ ${lines[14]} == "reply_CNAME 3" ]]
[[ ${lines[15]} == "reply_IP 23" ]]
[[ ${lines[16]} == "reply_DOMAIN 0" ]]
[[ ${lines[17]} == "reply_RRNAME 5" ]]
[[ ${lines[18]} == "reply_SERVFAIL 0" ]]
[[ ${lines[19]} == "reply_REFUSED 0" ]]
[[ ${lines[20]} == "reply_NOTIMP 0" ]]
[[ ${lines[21]} == "reply_OTHER 0" ]]
[[ ${lines[22]} == "reply_DNSSEC 5" ]]
[[ ${lines[23]} == "reply_NONE 0" ]]
[[ ${lines[24]} == "reply_BLOB 10" ]]
[[ ${lines[25]} == "dns_queries_all_replies 47" ]]
[[ ${lines[26]} == "privacy_level 0" ]]
[[ ${lines[27]} == "status enabled" ]]
[[ ${lines[28]} == "" ]]
}

# Here and below: It is not meaningful to assume a particular order
Expand Down

0 comments on commit 4f6f6ca

Please sign in to comment.