Opened 9 years ago

Closed 6 years ago

#956 closed enhancement (fixed)

Message::toWire() should be TSIG agnostic

Reported by: jinmei Owned by: muks
Priority: medium Milestone: bind10-1.2-release-freeze
Component: libdns++ Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 3.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

I found the current interface of Message::toWire() not very convenient
with or without a TSIG context. The caller often needs to write if-else
depending on a TSIG is to be used like this:

    if (tsig_context.get() != NULL) {
        message->toWire(renderer, *tsig_context);
    } else {
        message->toWire(renderer);
    }

I now think we should convine these two versions of toWire, changing
the type of tsig_ctx to a pointer with a default value of NULL:

    void toWire(AbstractMessageRenderer& renderer,
                TSIGContext* tsig_ctx = NULL);
    // (and remove the single parameter version)

I chose the current API because I generally prefer avoiding NULL to be
accidentally passed, but in this case the convenience seems to
outweigh the possible benefit of safety.

Likewise, we should allow the python version Message.toWire() to
accept None for the tsig_ctx parameter.

Subtickets

Change History (16)

comment:1 Changed 9 years ago by shane

  • Milestone changed from New Tasks to Year 3 Task Backlog

comment:2 Changed 9 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from new to assigned

comment:3 Changed 9 years ago by jinmei

  • Milestone changed from Year 3 Task Backlog to Next-Sprint-Proposed

comment:4 Changed 9 years ago by stephen

  • Estimated Difficulty changed from 0.0 to 3

comment:5 Changed 8 years ago by jinmei

  • Milestone set to Next-Sprint-Proposed

comment:6 Changed 8 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to New Tasks

comment:7 Changed 8 years ago by jinmei

  • Milestone changed from New Tasks to Next-Sprint-Proposed

now we are updating xfrout, it's probably a good timing to address this.

comment:8 Changed 8 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Year 3 Task Backlog

comment:9 Changed 8 years ago by jinmei

ddns will benefit from this. So I propose solving this at this
opportunity.

comment:10 Changed 8 years ago by jinmei

  • Milestone changed from Year 3 Task Backlog to Next-Sprint-Proposed

comment:11 Changed 8 years ago by jinmei

  • Milestone changed from Year 3 Task Backlog to Next-Sprint-Proposed

comment:12 Changed 6 years ago by muks

  • Milestone set to bind10-1.2-release-freeze
  • Status changed from assigned to reviewing

Up for review.

comment:13 Changed 6 years ago by muks

No ChangeLog is required as it is not user visible.

comment:14 Changed 6 years ago by kean

  • Owner changed from UnAssigned to kean

comment:15 Changed 6 years ago by kean

  • Owner changed from kean to muks

The code looks just fine and all unit tests pass. Please go ahead and merge and close.

comment:16 Changed 6 years ago by muks

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

Merged to master branch in commit c1b9c0314457a680d7b85a81d794ba5625bca791:

* 86d5acd [956] Make Message::toWire() TSIG agnostic

Resolving as fixed. Thank you for the review.

Note: See TracTickets for help on using tickets.