BIND 10 #116: review: NSEC RDATA "from wire" fixes and tests

[BIND 10 Development]

#116: review: NSEC RDATA "from wire" fixes and tests
---------------------------+------------------------------------------------
 Reporter:  jinmei         |        Owner:  each                       
     Type:  defect         |       Status:  reviewing                  
 Priority:  critical       |    Milestone:  03. 1st Incremental Release
Component:  DNSPacket API  |   Resolution:                             
 Keywords:                 |    Sensitive:  0                          
---------------------------+------------------------------------------------
Changes (by jinmei):

  * owner:  jinmei => each
  * status:  assigned => reviewing


Comment:

 Replying to [comment:3 shane]:
 > I decided I would go ahead and have a look at this. I'm not sure if this
 code has already been merged or not, but I figured it couldn't hurt to
 review it. I'm certainly not the best person for this, but I'll have a
 look.

 Thanks for the check, and, actually I think you are the best possible
 reviewer as the original code was written by you (although you might think
 you're not just for the same reason:-).

 The code is already in trunk (it was done with confusion about how we use
 trunk/branches wrt review, etc), but hasn't been "reviewed".

 > Typo here:
 [snip]
 > If the length cannot be 0, then the assertion should be:

 Both are correct, thanks.  Fixed in the trunk.

 > Further, do we consider it good practice to use assert()?

 Good question...a non documented, probably not so consistent policy of
 mine is that I use assert() when an invariant isn't met or other internal
 error, while I generally use an exception for an unexpected/invalid input
 or run time system error (memory shortage, failure of opening a file,
 etc).  In this specific, case, since the bitmap is private, is populated
 only via the constructors, and the constructors validate the source of
 bitmaps, the bitmaps must be valid once an NSEC object is successfully
 constructed.  If this assumption fails it means there should be an
 internal bug in the class implementation or something very catastrophic
 should happen (such as memory corruption, the application overrides the
 memory region for the object with bogus data, etc).

 We could still raise an exception even for such a case.  For example, we
 could use a std::logic_error for internal bug.

 That's certainly one possible approach, but I didn't do that for the
 following reasons:

  - even if we use an exception, I believe we cannot safely recover from
 such a serious error anyway, and the end result is the same: exit.
  - if we exit anyway, IMO it's better to let the program crash (hopefully
 with dumping a core) as soon as the error is detected.  Rraising an
 exception may trigger a different exception, which will hide the real
 cause of the bug.  Or worse, sometimes we are lazy and write catch-all
 catch clause just as an easy way of exception handling.  Even though we
 should discourage such a style in any event, it should be prudent not to
 assume we always do the right thing, especially in a critical situation
 like this.

 Admittedly, however, I'm probably not so consistent on when to use assert,
 and the policy wouldn't be clear.

 If we can agree with how/when to use exceptions/assertions, we should
 probably document it as part of coding guideline.

 > Will this raise an exception or simply end the program?

 No, if the assertion fails the program will just abort itself.

 > Hopefully this is useful. :)

 It is.  Thanks.

 Do you have any other comment for this ticket, or can we close it?

-- 
Ticket URL: <https://bind10.isc.org/ticket/116#comment:4>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development