Opened 8 years ago

Closed 8 years ago

#1258 closed task (complete)

"preserve order" option for Message::fromWire()

Reported by: jinmei Owned by: jinmei
Priority: very high Milestone: Sprint-20111011
Component: libdns++ Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

This is a subtask of #1212.

See the DNS_MESSAGEPARSE_PRESERVEORDER option of BIND 9's
dns_message_parse(). To quote the important part: "If
DNS_MESSAGEPARSE_PRESERVEORDER is set, a separate dns_name_t object
will be created for each RR in the message."

Subtickets

Change History (10)

comment:1 Changed 8 years ago by jinmei

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

comment:2 Changed 8 years ago by jinmei

Branch trac1258 is ready for review.

I believe it's straightforward and non controversial. Since the main
part is simple, I've made some improvements to fromwire that are not
directly related to the task subject: adding some more basic tests
(the message class still lacks many tests and we should gradually improve the
situation anyway), making the python wrapper code safer and (IMO) cleaner
by catching exceptions/error cases more and avoiding unnecessary
reinterpret_cast.

Also, as a result of adding new tests I've effectively addressed #927, too.
So, I plan to close that ticket once this one is merged.

This feature itself is a separate functionality, so I propose adding
a changelog entry for this ticket:

288.?	[func]		jinmei
	libdns++/pydnspp: added an option parameter to the "from wire"
	methods of the Message class.  One option is defined,
	PRESERVE_ORDER, which specifies the parser to handle each RR
	separately, preserving the order, and construct RRsets in the
	message sections so that each RRset contains only one RR.
	(Trac #1258, git TBD)

comment:3 Changed 8 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from accepted to reviewing

comment:4 Changed 8 years ago by jelte

  • Owner changed from UnAssigned to jelte

comment:5 follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to jinmei

I've pushed a trivial comment change in message.h (and my editor removed a few
spaces in that file)

Code looks good; just one small comment:

message_python.cc:688: Shouldn't we just let the normal python error be
propagated here? It should also specify which types are wanted, albeit only
from the second call. Otherwise, I'd propose to at least add which types we
expect to the error message (just 'invalid arguments' is true but not extremely
helpful).

comment:6 in reply to: ↑ 5 Changed 8 years ago by jinmei

Replying to jelte:

Thanks for the prompt review.

I've pushed a trivial comment change in message.h (and my editor removed a few
spaces in that file)

Looks good, thanks.

Code looks good; just one small comment:

message_python.cc:688: Shouldn't we just let the normal python error be
propagated here? It should also specify which types are wanted, albeit only
from the second call. Otherwise, I'd propose to at least add which types we
expect to the error message (just 'invalid arguments' is true but not extremely
helpful).

Hmm, I added explicit PyErr_SetString() just following the convention
adopted in many other such cases of our binding without considering
the implications much. So I checked the behavior without removing
PyErr_SetString(), and I found it possibly more confusing in some cases.
For example, from_wire(1, 1) would result in:

TypeError: function takes exactly 1 argument (2 given)

This is rather confusing as this method should actually accept 2 arguments.

On the other hand, I agree that just saying 'invalid arguments' isn't
helpful either.

So, I chose to keep our own PyErr_SetString() and tried to make the
error string more helpful. Please check.

comment:7 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:8 follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to jinmei

ohright when there are multiple options it can indeed be weird. This looks good, go ahead :)

comment:9 in reply to: ↑ 8 Changed 8 years ago by jinmei

Replying to jelte:

ohright when there are multiple options it can indeed be weird. This looks good, go ahead :)

Thanks, pushed, closing ticket.

comment:10 Changed 8 years ago by jinmei

  • Resolution set to complete
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.