Opened 9 years ago

Closed 9 years ago

#334 closed defect (fixed)

message rendering problem in xfrout

Reported by: jelte Owned by: jelte
Priority: medium Milestone:
Component: xfrout Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 0.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

While tracking the cause of another bug, I noticed that the behaviour of xfrout is a bit weird;

within the xfrout stream, messages are all < 512 octets, and have the TC bit set. Xfrout itself also does a 'test' render of the whole message for every RRset that is added, which is very inefficient. While the final renderer set the maximum size to 65535 octets, this check does not, hence the <512 octet messages. But if we change this test to check for <65535, the inefficiency gets really painful.

My proposal is to render each rrset once, check the length of that (which will not have full compression) plus the length of the rrsets added so far, and if that reaches a certain limit, then do a full render on the packet. Still room for some more optimizations, but at least the complexity is bound :)

Subtickets

Attachments (3)

bind10_xfrout_render.diff (5.5 KB) - added by jelte 9 years ago.
bind10_xfrout_render2.diff (7.5 KB) - added by jelte 9 years ago.
bind10_xfrout_render3.diff (8.0 KB) - added by jelte 9 years ago.

Download all attachments as: .zip

Change History (11)

Changed 9 years ago by jelte

comment:1 Changed 9 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from new to reviewing

Could create a subversion branch, but I had this diff before I created the ticket :)

comment:2 Changed 9 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:3 Changed 9 years ago by stephen

  • Owner changed from stephen to jelte

Changes are OK - go ahead an merge.

Suggest that another ticket be opened to extend the tests. test_send_message_with_last_soa is only tested with a message_upper_len value of 0; another test needs to be added with a value of this parameter large enough to check that attempting to add the final SOA to a full message will trigger the sending of two messages.

Suggestion for optimization: get_rrset_length obtains the wire format of the RRset in order to determine its length. The wire format is then discarded, but is presumably recreated when the RRset is actually added to the message. Could we save a second parse of the RRset by retaining the wire format of the message and adding a method to the message class to accept an RRset in that format?

Changed 9 years ago by jelte

comment:4 Changed 9 years ago by jelte

  • Owner changed from jelte to stephen

Adding that test was simple enough to do it right away (although it did require a small upgrade of the fake test socket class defined in the same test file), attached a second diff (full diff from trunk, i.e. include original diff, to see these changes, diff the diffs)

I agree that there is room for optimization, but I disagree on that specific approach, though I think we can leave that to another discussion.

comment:5 Changed 9 years ago by stephen

  • Owner changed from stephen to jelte

The new test does check that two messages get sent if the addition of the final SOA overflows the message buffer. However, since the message already in the buffer is the SOA, there is no way of telling whether the code has worked correctly, whether the message in the buffer has been sent twice, or whether the final SOA has been sent twice. I suggest using something other than SOA as the message in the buffer.

Changed 9 years ago by jelte

comment:6 Changed 9 years ago by jelte

  • Owner changed from jelte to stephen

Ok, see diff 3. I also added a few checks to see if there isn't any other data in those packets.

comment:7 Changed 9 years ago by stephen

  • Owner changed from stephen to jelte

All OK, please commit to trunk.

comment:8 Changed 9 years ago by jelte

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

Done, r2931.

Note: See TracTickets for help on using tickets.