Opened 8 years ago

Closed 7 years ago

#1651 closed task (fixed)

Integrate DHCP4 process startup into BIND 10

Reported by: stephen Owned by: tomek
Priority: medium Milestone: Sprint-DHCP-20120703
Component: dhcp4 Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description (last modified by tomek)

Currently the processes have to be started from the command line. This change makes them part of BIND 10, able to be started and stopped from bindctl.

This is for v4 only. See #1708 for v6 counterpart.

Subtickets

Change History (12)

comment:1 Changed 8 years ago by tomek

  • Owner set to tomek
  • Status changed from new to accepted

comment:2 Changed 8 years ago by tomek

  • Component changed from dhcp to dhcp4
  • Description modified (diff)
  • Summary changed from Integrate DHCP process startup into BIND 10 to Integrate DHCP4 process startup into BIND 10

comment:3 Changed 8 years ago by tomek

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

Code is implemented and awaiting review. Implemented approach (using select() with a socket descriptor extracted from session object) was proposed by vorner (https://lists.isc.org/pipermail/bind10-dev/2012-May/003439.html).

BIND10 User's guide has been updated.

Developer's guide documentation has been updated.

There are no direct tests for configuration passing as configuration is not supported yet. There are no tests for shutdown command. We could write them now, but really don't know how to simulate commands. Furthermore, it would be needed to make the whole test setup working as non-privileged user. I would love to see a contributed code here (or at least pointers to similar tests, where I could learn how one can simulate msgq and pass control commands over it).

comment:4 Changed 8 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:5 follow-ups: Changed 8 years ago by stephen

  • Owner changed from stephen to tomek

Regarding testing:

We could write them now, but really don't know how to simulate commands.

Do the tests in src/bin/auth/tests give any clue to this?

Review
To avoid confusion with the number of files changed where "master" was merged into this branch, the review was done in two stages:

  • changes between commits 18eda2228413b1bd5764ad23dd5ab45a2d9c1809 and 66e868079f7a572809e5030d64de9caa4c550f73
  • changes between commits 4a09f5d1d17004095f18b9ada885cbdf61e8f088 and d1accb3e7b04acca3711181ab3e9cc5833e92f0f

src/bin/dhcp4/dhcp4_srv.h
"instructs" should start with capital I.

src/bin/dhcp4/main.cc
verbose_mode should have a comment descibing what it is.

I don't like global objects either. However, if you put the declaration of "dhcp4" in the anonymous namespace (preferred over declaring it static) it is scope-limited to the main.cc file, so I would not be too concerned about it.

The logic behind extracting the control socket and plugging it into the interface manager should be explained in a comment. (In fact, a fuller description of the interaction between asio and select() should be explained in a header to the file, as it is not limited to a single function.)

Function names should be likeThis (not like_this).

disconnect_session (or disconnectSession) needs a header comment.

src/lib/cc/tests/session_unittests.cc
The final EXPECT_TRUE(0 < socket) is better as EXPECT_LT(0, socket), as the error message is more informative in that it should print out the value of socket.

src/lib/dhcp/iface_mgr.{h,cc}
General: please start comments with a capital letter.

receive4(): should the argument be of type uint32_t?

receive4(): in the loop over the socket collection, suggest we replace:

    // We don't want IPv6 addresses here.
    if (s->addr_.getFamily() != AF_INET) {
        continue;
    }

    names << s->sockfd_ << "(" << iface->getName() << ") ";

    // add this socket to listening set
    FD_SET(s->sockfd_, &sockets);
    if (maxfd < s->sockfd_)
        maxfd = s->sockfd_;

with

    // Only deal with IPv4 addresses.
    if (s->addr_.getFamily() == AF_INET) {
       names << s->sockfd_ << "(" << iface->getName() << ") ";

       // Add this socket to listening set
       FD_SET(s->sockfd_, &sockets);
       if (maxfd < s->sockfd_) {
           maxfd = s->sockfd_;
       }
    }

(Note also the braces around the single-line if statement.)

receive4(): when testing maxfd against session_socket_, there should be a brace around the single-line "if" statement.

receive4(): this builds the socket set on each call. Do we need this? Can't the socket set be encapsulated in a separate object and stored as a member of the IfaceMgr class?

receive4(): at the "Socket read error" message, is there a need to strncpy() the return from strerror() into a temporary buffer? Can't a call to strerror() be incorporated in the cout call directly>

receive4(): the "if (result < 0) {" test at the "Socket read error" message is probably better as an "else if", since it is mutually exclusive with the preceding "if" clause.

receive4(): rather than checking "if (session_socket_)", better is "if (session_socket_ > 0)" (as the socket is a number.) Incidentally, as 0 is a valid file descriptor, wouldn't it be better to initialize session_socket_ to an invalid descriptor number, e.g. -1, to indicate that it has not been set?

comment:6 in reply to: ↑ 5 Changed 8 years ago by tomek

Replying to stephen:

Regarding testing:

We could write them now, but really don't know how to simulate commands.

Do the tests in src/bin/auth/tests give any clue to this?

I was hoping for a bit more specific location. Another place where I looked for is srv/bin/resolver/tests.

src/bin/dhcp4/dhcp4_srv.h
"instructs" should start with capital I.

Done.

src/bin/dhcp4/main.cc
verbose_mode should have a comment descibing what it is.

I don't like global objects either. However, if you put the declaration of "dhcp4" in the anonymous namespace (preferred over declaring it static) it is scope-limited to the main.cc file, so I would not be too concerned about it.

To test session code, I had to refactor it a bit. To call session related methods/functions, they were moved to separate class ControlledDhcpv4Srv. The original idea still stands - for embedded environment, we will instantiate Dhcpv4Srv object directly. For "normal" BIND10 environment, we will create ControlledDhcpv4Srv. This new class code is available in ctrl_dhcp4_srv.{cc|h} files.

The logic behind extracting the control socket and plugging it into the interface manager should be explained in a comment. (In fact, a fuller description of the interaction between asio and select() should be explained in a header to the file, as it is not limited to a single function.)

It is described thoroughly in doc/devel/02-dhcp.dox. I'm not sure if you haven't seen this description, saying that it is not detailed enough or located in the wrong place?

This is a generic description of the architecture and spans several classes, thus no single class or file is the right place for such description, thus separate file. I have added pointers to that file in main.cc, dhcp4_srv.h and ctrl_dhcp4_srv.h.

Function names should be likeThis (not like_this).

Fixed.

disconnect_session (or disconnectSession) needs a header comment.

It was moved to a newly created ctrl_dhcpv4_srv.{cc|h} class, renamed and commented appropriately.

The code has been refactored significantly. Most of the code that used to be in main.cc is now part of the ControlledDhcpv4Srv class. There are no more global objects anymore.

comment:7 in reply to: ↑ 5 Changed 8 years ago by tomek

  • Owner changed from tomek to stephen

Replying to stephen:

src/lib/cc/tests/session_unittests.cc
The final EXPECT_TRUE(0 < socket) is better as EXPECT_LT(0, socket), as the error message is more informative in that it should print out the value of socket.

Done.

src/lib/dhcp/iface_mgr.{h,cc}
General: please start comments with a capital letter.

Done. I hope I spotted them all.

receive4(): should the argument be of type uint32_t?

No necessarily. uintXX_t formats are typically used when it is essential to maintain consistency between 32 and 64 bits. Here it is not a problem - 64bit compilation could select for longer if there was nothing to do (e.g. empty lease database, so there is nothing to expire. 32bit would call select() for 4 billion seconds, 64 bit would call it for a lot longer). Of course it doesn't matter much, so I've changed it as requested.

receive4(): in the loop over the socket collection, suggest we replace:

    // We don't want IPv6 addresses here.
    if (s->addr_.getFamily() != AF_INET) {
        continue;
    }

    names << s->sockfd_ << "(" << iface->getName() << ") ";

    // add this socket to listening set
    FD_SET(s->sockfd_, &sockets);
    if (maxfd < s->sockfd_)
        maxfd = s->sockfd_;

with

    // Only deal with IPv4 addresses.
    if (s->addr_.getFamily() == AF_INET) {
       names << s->sockfd_ << "(" << iface->getName() << ") ";

       // Add this socket to listening set
       FD_SET(s->sockfd_, &sockets);
       if (maxfd < s->sockfd_) {
           maxfd = s->sockfd_;
       }
    }

(Note also the braces around the single-line if statement.)

Done. Personally I think first style, as there is one less indent level. The code readability was marginally better in my opinion. I don't have strong opinion, so I changed the code as requested.

receive4(): when testing maxfd against session_socket_, there should be a brace around the single-line "if" statement.

receive4(): this builds the socket set on each call. Do we need this? Can't the socket set be encapsulated in a separate object and stored as a member of the IfaceMgr class?

Yes, we need this. No, i can't be encapsulated. select() modifies passed set and sets those sockets, which have a data to read. We could probably define a class member of fd_set type, initialize it once and then use its copy every time we want to read something. There are 2 problems with that approach:

  1. fs_set is a bitfield. Its size my vary from system to system. It is a C structure, so there is no constructor. It could probably be copied with memcpy(dst,src, sizeof(fd_set)). The only allowed way of modifying fd_set is with provided FD_SET, FD_ISSET, FD_CLEAR, etc. macros. It is possible that on some systems that is not a structure, but an array of some sort.
  2. Once we implement support for dynamic interfaces (i.e. detecting that interface disappeared), we will need to update that structure.

So the answer is: yes, it can be code, I just don't want to spend too much time on it at this stage. Added todo comment about possible marginal performance improvement.

receive4(): at the "Socket read error" message, is there a need to strncpy() the return from strerror() into a temporary buffer? Can't a call to strerror() be incorporated in the cout call directly>

Removed temporary buffer.

receive4(): the "if (result < 0) {" test at the "Socket read error" message is probably better as an "else if", since it is mutually exclusive with the preceding "if" clause.

Fixed.

receive4(): rather than checking "if (session_socket_)", better is "if (session_socket_ > 0)" (as the socket is a number.) Incidentally, as 0 is a valid file descriptor, wouldn't it be better to initialize session_socket_ to an invalid descriptor number, e.g. -1, to indicate that it has not been set?

In theory session_socket_ could be 0, but that file descriptor is always taken by stdin. I fail to imagine a situation, where control socket could be the first file descriptor opened by a process. Anyway, Introduced InvalidSocket? constant (with -1 value).

Also added references to the common description.

Ready for review.

comment:8 Changed 8 years ago by tomek

I forgot to push my changes. Sorry. Pushed to public repo, brach trac1651.

comment:9 follow-up: Changed 8 years ago by stephen

  • Owner changed from stephen to tomek

Reviewed commit afc4bd6b79b9469b5717d3a1b0ac729cf6aacef7

src/bin/dhcp4/ctrl_dhcp4_srv.cc
Initialization of logging should be done in main() - see below.

dhcp4CommandHandler(): Suggest outputting a message if "shutdown" is received and server_ is null - something must be wrong.

establishSession():

The logic behind extracting the control socket and plugging it into the interface manager should be explained in a comment. (In fact, a fuller description of the interaction between asio and select() should be explained in a header to the file, as it is not limited to a single function.)

It is described thoroughly in doc/devel/02-dhcp.dox. I'm not sure if you haven't seen this description, saying that it is not detailed enough or located in the wrong place?

Locating the detailed explanation in the .dox file is fine. (The contents are fine, although we probably need to run it - and all other .dox files - through an editing pass at some time.) All I was suggesting is adding a short comment explaining why the control socket is retrieved from the configuration session and set in the interface manager at the point where this takes place (and the reason is not obvious). Something along the lines of:

// Integrate the asynchronous I/O model of BIND 10 configuration
// control with the "select" model of the DHCP server.  This is
// fully explained in \ref dhcpv4Session.

disconnectSession(): closing the configuration session probably closes the file descriptor passed to the interface manager during initialization. Although this is part of shutdown, it might safest as part of session disconnection to clear the socket in the interface manager to prevent its further use. (Incidentally, in iface_mgr.h, InvalidSocket is probably better named INVALID_SOCKET to emphasise that it is a constant.)

The code has been refactored significantly. Most of the code that used to be in main.cc is now part of the ControlledDhcpv4Srv class. There are no more global objects anymore.

Not strictly true - ControlledDhcpv4Srv::server_ is still global in that it can be referenced outside the file.

execDhcpv4ServerCommand(): Argument to first "return" should be enclosed in parentheses.

src/bin/dhcp4/ctrl_dhcp4_srv.h
disconnectSession(): in the header comments, s/not/no/

If only referenced by handlers within the ControlledDhcpv4Srv class, server_ could be declared private.

src/bin/dhcp4/main.cc
In main(), the declaration of "server" could be within the "try" block (and part of the initialization) as it is not referenced outside. (This also means that the setting of server to NULL could be removed.)

The initialization of logging has been moved to ControlledDhcpv4Srv. I think that should be left in main() and done as early as possible, else no messages can be output until the server is initialized (which prevents any debug messages stating that the server is initializing).

Something missed on the last review: does the "getopt()" argument require the colon in the ":v" flags? It - and the "case ':'" could be removed.

The program should probably terminate if "usage()" is executed - it implies that the program has been incorrectly invoked. (Or invoked interactively with the wrong flags - perhaps the user specified "-h"?)

src/lib/dhcp/iface_mgr.cc

Yes, we need this. No, it can't be encapsulated.

You're right, I missed the bit that select() modifies the list of file descriptors.

I fail to imagine a situation, where control socket could be the first file descriptor opened by a process.

Never underestimate the influence of Murphy's Law :-)

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

  • Owner changed from tomek to stephen

Replying to stephen:

Reviewed commit afc4bd6b79b9469b5717d3a1b0ac729cf6aacef7

src/bin/dhcp4/ctrl_dhcp4_srv.cc
Initialization of logging should be done in main() - see below.

dhcp4CommandHandler(): Suggest outputting a message if "shutdown" is received and server_ is null - something must be wrong.

Done.

establishSession():
Something along the lines of:

// Integrate the asynchronous I/O model of BIND 10 configuration
// control with the "select" model of the DHCP server.  This is
// fully explained in \ref dhcpv4Session.

Thank you for contributting the text. Added it to establishSession() description.

disconnectSession(): closing the configuration session probably closes the file descriptor passed to the interface manager during initialization. Although this is part of shutdown, it might safest as part of session disconnection to clear the socket in the interface manager to prevent its further use. (Incidentally, in iface_mgr.h, InvalidSocket is probably better named INVALID_SOCKET to emphasise that it is a constant.)

Renamed InvalidSocket? to INVALID_SOCKET. disconnectSession() now also deregisters control socket.

The code has been refactored significantly. Most of the code that used to be in main.cc is now part of the ControlledDhcpv4Srv class. There are no more global objects anymore.

Not strictly true - ControlledDhcpv4Srv::server_ is still global in that it can be referenced outside the file.

That is technically true, but it is much easier to understand the context of this globally accessible class member.

execDhcpv4ServerCommand(): Argument to first "return" should be enclosed in parentheses.

Done.

src/bin/dhcp4/ctrl_dhcp4_srv.h
disconnectSession(): in the header comments, s/not/no/

Done.

If only referenced by handlers within the ControlledDhcpv4Srv class, server_ could be declared private.

Done.

src/bin/dhcp4/main.cc
In main(), the declaration of "server" could be within the "try" block (and part of the initialization) as it is not referenced outside. (This also means that the setting of server to NULL could be removed.)

Done.

The initialization of logging has been moved to ControlledDhcpv4Srv. I think that should be left in main() and done as early as possible, else no messages can be output until the server is initialized (which prevents any debug messages stating that the server is initializing).

Done.

Something missed on the last review: does the "getopt()" argument require the colon in the ":v" flags? It - and the "case ':'" could be removed.

Done.

The program should probably terminate if "usage()" is executed - it implies that the program has been incorrectly invoked. (Or invoked interactively with the wrong flags - perhaps the user specified "-h"?)

Note that exit() is called at the end of usage() function. Note that this part of the code was refactored as part of #1503 ticket (already merged). I'd like to keep changes in this part of the code to bare minimum to make the merge easier.

I fail to imagine a situation, where control socket could be the first file descriptor opened by a process.

Never underestimate the influence of Murphy's Law :-)

Unfortunately, I know this gentleman too well already. Anyway, previous commit added specific check against InvalidSocket? (now renamed to INVALID_SOCKET), so the code is safe.

I believe this commit solves all outstanding issues. If there are no new comments, it is ready to merge.

comment:11 Changed 7 years ago by stephen

  • Owner changed from stephen to tomek

Reviewed commit 4472bf857531304a459cbd6fc92d048a0d0d801f

Thank you for contributing the text. Added it to establishSession() description.

I intended that the text comment the two lines in the .cc file that obtained the control socket and set it in the interface manager. The description of establishSession() was fine - it was just that the reason for that part of its implementation was not immediately clear. I've moved the lines between the two files and pushed back to the repository.

All the other changes are OK, so if you're happy with my modification, please merge.

comment:12 Changed 7 years ago by tomek

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

Pushed to master, closing ticket.

Note: See TracTickets for help on using tickets.