Opened 7 years ago

Closed 7 years ago

#2559 closed defect (complete)

Obtain DHCP lease database parameters from configuration database

Reported by: stephen Owned by: stephen
Priority: medium Milestone: Sprint-DHCP-20130122
Component: database-all 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

Currently the database access parameters (username, password etc.) for the DHCP MySQL backend are had-coded. They should be obtainable from the configuration database.

Subtickets

Change History (11)

comment:1 Changed 7 years ago by stephen

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

comment:2 Changed 7 years ago by stephen

  • Status changed from assigned to accepted

comment:3 Changed 7 years ago by stephen

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

Now ready for review.

Missing is the documentation: after the review, I intend to merge master into this branch to get the BIND 10 guide to the latest version then add the information about configuring the database. I have not done so for this review, as I have already merged with master once, and don't wish to make life more difficult for the reviewer.

comment:4 Changed 7 years ago by marcin

  • Owner changed from UnAssigned to marcin

comment:5 Changed 7 years ago by marcin

  • Owner changed from marcin to stephen

Reviewed commit 79f0d62ef03b3774a0b0c6237eff78af821bdbc4

General: the modified and new files should have copyright headers updated to 2013:

  • src/bin/dhcp6/ctrl_dhcp6_srv.cc
  • src/bin/dhcp6/ctrl_dhcp6_srv.h
  • src/bin/dhcp6/dhcp6_srv.h
  • src/bin/dhcp6/main.cc
  • src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
  • src/lib/dhcpsrv/dbaccess_parser.cc
  • src/lib/dhcpsrv/lease_mgr_factory.cc
  • src/lib/dhcpsrv/memfile_lease_mgr.cc
  • src/lib/dhcpsrv/dbaccess_parser.h
  • src/lib/dhcpsrv/tests/dbaccess_parser_unittest.cc

src/bin/dhcp4/config_parser.cc
Line 1386: since the current_parser variable has been introduced it makes sense to use it further in the code rather than config_pair.first, e.g.

ParserPtr parser(createGlobalDhcp4ConfigParser(config_pair.first));

can be replaced with

ParserPtr parser(createGlobalDhcp4ConfigParser(current_parser));

The same applies to line 1402.

src/bin/dhcp6/config_parser.cc
Line 1421 and line 1437: the same comments as for src/bin/dhcp4/config_parser.cc

DbAccessParser::build: suggest that you consider using std::swap() to assign contents of the values_copy to values_ when the parsing is successful. In theory it should be faster than using the assignment operator.

DbAccessParser::commit: spurious space before dbaccess.empty() in line 91

src/lib/dhcpsrv/dbaccess_parser.h

ConfigPair definition (line 48): this type could be declared private within this class as it is not used externally. Alternatively, could this be declared in the dhcp_config_parser.h and become common for all parsers?

In the header of getDbAccessParameters function: s/corrected/correctly.

src/lib/dhcpsrv/dhcpsrv_messages.mes
Suggest that the following warning message:

% DHCPSRV_MEMFILE_WARNING using early version of memfile - leases will be lost after a restart

is modified to

% DHCPSRV_MEMFILE_WARNING using early version of memfile lease database - leases will be lost after a restart

as the bare word ''memfile'' is vague.

src/lib/dhcpsrv/tests/dbaccess_parser_unittest.cc

There are no tests for commit() function.

Do you need to:

#include <fstream>
#include <iostream>
#include <sstream>
#include <arpa/inet.h>

?

comment:6 Changed 7 years ago by stephen

General: the modified and new files should have copyright headers updated to 2013:

Well spotted - changed.

src/bin/dhcp4/config_parser.cc
src/bin/dhcp6/config_parser.cc
Line 1386: since the current_parser variable has been introduced it makes sense to use it further in the code rather than config_pair.first, e.g.

On reflection, it seemed better to remove current_parser and instead make config_pair the variable accessible from the catch clause. It is has been declared before the "try" block, and the declarations in the BOOST_FOREACH clauses removed.

DbAccessParser::build: suggest that you consider using std::swap() to assign contents of the values_copy to values_ when the parsing is successful. In theory it should be faster than using the assignment operator.

Again, well spotted. Changed.

DbAccessParser::commit: spurious space before dbaccess.empty() in line 91

Removed.

src/lib/dhcpsrv/dbaccess_parser.h
ConfigPair definition (line 48): this type could be declared private within this class as it is not used externally. Alternatively, could this be declared in the dhcp_config_parser.h and become common for all parsers?

Moved to dhcp_config_parser.h

In the header of getDbAccessParameters function: s/corrected/correctly.

Corrected.

src/lib/dhcpsrv/dhcpsrv_messages.mes
Suggest that the following warning message...

Warning message changed.

src/lib/dhcpsrv/tests/dbaccess_parser_unittest.cc
There are no tests for commit() function.

The code for creating the databqase access string has been broken into a separate method and a test added for it. A test for commit() has also been added.

Do you need to:
#include <fstream>
:

No - result of cut-and-paste. Redundant #includes removed.

comment:7 Changed 7 years ago by stephen

  • Owner changed from stephen to marcin

comment:8 Changed 7 years ago by marcin

  • Owner changed from marcin to stephen

src/lib/dhcpsrv/tests/dbaccess_parser_unittest.cc
Changing the scope of protected functions could be simplified (the redeclaration of functions could be avoided):

const StringPairMap& getDbAccessParameters() const {
    return (DbAccessParser::getDbAccessParameters());
}

std::string getDbAccessString() const {
    return (DbAccessParser::getDbAccessString());
}

can be replaced with

using DbAccessParser::getDbAccessParameters;
using DbAccessParser::getDbAccessString;

The ChangeLog entry should be created for this ticket as it is the feature that can be used on user-end.

comment:9 Changed 7 years ago by stephen

  • Owner changed from stephen to marcin

src/lib/dhcpsrv/tests/dbaccess_parser_unittest.cc
Suggested changes made.

src/bin/dhcpX/config_parser.cc
Removed a typedef in that is present in src/lib/dhcpsrv/dhcp_config_parser.h

ChangeLog
Suggested entry is:

XXX.	[func]		stephen
	Values of the parameters to access to the DHCP server database can
        now be set through the BIND 10 configuration mechanism.
	(Trac #2559, git xxxxx)

comment:10 Changed 7 years ago by marcin

  • Owner changed from marcin to stephen

Reviewed commit aad83b6edde86e70a460cdf887b985841c161016

All ok. Please merge.

comment:11 Changed 7 years ago by stephen

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

Merged to master in commit 6c6f405188cc02d2358e114c33daff58edabd52a

Note: See TracTickets for help on using tickets.