Opened 10 years ago

Closed 10 years ago

#64 closed task (invalid)

review: data source code

Reported by: jinmei Owned by: shane
Priority: medium Milestone: 03. 1st Incremental Release
Component: Unclassified 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

I'm now looking into the data source and auth server implementation.

To save time, I plan to make relatively trivial changes/fixes directly rather than performing review-revise-confirm cycles. I'll keep the log of the changes here.

trunk rev 1110 is the first of such changes:

Log:
overall cleanup for the data source code, phase 1.

  • use forward declarations instead of including headers when possible
  • partly as a result of that, moved method definitions from .h to .cc unless it does very trivial thing and is deemed to be performance sensitive.
  • avoid 'using namespace' in header files
  • made data-source related objects non-copyable as much as possible
  • fixed a bug of an uninitialized variable
  • made coding style more consistent

Subtickets

Change History (5)

comment:1 Changed 10 years ago by jinmei

In Revision: 1113

fixed memory leak for QueryTask::zone

this led to a larger refactoring:

  • I figured out we actually don't have to have this information in QueryTask?: everything can be passed directly as a function argument.
  • I've also changed 'Name* zone' method argument to 'const Name*' based on the policy of 'make things const as much as possible'
  • also changed variable name from zone to zonename (because we might want to have a more generic notion of "zone" like BIND9's lib/dns/zone module)

Although the change set is large, this basically does trivial thing and I
believe it's non-controversial patch.

One possible open question is wheter we want to have the zone name in
QueryTask? for some other reasons. My suggestion is to add it back when we
see a real need for this. For year1 release the current code (with
this patch) should be good enough.

A specific note about the memory leak: in general, it's not advisable
to store bare memory pointer dynamically allocated by 'new', esepecially
in a big and complicated method like DataSrc::doQuery(), and especially
when we rely on exceptions (and we do). It's very easy to cause memory leak.
We should either do:

  • use an object encapsulating the allocated memory and release it in the destructor (like the NameMatch? class)
  • use smart pointers (boost::shared_ptr specifically)
  • redesign the code so that we don't have to allocate memory in the first place (just like I did in the patch)

comment:2 Changed 10 years ago by jinmei

Revision: 1118

Log:

  • made AuthSrv? construction exception-safe
  • fixed memory leak for Datasrc* stored in the MetaDataSrc? vector. there are several possible ways to do this, but I chose to using boost::shared_ptr. expect for portability issues this seems to be the cleanest solution, and, regarding portability, we already heavily rely on boost anyway, so we should revisit the whole design if/when we seriously consider binary portability.

Note: this is basically a fix to the auth server implementation, but affects
the data source API.

comment:3 follow-up: Changed 10 years ago by shane

  • Owner set to jinmei
  • Status changed from new to assigned

Jinmei, does it make sense for someone to go through these individual changes and look at them?
Has the code been stable after these?

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

  • Owner changed from jinmei to shane

Replying to shane:

Jinmei, does it make sense for someone to go through these individual changes and look at them?
Has the code been stable after these?

See my comment on ticket #50 (https://bind10.isc.org/ticket/50#comment:18).

I guess what makes most sense is to close this ticket for now and give a fresh review to the lib/auth stuff.

I believe the changes described in this ticket are quite stable, btw, thanks to the relatively more detailed tests written since then.

comment:5 Changed 10 years ago by shane

  • Resolution set to invalid
  • Status changed from assigned to closed

Okay, considering this ticket overtaken by events.

Thanks Jinmei.

Note: See TracTickets for help on using tickets.