Opened 9 years ago

Closed 9 years ago

#221 closed enhancement (fixed)

review: Refactoring auth server and merge the axfr and notify logic into it

Reported by: hanfeng Owned by: each
Priority: medium Milestone: 06. 4th Incremental Release
Component: b10-auth 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?: no

Description

Current auth server can't support notify in, and the implementation of axfr logic is quite tricky which because of the interface of auth server isn't complete. The refactoring will add notify logic and related unit test code.

Subtickets

Attachments (1)

auth.diff (2.4 KB) - added by each 9 years ago.

Download all attachments as: .zip

Change History (30)

comment:1 Changed 9 years ago by shane

  • Component changed from Unclassified to b10-auth

comment:2 follow-up: Changed 9 years ago by hanfeng

  • Owner changed from hanfeng to jinmei
  • Status changed from new to reviewing

After the refine, the logic isn't changed too much, just another class which called UserInfo? is added (the name is under consideration) which stored all the necessary information for axfr and notify process. the unittest code is also modified but two test cases are failed, one of them is edns0 buffer size after one malformed edns0 query, another is about flag bit. I don't know what's the purpose for these two test cases, please have a look.

comment:3 Changed 9 years ago by hanfeng

BTW, the code is in path branches/feng-authnotify/src/bin/auth

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

  • Owner changed from jinmei to hanfeng

First off, could you rebase this branch so that it will use non boost ASIO?

Right now it seems to partially merge recent changes of trunk:

  • it has new asio_link.{h,cc}
  • but it doesn't have ext/asio

Since this part of change in trunk affects this patch substantially, it doesn't make sense to base it with an older version of trunk (and partial incorporation cannot be an option, of course).

On top of that, please:

  • don't include asio.hpp from asio_link.h. It breaks build, and that's exactly the reason why we introduced asio_link.
  • don't use "using namespace asio::ip" in a header file. this is generally a bad practice.

I'm now giving the ticket back to hanfeng.

Oh, and when you rebase it, I suggest you change the branch name using our common convention: trac### (in this case trac221).

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

Replying to jinmei:

First off, could you rebase this branch so that it will use non boost ASIO?

Right now it seems to partially merge recent changes of trunk:

  • it has new asio_link.{h,cc}
  • but it doesn't have ext/asio

Since this part of change in trunk affects this patch substantially, it doesn't make sense to base it with an older version of trunk (and partial incorporation cannot be an option, of course).

Is there any progress on this? Assuming not, I'm planning to create a new branch myself so that we can move forward faster.

comment:6 follow-up: Changed 9 years ago by hanfeng

  • Owner changed from hanfeng to jinmei

I have merge trunk into branch branches/trac221 please have a look.
And I also modified some part according to your suggestions.
please have a check.

comment:7 in reply to: ↑ 6 ; follow-up: Changed 9 years ago by jinmei

Replying to hanfeng:

I have merge trunk into branch branches/trac221 please have a look.
And I also modified some part according to your suggestions.
please have a check.

Perhaps you did something different what you inteded to do?

I've already created branches/trac221, and you seemed to copy feng-authnotify under the top directory of the branch. I don't see much difference in trac221/feng-authnotify from your original branch version.

Anyway, I'm writing a counter proposal ASIO wrapper layer in the trac221, some of which have already been committed to the repository. Once it's completed I'll merge other part of your change and let you know.

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

Replying to jinmei:

Anyway, I'm writing a counter proposal ASIO wrapper layer in the trac221, some of which have already been committed to the repository. Once it's completed I'll merge other part of your change and let you know.

Okay, I've completed my counter proposal patch.

It's committed to branches/trac221. The diff is:
svn diff -r 2169 svn+ssh://bind10.isc.org/svn/bind10/branches/trac221

This is a completely from-the-scratch patch, and should be reviewed by someone. Can you review it?

FWIW, an HTML version of the doxygen documentation is available at:
http://bind10.isc.org/~jinmei/bind10/cpp/namespaceasio__link.html

Next, here are some preliminary comments on the code for the AXFR (+ notify?) support. Essentially these are comments only on AuthSrvImpl::processAxfrQuery() and AuthSrvImpl::processIxfrQuery(), but I have some general comments:

  • general
    • please read the coding guideline carefully. your original patch contains many code fragments that break the guideline.
    • I didn't adopt the "nag()" method. I agree we could log things better than in the current form, but it's expensive to construct log message string before checking the verbose level.
    • I also didn't use boost::noncopyable. My understanding is that we agreed we don't want to add dependency on boost for this purpose. I didn't oppose to homemaid noncopyable class, but in any event that should go to a different task.
  • processAxfrQuery
    • add detailed tests, please.
    • (as a result of refactoring) it's now possible that we receive an AXFR query over UDP. It should be detected and rejected here. BIND 9 returns FORMERR (although I don't know it's implementation specific or is defined in the protocol spec)
    • we shouldn't use catch-all catch. for example, we don't want to catch std::bad_alloc here. Be as specific as poissible, and the broadest allowable exception class is isc::Exception.
    • when exception happens we should return an error, rather than ignore the query.
    • is it reasonable to establish a connection with the xfrout process every time we receive AXFR request? It seems to be an unnecessary waste. It would be better to open a connection at initialization and keep it for subsequent AXFR processing. (but this should probably go to a separate ticket).
    • we should probably do a bit more validation on the request. for example, some evil node may send a very long AXFR request, trying to mount a DoS by increasing the overhead between the auth server and the xfrout process.
    • a related note: alghouh unlikely, sendXfroutRequestInfo() can block, and if that happens will stop responding to other queries. I don't think this is an ugent problem, but we'll probably need to make it asynchronous eventually.
  • processIxfrQuery
    • first off, adding this stuff seems to go beyond the scope of "refactoring". maybe we should leave this to a separate subseqent ticket.
    • secondly, detailed tests, please. whether or not we do this work in this branch or in a separate ticket.
    • thirdly, is this for "IXFR"? The original branch name and some part of the code seem to indicate NOTIFY. On the other hand, the dispatch part of the code seems to indicate it's for IXFR. I guess it's for notify, because if it's for IXFR the remaining code doesn't make sense at all. In what follows I assume this is for notify. note that this kind of confusion should have been eliminated if you began with tests.
    • overall, this method ignores many error cases/possible exceptions (some specific points are noted as comments). it should be massively corrected.
    • is this a short term workaround, or is this the model you're proposing for notify support? if it's a short term hack, that's okay with me. But if this is intended to be a longer term solution, I must oppose to that because what it does is very expensive, involving many string manipulations.
    • assuming it's a notify, we need to respond to it from this method (currently it's ignored)

comment:9 Changed 9 years ago by jinmei

  • Owner changed from jinmei to hanfeng

comment:10 follow-up: Changed 9 years ago by hanfeng

  • Owner changed from hanfeng to jinmei

Refine the code according to jinmei's feedback. Add part test for notify process.

comment:11 in reply to: ↑ 10 Changed 9 years ago by jinmei

Additional comments on the latest version of processNotify() and processAxfrQuery().

  • general
    • Style issues still remain. I've directly fixed them and committed the fix to the branch.
    • please don't make notifyin_fromwire by naively copy other data files (btw the comment is wrong). they are expected to be generated from a .spec file using lib/dns/tests/testdata/gen-wiredata.py.
  • processMessage
    • update the comment: "we only support normal queries", which is not true any more.
  • processAxfrQuery
    • when axfr_client_.connect() fails I suspect the auth server will receive an ASIO exception (and kill the server as a result). It should be hidden within the XfroutClient module
    • why returning SERVFAIL for AXFR/UDP? does RFC say so?
    • parentheses after return: please make the style consistent.
    • I suspect it shouldn't return a response if the axfr request is successfully forwarded.
    • variable name "is_xfrin_connection_established_" is confusing because we use the term "xfrin" for the opposite direction.
  • processNotify
    • many of my previous comments still hold.
    • if the session cannot be established it should return an error message.
    • at least according to the doc Element::createFromString() could return NULL. If that's the case it should be handled accordingly. (see Trac #250)
    • shouldn't it handle the case where "rcode" indicates an error?
    • why do we need these two catch clauses? Although the error messages are slightly different, I don't see much benefit in this case to separate these two (note that we could show where the exception is triggered from isc::Exception). On the other hand, since we cannot be sure what kind of underlying modules are necessary in the try clause, just catching these two specific exception classes may be too restricted. If I were to implement it, I'd catch all (isc::)Exception objects (see also processNormalQuery).
    • when the error happens, return SERVFAIL, rather than ignoring it.

comment:12 Changed 9 years ago by jinmei

  • Owner changed from jinmei to hanfeng

comment:13 Changed 9 years ago by jinmei

Oh, and one more thing. Did you review my refactoring code? That's a quite large chnge (although relatively trivial, I hope) and I'd expect the reviewer finds something that needs to be fixed.

comment:14 follow-up: Changed 9 years ago by hanfeng

  • Owner changed from hanfeng to jinmei

processNotify

o at least according to the doc Element::createFromString() could return NULL. If that's the case it should be handled accordingly. (see Trac #250)
after checking the code I found it will raise exception when meet error

o shouldn't it handle the case where "rcode" indicates an error?
the parseAnswer function is quite strange, it raise exception, then I don't understand the rcode value is used for what:(
o when the error happens, return SERVFAIL, rather than ignoring it.
according to RFC 1996, the master only care three things, one is no reply received, and the master will send notify later, the second is with NOTIMP, it's same with master received NOERROR reply, master willn't send the notify to the slave again. So if error happens, we just ignore it, when wait for next try

For asio_link wrap,
1 if we wrap the asio lib, all the other part in BIND10 using asio should through the asio wrapper, maybe this should be put into another ticket.

2 For class ServerSet?, it's functionality can be replace by using auto_ptr which from STD, and also can make sure if exception occured, the memory will be freed during the construction call.

comment:15 in reply to: ↑ 14 Changed 9 years ago by jinmei

Replying to hanfeng:

First off, don't "fix" the points before tests, please. By giving early feedback, I expected you to add test cases to reproduce the issues I pointed out (or confirm they are not an issue), then fix them, and then confirm they are fixed using the tests.

processNotify

o at least according to the doc Element::createFromString() could return NULL. If that's the case it should be handled accordingly. (see Trac #250)
after checking the code I found it will raise exception when meet error

Yes, Jelte clarified that in Trac #250. But the more important point is that we should be aware of return values of any method we use or the possibility of exception being thrown *at the time we wrote the code*. Of course we sometimes overlook such possibilities and we don't have to be 100% correct at that point (and that's actually one of the reasons why we need code review), but when I saw the initial code it made me nervous because so many error cases seemed to be just ignored or very roughly handled (e.g. by using a catch-all catch, which is itself bad, and even it doesn't help if the error is reported by a return value).

So, while you can count on reviewers pointing out overlooked failure mode, please be a bit more careful about such cases *before asking for review*.

o shouldn't it handle the case where "rcode" indicates an error?
the parseAnswer function is quite strange, it raise exception, then I don't understand the rcode value is used for what:(

I'm not sure what you mean here, but at least according to the function description the rcode can be non 0, indicating an error, in which case parseAnswer() returns an error message. We should handle that case explicitly (or if we are confident it's impossible in this context, we should assert() that).

But again, don't fix it *now*. Add tests first, please:-)

o when the error happens, return SERVFAIL, rather than ignoring it.
according to RFC 1996, the master only care three things, one is no reply received, and the master will send notify later, the second is with NOTIMP, it's same with master received NOERROR reply, master willn't send the notify to the slave again. So if error happens, we just ignore it, when wait for next try

Ah, okay. Could you leave code comments on this? I think this is a non trivial behavior.

For asio_link wrap,
1 if we wrap the asio lib, all the other part in BIND10 using asio should through the asio wrapper, maybe this should be put into another ticket.

Yes, that's the intent. I could have tried that level of overhaul refactoring, but didn't do so because we've already done too many things in this branch.

2 For class ServerSet, it's functionality can be replace by using auto_ptr which from STD, and also can make sure if exception occured, the memory will be freed during the construction call.

Ah, yes, good suggestion. I've modified the code so that it uses boost::shared_ptr instead of the helper class (and confirmed that it didn't break anything trhough tests). We could use auto_ptr in this case as you said, but I simply followed our common practice with shared_ptr since auto_ptr has so many pitfalls to be used safely. We've already heavily relied on shared_ptr's and the usage in this case isn't leaked via a public header file, so I think the additional dependency (on boost) is acceptable.

Besides, we'll eventually be free from creating UDP/TCPServer classes within the asio_link module once we generalize it. That job will be shifted to the user module. So this part will likely be revised anyway.

Are other part of the patch okay, or will more comments come? (I normally expect this size of patch would have tens of comments including overlooked bugs:-)

comment:16 follow-up: Changed 9 years ago by jinmei

  • Owner changed from jinmei to hanfeng

comment:17 in reply to: ↑ 16 ; follow-up: Changed 9 years ago by jinmei

What's the current status of this ticket?

I'd like to see this feature in the next release (but I don't like to make an easy compromise on test coverage and code/documentation quality). If it stalls I'm happy to help write tests, etc.

comment:18 in reply to: ↑ 17 ; follow-up: Changed 9 years ago by hanfeng

Replying to jinmei:

What's the current status of this ticket?

I'd like to see this feature in the next release (but I don't like to make an easy compromise on test coverage and code/documentation quality). If it stalls I'm happy to help write tests, etc.

I am a little busy now, so if you can help write some tests, that will be great, thanks a lot

comment:19 in reply to: ↑ 18 Changed 9 years ago by jinmei

  • Owner changed from hanfeng to UnAssigned
  • Summary changed from Refactoring auth server and merge the axfr and notify logic into it to review: Refactoring auth server and merge the axfr and notify logic into it

Replying to hanfeng:

What's the current status of this ticket?

I'd like to see this feature in the next release (but I don't like to make an easy compromise on test coverage and code/documentation quality). If it stalls I'm happy to help write tests, etc.

I am a little busy now, so if you can help write some tests, that will be great, thanks a lot

I've added all possible tests I can think of, and fixed all bugs that were identified by the tests as well as the issues I've pointed out.

There are 16 new (including one substantially revised) tests: 5 for AXFR and 11 for notify. Please expect I'd request at least this level and amount of tests next time you ask for reivew:-)

Now I'm rather a (co)author of this branch than a reviewer, with a large set of changes I made myself, and I'm afraid we need a different reviewer. I'll get this ticket unassigned.

It would be great if we could get it reviewed and merged to trunk before the next week, but since it's a quite big change maybe it's too optimistic. In any case, I think we should learn a less from this ticket: this is an example case how we tend to underestimate the workload of adding tests in planning.

comment:20 follow-up: Changed 9 years ago by jinmei

  • Owner changed from UnAssigned to jinmei
  • Status changed from reviewing to accepted

It turns out that this branch requires other larger refactoring (see, e.g. r2393). Now that we've missed the July release, I'd propose to separate that part from this work, get it reviewed and merged first, and then rebase this work, review and merge it.

So, I'm going to pull this ticket back from the review queue, assigning to myself.

comment:21 Changed 9 years ago by jinmei

  • Milestone changed from 05. 3rd Incremental Release: Serious Secondary to 06. 4th Incremental Release

comment:22 in reply to: ↑ 20 Changed 9 years ago by jinmei

Replying to jinmei:

It turns out that this branch requires other larger refactoring (see, e.g. r2393). Now that we've missed the July release, I'd propose to separate that part from this work, get it reviewed and merged first, and then rebase this work, review and merge it.

A separate branch is created, currently under review (Trac #275).

comment:23 Changed 9 years ago by jinmei

  • billable set to 1
  • Internal? unset

branches/trac221b is ready for review.

The diff from the branch point can be retrieved by

svn diff -r2462 svn+ssh://bind10.isc.org/svn/bind10/branches/trac221b

This branch actually consists of two (mostly) independent sets of changes:

  • refactoring ASIO link to separate xfr/notify logic from the ASIO interface. detailed documentation and tests are also provided.
  • support for incoming AXFR and NOTIFY messages, including detailed tests

The following files are modified for the first set of changes:

  • src/bin/auth/main.cc
  • src/bin/auth/asio_link.cc
  • src/bin/auth/asio_link.h
  • src/bin/auth/tests/asio_link_unittest.cc
  • src/lib/cc/session.h (just for minor editorial changes + comments)

and these are for the second set of changes:

  • src/lib/xfr/xfrout_client.cc
  • src/lib/xfr/xfrout_client.h
  • src/bin/auth/auth_srv.cc
  • src/bin/auth/auth_srv.h
  • src/bin/xfrin/tests/xfrin_test.py
  • src/bin/xfrin/xfrin.py.in

The size of entire diff is pretty big, so we might want to assign two
reviewers to separate the review task. Or, if it's desirable we could
make another supplemental branch for the first set of changes before
getting the second reviewed. I'd leave it to the reviewer and/or
Shane.

For convenience, an HTML version of the ASIO link documentation is
available at:
http://bind10.isc.org/~jinmei/bind10/cpp/namespaceasio__link.html

I should also note that I disabled the code in xfrin.py that starts
xfrin triggered by a notify for security reasons (see the comment). I
guess we have to wait for the zone manager implementation that can
handle incoming notifies in a securer way. Nevertheless the code is
provided as a proof of concept.

Suggested ChangeLog entry is as follows:

  75.?	[func]		feng, jinmei
	Refactored the ASIO link interfaces to move incoming XFR and
	NOTIFY processing to the auth server class.  Wrapper classes for
	ASIO specific concepts were also provided, so that other BIND 10
	modules can (eventually) use the interface without including the
	ASIO header file directly.  On top of these changes, AXFR and
	NOTIFY processing was massively improved in terms of message
	validation and protocol conformance.  Detailed tests were provided
	to confirm the behavior.
	Note: Right now, NOTIFY doesn't actually trigger subsequent zone
	transfer due to security reasons. (Trac #221, rTBD)

comment:24 Changed 9 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from accepted to reviewing

Changed 9 years ago by each

comment:25 follow-up: Changed 9 years ago by each

  • Owner changed from UnAssigned to jinmei

I don't see anything particularly wrong here, but I do have a few nits and some questions about design decisions, see below.

It's generally okay, and I expect any changes you make in response to the comments below to be fairly insignificant. I'll leave it to your judgment whether to go ahead and commit after you've made such changes, or to seek further review.

Index: src/lib/xfr/xfrout_client.h

Minor nit 1: In some other abstract base classes (e.g. AbstractDataSrc, AbstractRRset) you've put in a boilerplate comment explaining why the constructor is declared "protected". I noticed that comment is missing here, and thought I'd mention it in case you consider it important enough to add.

Minor nit 2: Seeing that the methods in XfroutClient had been changed from non-virtual to virtual confused me for a bit; I thought there must be another derived class somewhere that extended XfroutClient them, and was wondering if I was missing part of the code. I'd appreciate a comment explaining why they're virtual.

Index: src/lib/xfr/xfrout_client.cc

All okay.

Index: src/lib/cc/session.h

Same note about missing comment on the "protected" constructor declaration.

Index: src/bin/auth/auth_srv.h

Seems fine.

Index: src/bin/auth/auth_srv.cc

"if (is_axfr_connection_established_)" is incorrect English ;)

More seriously, that flag only has to do with xfrout, not xfrin, so maybe it should have a more specific name? "xfrout_connected" or something.

I'm not clear on why xfrout_client needs to be declared by the caller and passed into the AuthSrv constructor, as opposed to having it instantiated by the AuthSrvImpl constructor and leaving the AuthSrv constructor's signature unchanged. I don't see any place where xfrout_client is accessed from outside AuthSrv.

Index: src/bin/auth/asio_link.h

+namespace asio {
+class io_service;
+namespace ip {
+class address;
+}
+}

Does the above code serve a useful purpose? I commented it out and found it still compiled fine, though maybe other compilers would have problems. If it's needed, please add a comment explaining the reason.

There were some minor grammatical errors in the comment explaining the asio_link class (missing paragraph breaks, singular/plural disagreement)--I can just fix them in the branch.

The comments about IOEndpoint say that it's a wrapper for both ip::address and for the tcp/udp endpoint classes. I think the first one is a mistake and will remove it in the branch.

Comment on IOEndpoint::getAddress() says the return value is "not a real object" but appears to mean that it is one. I'm fixing the comment.

The static IOSocket::getDummy* methods seem rather unusual; why can't there just be a derived class DummySocket? I'm not seeing why this needs to be in the base class API.

+ / This method blocks until the \c stop() method is called via some
+
/ handler.

It does some other stuff too, right? :)

Index: src/bin/auth/asio_link.cc

This code seems fine...

+ asio_endpoint_placeholder_(
+ new tcp::endpoint(ip::address::from_string(address.toText()),
+ port)),
+ asio_endpoint_(*asio_endpoint_placeholder_)

...but I don't understand the point. Why not have asio_endpoint_ be a real object instead of a reference, and construct it in the normal way, without using "new" and "delete"? (See attached diff.)

Index: src/bin/auth/main.cc

All seems fine except for the point mentioned before--that I don't know why xfrout_client is declared here.

Index: src/bin/xfrin/xfrin.py.in

+ # The default RR class is IN. We should fix this so that
+ # the class is passed in the command arg (where we specify
+ # the default)

This comment appears to be outdated; it seems as if the rrclass is passed in the command arguments now.

Index: src/bin/auth/tests/asio_link_unittest.cc

Two of the tests currently fail:

[ FAILED ] IOServiceTest.badPort
[ FAILED ] IOServiceTest.unavailableAddress

So fix or disable them.

Index: src/bin/auth/tests/auth_srv_unittest.cc
Index: src/bin/xfrin/tests/xfrin_test.py

These look fine.

comment:26 in reply to: ↑ 25 Changed 9 years ago by jinmei

Replying to each:

I don't see anything particularly wrong here, but I do have a few nits and some questions about design decisions, see below.

It's generally okay, and I expect any changes you make in response to the comments below to be fairly insignificant. I'll leave it to your judgment whether to go ahead and commit after you've made such changes, or to seek further review.

Thanks for the detailed review. I'm going to incorporate trivial changes and merge the branch to trunk anyway because other important features such as secondary manager depends on it. We can create a separate follow up tickets for any remaining open issues.

I'll give this ticket back to you, keeping it open just in case. If you think this particular ticket is done, please simply close it; if you want to continue discussions on some of the points below within this ticket, please do so; if you want to discuss the points in a seprate ticket, please close this one and open new one(s).

Index: src/lib/xfr/xfrout_client.h

Minor nit 1: In some other abstract base classes (e.g. AbstractDataSrc, AbstractRRset) you've put in a boilerplate comment explaining why the constructor is declared "protected". I noticed that comment is missing here, and thought I'd mention it in case you consider it important enough to add.

Addressed in r2554. Also for session.h.

Minor nit 2: Seeing that the methods in XfroutClient had been changed from non-virtual to virtual confused me for a bit; I thought there must be another derived class somewhere that extended XfroutClient them, and was wondering if I was missing part of the code. I'd appreciate a comment explaining why they're virtual.

Hmm, I remember a discussion on jabber about the (redundant) virtual declaration in a derived class. I think this is a project-wide style guideline issue, rather than a nit to be discussed and addressed case by case basis. I'll bring this topic to bind10-dev. At the moment I'll keep the code unchanged.

Index: src/bin/auth/auth_srv.cc

"if (is_axfr_connection_established_)" is incorrect English ;)

More seriously, that flag only has to do with xfrout, not xfrin, so maybe it should have a more specific name? "xfrout_connected" or something.

It was named so in the original 221 branch (not by me), and I don't have a preference. I tend to agree xfrout_connected sounds better, so I changed it. r2555.

I'm not clear on why xfrout_client needs to be declared by the caller and passed into the AuthSrv constructor, as opposed to having it instantiated by the AuthSrvImpl constructor and leaving the AuthSrv constructor's signature unchanged. I don't see any place where xfrout_client is accessed from outside AuthSrv.

It's for testing. I've commented on this in auth_srv.h. r2555.

Index: src/bin/auth/asio_link.h

+namespace asio {
+class io_service;
+namespace ip {
+class address;
+}
+}

Does the above code serve a useful purpose? I commented it out and found it still compiled fine, though maybe other compilers would have problems. If it's needed, please add a comment explaining the reason.

Ah, good catch. We don't need the forward declation for ip::address any more. Removed. We still need io_service (this style of forward declartion is used in other places, but added a comment anyway). If a .cc compiles without this, it's probably because some other header file already declares it.

In a longer term, however, we should (be able to) obsolete IOService::get_io_service(), at which point we can remove the declaration completely.

Addressed in r2556 (and r2557).

There were some minor grammatical errors in the comment explaining the asio_link class (missing paragraph breaks, singular/plural disagreement)--I can just fix them in the branch.

Please feel free to make such level of changes. I think it's okay to make the changes directly on trunk (assuming this branch is merged to trunk soon).

I've quickly reread asio_link.h, and made a copule of minor editorial fixes myself (r2561), but there are probably more.

The comments about IOEndpoint say that it's a wrapper for both ip::address and for the tcp/udp endpoint classes. I think the first one is a mistake and will remove it in the branch.

Ah, right. I guess it was a leftover from an intermediate version of the branch. Thanks for the fix.

Comment on IOEndpoint::getAddress() says the return value is "not a real object" but appears to mean that it is one. I'm fixing the comment.

Good catch, that's correct.

The static IOSocket::getDummy* methods seem rather unusual; why can't there just be a derived class DummySocket? I'm not seeing why this needs to be in the base class API.

This is because I generally wanted to make this class opaque: the applications won't instantiate these (derived) classes, and they'll normally use instantiated sockets within asio_link (based on the real asio object). Dummy objects are exceptions for testing purposes.

That said, the decision on this point is in flux. When we generalize the wrapper interface we may want to allow the apps to instantiate specific derived socket classes directly. At that point we might revisit the policy.

At the moment I noted this point as comments. r2558.

+ / This method blocks until the \c stop() method is called via some
+
/ handler.

It does some other stuff too, right? :)

Right, and I saw you updated it. Thanks.

Index: src/bin/auth/asio_link.cc

This code seems fine...

+ asio_endpoint_placeholder_(
+ new tcp::endpoint(ip::address::from_string(address.toText()),
+ port)),
+ asio_endpoint_(*asio_endpoint_placeholder_)

...but I don't understand the point. Why not have asio_endpoint_ be a real object instead of a reference, and construct it in the normal way, without using "new" and "delete"? (See attached diff.)

We could do it (and admittedly it's simpler), but I didn't chose that approach due to the copy overhead. For TCP endpoints it might not be a big deal, but we'd probably want to avoid the overhead for every UDP query.

Admittedly, though, this might be a premature optimization. If you think we should keep the initial implementation simpler and revisit it if necessary with benchmark results, I'm fine with that.

At the moment, I've added some comments on this point. r2559.

Index: src/bin/xfrin/xfrin.py.in

+ # The default RR class is IN. We should fix this so that
+ # the class is passed in the command arg (where we specify
+ # the default)

This comment appears to be outdated; it seems as if the rrclass is passed in the command arguments now.

This is only partially true: rrclass is passed only in the notify case. I've updated the comment to indicate it's not *always* the case.

Index: src/bin/auth/tests/asio_link_unittest.cc

Two of the tests currently fail:

[ FAILED ] IOServiceTest.badPort
[ FAILED ] IOServiceTest.unavailableAddress

Hmm, I'm not sure why badPort failed. I guess your environment is Linux, but it passed on bind10.isc.org. I'll keep this test case for now. If any of the regular builders reports a failure, I'll look into it. If it's not identified and fixed by the time you're back, please report it as a separate new ticket.

I found why it failed for unavailableAddress, and fixed it. (r2553).

comment:27 Changed 9 years ago by jinmei

  • Status changed from reviewing to accepted

Branch merged.

Giving it back to Evan for any open point as noted in my previous response.

I'll keep this ticket open but move it from the review queue.

comment:28 Changed 9 years ago by jinmei

  • Owner changed from jinmei to each
  • Status changed from accepted to assigned

comment:29 Changed 9 years ago by each

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

I see no need to keep this open. Thanks, Jinmei.

Note: See TracTickets for help on using tickets.