Opened 9 years ago

Closed 9 years ago

#487 closed enhancement (complete)

Handle query response from authoritative server

Reported by: shane Owned by: stephen
Priority: medium Milestone: R-Team-Sprint-20110125
Component: resolver Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 3.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Handle query response from authoritative server

  • Detect error
  • Detect answer
  • Detect referral
  • Detect CNAME

This will probably be in the RunningQuery class, but might just be a static function. Probably get a Message or *Message, and the question of the original query (a !DNSQuestion object).

Subtickets

Change History (7)

comment:1 Changed 9 years ago by stephen

  • Owner set to stephen
  • Status changed from new to assigned

comment:2 Changed 9 years ago by stephen

  • Owner changed from stephen to UnAssigned
  • Status changed from assigned to reviewing

First pass of the code in origin/trac487 is now ready for review (commit 30838487e181829182543e1a32e25ad313172b89).

comment:3 Changed 9 years ago by jelte

  • Owner changed from UnAssigned to jelte

comment:4 Changed 9 years ago by jelte

  • Owner changed from jelte to stephen

Looks good, only a few minor comments;

response_classifier.cc:234 according to the coding style guidelines (in this case the one from bind9) the ternary operator for a return value is a nono

I am wondering if we should differentiate between a 'normal' ANSWER result and a CNAME-but-answer one (behaviour of the caller on both cases should be the same, but still)

question.h contains a bit of garbage on line 191

question_unittests: I know this isn't done in the existing tests here, but shouldn't we use EXPECT_EQ and EXPECT_NE instead of EXPECT_TRUE(a == b) etc?

comment:5 Changed 9 years ago by stephen

  • Owner changed from stephen to jelte

Updated to revision 344dcf64d70d10d47d405b4e88e3cefca777aa6

response_classifier.cc:234 according to the coding style guidelines (in this case the one from bind9) the ternary operator
for a return value is a nono

Changed (and also changed the other occurrence in the file).

Do we want to revisit this for BIND-10? Although I can see cases where it can be abused, IMHO

return (<bool-or-simple-expression> ? <value1> : <value2>);

is clearer and easier to read than

if (<bool-or-simple-expression>) {
    return (<value1>);
} else {
    return (<value2>);
}

I am wondering if we should differentiate between a 'normal' ANSWER result and a CNAME-but-answer one (behaviour
of the caller on both cases should be the same, but still)

Done. Added the return category ANSWERCNAME, which is returned when a CNAME chain has been followed to get the answer.

Since the number of error returns is relatively large, I've also added "ResponseClassifier::error()" to quickly determine whether a return code is a success or error.

question.h contains a bit of garbage on line 191

Removed.

question_unittests: I know this isn't done in the existing tests here, but shouldn't we use EXPECT_EQ and EXPECT_NE instead of EXPECT_TRUE(a == b) etc?

I've left them as is, because these are multiple tests that operator==() and operator!=() work. (So the tests check (for example) that both A==B is true and A!=B is false; with EXPECT_EQ/NE we can only check that either A==B or A!=B for any condition.) I would tend to use EXPECT_EQ/NE for the case where we are checking some other function and implicitly assume that both "operator==()" and "operator!=()" work.

Notes
I've also added one additional check; the function now detects mixed QNAMES earlier if the QTYPE is ANY.

comment:6 Changed 9 years ago by jelte

  • Owner changed from jelte to stephen

Awesome. Ship it :)

(perhaps ooone more tiny thing, perhaps add a non-doxygen comment in the Category enum that the error() check needs the error states to come after the good states), no need to recheck with me if you do so or decide not to)

about that ternary operator btw, yeah, looking at the specific examples given in the bind9 guidelines i would say that the examples given there for what is allowed seem worse than the one for what is not... I don't really have a strong opinion though :)

comment:7 Changed 9 years ago by stephen

  • Resolution set to complete
  • Status changed from reviewing to closed

Added the comment. Committed as 18491370576e7438c7893f8551bbb8647001be9c.

Note: See TracTickets for help on using tickets.