Opened 9 years ago

Closed 9 years ago

#575 closed enhancement (complete)

Port the way addresses are configured from resolver to auth.

Reported by: vorner Owned by: vorner
Priority: medium Milestone: A-Team-Sprint-20110309
Component: b10-auth Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 5.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Make the way ports and addresses are configured in resolver (trough listen_addresses) work in auth as well (for the sake of consistency). Either by some kind of copy & paste or by putting the code in some shared library.

Subtickets

Change History (10)

comment:1 Changed 9 years ago by vorner

  • Owner changed from vorner to UnAssigned
  • Status changed from new to reviewing

Hello

The branch is ready. It is a bigger diff, but most of it is moving of code around so it is shared by both resolver and auth. There are some added tests because the code was tested implicitly inside something else before. The second part of the branch is using the generalized code by auth as well.

There's some trouble caused by the #388 problem and some disabled tests because of that.

The change to boss is a short term hack to make it start and should be handled in #576 properly. It still uses dummylog instead of the new API, since the code is mostly copy & paste.

I propose this changelog entry:

[func]       vorner
Listening address and port configuration of b10-auth is the same as for
b10-resolver now. That means, it is configured trough bindctl at runtime,
in the Auth/listen_on list, not trough command line arguments.

comment:2 Changed 9 years ago by jinmei

From a quick look at it, the diff seems to be quite big.

Not really thought about the possibility, but would it be possible to
break this down into two or more pieces?

comment:3 follow-up: Changed 9 years ago by vorner

Hello

Since the branch was quite messy inside (the topics of related changes were interleaved), I reordered the branch on top of current master. I created a trac575-cleanup branch with it.

I think it makes no sense to merge the topics one by one into master, but only together. But I marked intervals of kind of more tightly coupled changes, so they probably can be reviewed separately (each interval alone). I didn't create new tickets for them, but I'm not against solving issues with each of them separately (for example just send comments for interval or two and after we are done with them, we could move along) or all at once.

The intervals are marked as branches, so you can name them more comfortably (eg. diff trac575-parts/1-libserver-common..trac575-parts/2-parseAddress):

  • trac575-parts/0-base (No changes here, just marked the version of master it is on)
  • trac575-parts/1-libserver-common (Creation of library of code shared by the two servers)
  • trac575-parts/2-parseAddress (Function for parsing of address lists from configuration)
  • trac575-parts/3-setting-up (Function for putting list of addresses into server)
  • trac575-parts/4-address-interface (Interface for setting of address of auth server)
  • trac575-parts/5-config (Handling of listen-on in auth server)
  • trac575-parts/6-finalize (Various finalizations ‒ man page, etc)

Is it better this way?

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

Could either Jerry or Feng take on this review (whole or part)?

comment:5 Changed 9 years ago by vorner

I surely have no objections to it and I think it should be possible to understand a part of it without reading the previous parts. It depends on them, but I think the review can easily be split (I just didn't have the idea about how to split it when I was coding it, it turned out during it).

comment:6 Changed 9 years ago by zzchen_pku

  • Owner changed from UnAssigned to zzchen_pku

okay, I'll take it.

comment:7 follow-up: Changed 9 years ago by zzchen_pku

  • Owner changed from zzchen_pku to vorner

Hello

Review done, please find comments below:

trac575-parts/2-parseAddress

  • resolver/resolver.cc
    • updateConfig() has a typo: "forward_addresse" -> "forward_addresses"
    • just a suggestion, vector<AddressPair?> can be replaced by AddressList?, I found both of them in the source.
  • portconfig_unittest.cc
  • portconfig.cc and portconfig_unittest.cc
    • #include "###" and be replaced by #include <###> for consistency

trac575-parts/4-address-interface

  • auth_srv.h
    • The comments is confusing, I think it is copied from some place without change:-)
      "Assign an ASIO DNS Service queue to this Resolver object"
      

trac575-parts/5-config

  • testutils/portconfig.h
    • It is not correct any more, since #388 has been merged into master.
      * \param server The server object to test against.                           
      * \todo This tests currently aborts due to #388 (it is impossible to close   
      *     sockets, so it is impossible to configure, deconfigure and then        
      *     configure them again). Once #388 is solved, enable the tests. 
      
  • auth/config.cc
    • #include <memory.h>. It seems the header file is not required.
Last edited 9 years ago by zzchen_pku (previous) (diff)

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

  • Owner changed from vorner to zzchen_pku

Hello

Replying to zzchen_pku:

  • portconfig.cc and portconfig_unittest.cc
    • #include "###" and be replaced by #include <###> for consistency

Done, I agree with the consistency, but I believe we use it consistently wrong. We don't need include path search for local headers. But anyway, that's a different discussion.

trac575-parts/4-address-interface

  • auth_srv.h
    • The comments is confusing, I think it is copied from some place without change:-)

It was, how did you tell I copied it? ;-)

  • It is not correct any more, since #388 has been merged into master.

This required a merge with master. I fixed some conflicts there and enabled all the tests that were disabled before.

So, the branch has 4 more commits. The 3 last ones are trivial fixes, but the 4th from the head (fa05a534849c2e3ba68d6fdc4c5ddc6531948fc3) is a non-trivial merge.

If you want to see the diff of whole branch, git diff 0a9ce967a515894bd7c46cf86a6ff4bc3d856b3a.. would do it. For some reason, git show fa05a534849c2e3ba68d6fdc4c5ddc6531948fc3 shows quite a lot of noise around. It looks like git is unable to get rid of it, but most of the lines that start with ' +' (+ in the second column) look uninteresting. It might help enabling colors for git diff a little.

Anyway, I believe all the things you mention are addressed.

Thank you

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

  • Owner changed from zzchen_pku to vorner

Replying to vorner:

trac575-parts/4-address-interface

  • auth_srv.h
    • The comments is confusing, I think it is copied from some place without change:-)

It was, how did you tell I copied it? ;-)

Hehe, as most of this task is moving of code around, so... just a guess:-)

  • It is not correct any more, since #388 has been merged into master.

This required a merge with master. I fixed some conflicts there and enabled all the tests that were disabled before.

So, the branch has 4 more commits. The 3 last ones are trivial fixes, but the 4th from the head (fa05a534849c2e3ba68d6fdc4c5ddc6531948fc3) is a non-trivial merge.

If you want to see the diff of whole branch, git diff 0a9ce967a515894bd7c46cf86a6ff4bc3d856b3a.. would do it. For some reason, git show fa05a534849c2e3ba68d6fdc4c5ddc6531948fc3 shows quite a lot of noise around. It looks like git is unable to get rid of it, but most of the lines that start with ' +' (+ in the second column) look uninteresting. It might help enabling colors for git diff a little.
Anyway, I believe all the things you mention are addressed.

Checked, it looks ok now, please go ahead and merge.

comment:10 Changed 9 years ago by vorner

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

Thanks, merged.

Note: See TracTickets for help on using tickets.