Opened 7 years ago

Closed 6 years ago

#2518 closed task (fixed)

introduce exception hierarchy for "from text" errors in libdns++

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: loadzone-ng
Estimated Difficulty: 4 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

It's better to have some hierarchy in exception classes in libdns++
so that we can have a base exception class that indicates some
syntax (and maybe some of semantics) errors in a unified manner.

My specific suggestion is to define DNSTextError in
lib/dns/exceptions.h, and update existing exception classes thrown
from "from text" type of methods so that they are derived from
DNSTextError. Examples are TooLongLabel, BadLabelType,
InvalidRRType.

If it's not big, also update the Python wrapper to be consistent with
that.

Subtickets

Change History (13)

comment:1 Changed 6 years ago by muks

  • Milestone set to Sprint-20131015
  • Status changed from new to reviewing

I had introduced this common base class already as part of #1627. But now I've moved it from name.h to exceptions.h so it is in a more central place.

Up for review.

comment:2 Changed 6 years ago by muks

No ChangeLog is necessary (as it is a minor reorganization of code).

comment:3 Changed 6 years ago by pselkirk

  • Milestone changed from bind10-1.2-release-freeze to Sprint-20131015
  • Owner changed from UnAssigned to muks

This seems sane (although 'make check' fails with an unrelated error).

Jinmei mentions updating the Python wrapper, but I see no evidence of that in either this branch or #1627. If no change is needed, you should mention that.

comment:4 Changed 6 years ago by muks

  • Milestone changed from Sprint-20131015 to bind10-1.2-release-freeze

comment:5 follow-up: Changed 6 years ago by muks

Note: Also update MasterLoader removing the generic catch.

comment:6 in reply to: ↑ 5 ; follow-up: Changed 6 years ago by pselkirk

Replying to muks:

Note: Also update MasterLoader removing the generic catch.

This comment seems unrelated to this ticket.

comment:7 follow-up: Changed 6 years ago by pselkirk

I have reviewed this code, and convinced make check to run, and everything looks good. The remaining question is whether we need to do anything in the Python wrapper.

comment:8 in reply to: ↑ 6 ; follow-up: Changed 6 years ago by muks

Replying to pselkirk:

Replying to muks:

Note: Also update MasterLoader removing the generic catch.

This comment seems unrelated to this ticket.

I am sorry I was not very clear in that comment as I had written it down as a note for myself.

It is for this ticket. There is a catch-all for isc::Exception in loadIncremental() which was put in place because the various Name parsing exceptions were not in any hierarchy. As they have been updated, the catch has to be replaced with catches for the specific exceptions that we expect to handle here.

comment:9 in reply to: ↑ 7 Changed 6 years ago by muks

Replying to pselkirk:

I have reviewed this code, and convinced make check to run, and everything looks good. The remaining question is whether we need to do anything in the Python wrapper.

The Python wrapper has been updated now. Some more reorganization was done and C++ unittests were also added.

The pydnspp module still requires considerable general cleanup (including use of exceptions), but it's not suitable for this ticket. I think there's a different ticket open for it.

comment:10 in reply to: ↑ 8 Changed 6 years ago by muks

Replying to muks:

It is for this ticket. There is a catch-all for isc::Exception in loadIncremental() which was put in place because the various Name parsing exceptions were not in any hierarchy. As they have been updated, the catch has to be replaced with catches for the specific exceptions that we expect to handle here.

This is also fixed in the branch.

comment:11 Changed 6 years ago by muks

  • Owner changed from muks to pselkirk

Please re-review.

comment:12 Changed 6 years ago by stephen

  • Owner changed from pselkirk to muks

Reviewed commit 5e1c35d6a779706ba16f3076a0f6fa21dbeea81d (in particular, the difference between it and commit ab8ff1eda4c0daaeaf06614651ecea31a38339ab, made on 30 January 2014.

All looks OK, please merge.

comment:13 Changed 6 years ago by muks

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

Merged to master branch in commit 920f29296cf82087c35cc6a57fe05bdd3eef99f7:

* 5e1c35d [2518] Add C++ unittests for DNS exception hierarchy
* ad3f1d3 [2518] Update common DNS exceptions into a hierarchy
* 293665a [2518] Fix description
* 4ca62ac [2518] Organize isc::Exception-s in a hierarchy
* de4c5e0 [2518] Add Python bindings for NameParserException hierarchy
* ab8ff1e [2518] Introduce DNSTextError and remove generic catch for isc::Exception from the MasterLoader
* 86faa35 [2518] Fix some more cases of unqualified use of Exception
* fc5c737 [2518] Untabify code
* 362a479 [2518] Introduce isc::dns::Exception and organize all DNS exceptions under it
* 18c3e89 [2518] Move NameParserException to exceptions.h header

Resolving as fixed. Thank you for the review Stephen.

Note: See TracTickets for help on using tickets.