Opened 10 years ago

Closed 10 years ago

#84 closed task (fixed)

review: auth server

Reported by: jinmei Owned by: jreed
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've completed cleanup for src/bin/auth. This is actually much more than trivial cleanups, including:

  • bougs input packet handling (mostly new features)
  • adding detailed unit tests for the AuthSrv? class

So I think the code should be reviewed by someone else.

Subtickets

Change History (9)

comment:1 Changed 10 years ago by each

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

I have reviewed all your changes to libauth as you've made them. There are one or two that I have reservations about and would like to revisit after Y1, but I think are acceptable for the present; the vast majority of the changes I approve without reservation.

I'm reading the code for libauth now to get a high-level view of what we've done....

One general comment is that it's woefully under-documented--that's mostly my fault. If I have time this week I'll try at least to write some brief descriptions of the functions in data_source.cc

I wish the unit test framework code in unit_test_util.cc were not duplicated with lib/dns/tests. I had hoped we'd create a general library for shared code, libisc or similar. But it isn't important for Y1.

A very minor comment on sqlite3_datasrc.cc: I notice that importSqlite3Rows() is in the anonymous namespace while several other local-only functions (findRecords(), hasExactZone(), etc) are private class members instead. I have the impression this is a difference in style between you and Michael, and there may not be any particular difference between the two, but I wonder if we should make a consistent choice within each file?

There are two or three remaining bugs in the query logic (and unit tests to check for them, which are commented out), and I'm fixing those today. Once you've approved those final changes, I think we can call this reviewed and merge it into the Y1 release.

comment:2 follow-ups: Changed 10 years ago by jinmei

Evan, thanks for the review, but please let me be sure about one fundamental point. Did you talk about src/lib/auth or src/bin/auth? I intended the latter in this ticket, but you seemed to focus on the former...

comment:3 in reply to: ↑ 2 Changed 10 years ago by each

Evan, thanks for the review, but please let me be sure about one fundamental point. Did you talk about src/lib/auth or src/bin/auth? I intended the latter in this ticket, but you seemed to focus on the former...

Whoops, my apologies, you're right. I was looking at lib/auth not bin/auth.

I'll review bin/auth shortly.

comment:4 in reply to: ↑ 2 ; follow-up: Changed 10 years ago by each

  • Owner changed from jinmei to jreed

Did you talk about src/lib/auth or src/bin/auth?

I have now read src/bin/auth, and approve it for Y1.

A couple of notes for Y2:

  • Needs more comments
  • DNSPORT, it seems to me, should be an aspect of AuthSrv? rather than a static variable in main.cc
  • I don't object, but I don't really understand the benefit of pimpl in AuthSrv?

Assigning to Jeremy for merge

comment:5 Changed 10 years ago by jreed

  • Owner changed from jreed to each

I copied to the review branch (branches/Y1 revision 1511). I am not closing this ticket yet until the issues brought up are fixed or have other tickets tracking them. Re-assigning to owner.

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

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

I have now read src/bin/auth, and approve it for Y1.

Okay, thanks. I'm going to close this ticket, but answer the comments for the record.

A couple of notes for Y2:

  • Needs more comments

Absolutely:-) And, actually, I think we should revisit the whole design and implementation of the main auth server code in Y2. The DNSPORT point would also be covered in that effort.

  • DNSPORT, it seems to me, should be an aspect of AuthSrv? rather than a static variable in main.cc
  • I don't object, but I don't really understand the benefit of pimpl in AuthSrv?

IMO the question should be reversed: In general we should ask why if a class doesn't use pimpl, and suggest using it unless there's specific reason for not doing so.

The strongest reason for that is because with pimpl we can hide depedency details from public header files. A class definition that doesn't use pimpl tends to contain object member variables defined outside the module or package. In the case of AuthSrv? class, if it were not using pimpl, it would contain

std::string db_file_;
MetaDataSrc? data_sources_;
ConstDataSrcPtr? cur_datasrc_;

(in addition to other, less harmful members)

Since these are real objects (i.e. not a pointer or reference) we need to include the corresponding header files for the definitions of these classes.

This will increase compile time because every time we modify data_source.h (for MetaDataSrc?) we'll also need to compile auth_srv.cc again even if the public interface isn't changed. Also, since MetaDataSrc? is a pretty big class that also possibly depends on other classes, the indirect dependency may increase the bulid overhead further.

Standard library objects such as std::string should be more stable in this sense, but they cause other kinds of problem: portability. Without pimpl, a binary image of an AuthSrv? object contains a binary image of std::string object at the build time (of BIND10), which may not be compatible with the binary image of std::string when it's linked with a third-party program.

ConstDataSrcPtr? is actually a typedef of boost::shared_ptr, so the probability problem could be even worse than std::string as it's a moving target.

Of course, there are cons with pimpl. One big issue with it is that construction, copy and assignment will be more complicated and less efficient because these will require dynamic memory allocation in addition to initialization of the member variables. So, for a class that is frequently copied *and* used in performance sensitive path, we should be careful about using the pimpl approach. This is one reason why I didn't adopt pimpl for the dns::Name class.

The AuthSrv? class doesn't have this issue. Actually, it's essentially a singleton object, much less being copied or assigned to other variables, or constructed/destructed multiple times.

Considering these points and other, relatively minor complexity issues due to pimpl, I believe this is a case where "we should use pimpl unless there's a reason for not doing so".

comment:7 Changed 10 years ago by jreed

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:8 Changed 10 years ago by jreed

  • Owner changed from each to jreed
  • Status changed from reopened to assigned

assigned to me so I don't forget

comment:9 Changed 10 years ago by jreed

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

Closing. I already copied to reviewed branch.

Note: See TracTickets for help on using tickets.