Opened 10 years ago

Closed 9 years ago

#185 closed defect (fixed)

review: more tests and bug fixes for xfrin

Reported by: jinmei Owned by: zhanglikun
Priority: medium Milestone: 04. 2nd Incremental Release: Early Adopters
Component: xfrin Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: Add Hours to Ticket:
Total Hours: Internal?:

Description

This is for backlog 92 of http://bind10.isc.org/wiki/f2F1_Y2_Wed and a continuation of ticket #179.

I'll create a separate branch, merge the patch of the first phase into it, and add more tests and fix bugs identified so far (+ any newly found bugs).

Subtickets

Change History (7)

comment:1 Changed 9 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from new to reviewing
  • Summary changed from more tests and bug fixes for xfrin to review: more tests and bug fixes for xfrin
  • Type changed from task to defect

branches/trac185 (more xfrin tests) is ready for review.

The entire diff is
svn diff -r 1844 svn+ssh://bind10.isc.org/svn/bind10/branches/trac185
(branch point is r1843, but it wouldn't be handy because it includes the first phase of this work, which has already been merged to trunk)

This branch contains a set of independent (relatively small) fixes. To review, it may be easier to check these one by one rather than looking at the entire diff of the branch. These are summarized in the xfrin/TODO file with "[FIXED in rNNNN].

The specific change sets are:
http://bind10.isc.org/changeset/1851
http://bind10.isc.org/changeset/1861
http://bind10.isc.org/changeset/1889
http://bind10.isc.org/changeset/1880
http://bind10.isc.org/changeset/1882
http://bind10.isc.org/changeset/1908
http://bind10.isc.org/changeset/1866
http://bind10.isc.org/changeset/1887

There are still some more open bugs. They seem to be relatively more complicated, so I'll give a separate ticket and branch for each of them.

comment:2 Changed 9 years ago by shane

  • Owner changed from UnAssigned to zhanglikun

comment:3 follow-up: Changed 9 years ago by zhanglikun

I have did some review on your change, yeah, looks quite well. I did some minor change in r1948, and fixed failed unit tests in r1961. The problem is there are some log message after running unittest, but anyway it can be avoided once we use logsys in future.

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

Replying to zhanglikun:

I have did some review on your change, yeah, looks quite well. I did some minor change in r1948, and fixed failed unit tests in r1961. The problem is there are some log message after running unittest, but anyway it can be avoided once we use logsys in future.

Likun, thanks for the review.

r1948 looks correct, good catch.

We cannot adopt r1961, however:

  • "3.3.3" will be rejected in BSD's getaddrinfo() implementation. This is because their implementation doesn't fully conform to the RFC (by applying stricter checks), but aside from that, what we should do in this context is to provide a "perfectly invalid" address string and let an exception be raised. So I use "3.3.3.3.3" (five 3's) and revised the test so that an exception will be detected.
  • we shouldn't allow 65536. Clearly it's an invalid number for a UDP/TCP port. Apparently Linux (glic's) implementation is buggy and incorrectly accepts such out-of-range numeric port numbers. I've added an additional (and redundant for non Linux implementations) validation ot work around this bug.

The changeset is r1968. Please check.

Assuming these are okay, is this patch ready to be merged to trunk? Or is this your intermediate report?

comment:5 follow-up: Changed 9 years ago by zhanglikun

I think it's ok to merge the code to trunk, and there should be a new ticket for implementation of the left TODOs. I will do the implementation after coming release.

comment:6 Changed 9 years ago by zhanglikun

After merging to trunk, I will add one test case for the notfiy command(send from auth process.)

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

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

Replying to zhanglikun:

I think it's ok to merge the code to trunk, and there should be a new ticket for implementation of the left TODOs. I will do the implementation after coming release.

Okay, thanks. Committed to trunk, r2000. Closing.

Note: See TracTickets for help on using tickets.