#4281 closed enhancement (complete)

Extend MySQL host data source with options

Reported by: tomek Owned by: marcin
Priority: medium Milestone: Kea1.1
Component: host-reservations Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 56 Add Hours to Ticket: .5
Total Hours: 75.5 Internal?: no

Description

HostReservationDesign plans for v4 and v6 options to be stored in MySQL. This ticket covers implementing this functionality.

Subtickets

Change History (13)

comment:1 Changed 20 months ago by marcin

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

comment:2 Changed 19 months ago by marcin

  • Add Hours to Ticket changed from 0 to 55
  • Estimated Difficulty changed from 0 to 56
  • Owner changed from marcin to UnAssigned
  • Status changed from accepted to reviewing
  • Total Hours changed from 0 to 55

I extended MySQL host data source to retrieve DHCPv4 and DHCPv6 options along with host information and IPv6 reservations. I also extended addHost method to insert options into the database. The updated addHost method uses transaction to rollback database updates if insertion fails at any stage.

Proposed ChangeLog entry:

11XX.	[func]		marcin
	Extended MySQL host datasource to retrieve DHCPv4 and DHCPv6
	options associated with hosts from a MySQL database.
	(Trac #4281, git abcd)

comment:3 Changed 19 months ago by tmark

  • Owner changed from UnAssigned to tmark

comment:4 follow-up: Changed 19 months ago by tmark

  • Add Hours to Ticket changed from 55 to 10
  • Total Hours changed from 55 to 65

I can see you put a lot of thought and effort into this. Nicely done.

First some general items:


Are we anticipating reusing the dhcp4_options and dhcp6_options tables to store global, subent, and class options? I'm assuming we are and that the following would be true:

  1. A global option would be class = NULL, subnet id = NULL, and host id = NULL
  2. A subnet option would be class = NULL, subnet id != NULL, and host id = NULL
  3. A class option would be class != NULL, subnet id = NULL, and host id = NULL

I'm wondering if we should add a "scope" column now that would G=global, C=class, S=subnet, H=host? That would give us a way to index/sort options by scope and make where clauses and constraints easier to write.

It would be better to do this now rather than later.


I don't see the point of having MySqlHostDataSourceImpl?. MySqlDataHostDataSource? already
has a lot of direct knowledge of MySQL API details such as the bind arrays. We couldn't
realistically replace the impl with much that wouldn't also require changiing MySqlHostDataSource?. Using PIMPL here seems to add an extra layer with no real advantage. Am I missing something?


Why are you using the DISTINCT keyword on bascially all of the host selects? Are you using this as a safeguard against bad data or are there specific case(s) where the data is valid and the queries would otherwise return duplicate rows? The distinct keyword compares each row by the entire row contents. If we don't need it, we should omit as it has performance implications.


Now to the specifics:


src/lib/dhcp/libdhcp++.cc

LibDHCP::optionSpaceToVendorId(), you could combine the
content checks and range checks into single expressions, and like so:

    // Minimal content is "vendor-X" format 
    if ((option_space.size() < 8) || (option_space.substr(0,7) != "vendor-")) {
        return (0);
    }

and

    if ((check < 0) || (check > std::numeric_limits<uint32_t>::max())) {
        return (0);
    }

and reduce the scope of std::string x by moving it's declaration inside the the try block.

Just sayin ...


MySqlHostDataSourceImpl?;;checkError - We need to consider what we want to do about fatal DB errors here (see MySqlLeaseMgr::checkError()). If the server cannot talk to it's host datasource, what should it do? If it is the same source as the lease data source, then the latter should fail, but when they are different you could lose one but not the other.


MySqlHostDataSource::add()

  • line 2181 Comment says we'll call "addHost() code" but there's no such animal, just change it to Insert the host...
  • We only set host_id if there is at least one reservation but otherwise will be zero, which means it will be wrong when inserting options for hosts with no reservations. You should simply set host_id when you declare it.
  • I would also be tempted to move the block of code that inserts the IPv6Rervations to after the options are done (ie. just above the commit.)

I would prefer you renamed addQuery to addStatement Query tends to refer to questions, i.e. selects() or things which return result sets, where as statements in SQL return an count of rows affected.


MySqlTransaction? -

It seems a litle odd for the MySqlConnection? class to implement commit() and rollback() while starting a transaction is implemented in MySqlTransaction?. MySqlConnection?() should implement a startTransacion() method and MySqlTransaction?'s constructor should invoke that. That way the MySQL details for starting, committing, or rolling back transactions are all in the same class.


MysqlHostDataSourceImpl? constructor

Since we are using explicit transactions we should be turning
autocommit OFF not turning it on.

GenericHostDataSourceTest::testOptionsReservations4()

In this test though you use addTestOptions() to create the host to insert, which in fact
inserts a host with both v4 and v6 options. The v6 options are (correctly) not returned by the get4 calls. The only reason tests pass is because of the order of the arguments to compareHosts(). If you reverse the order like so:

    ASSERT_NO_FATAL_FAILURE(compareHosts(*hosts_by_addr.begin(), host));

the test fails. At first read, one wonders how this test passes. On the one hand it is a verification that get4() calls exclude V6 options, on the other hand it is unclear that you intended this. You need to at least document this behavior. Alternatively, you could alter addTestOptions to accept an argument of MySqlHostWithOptionsExchange::FetchedOptions?() and only create the options you intend.
(This also applies to testOptionsReservations6())

Also, the last line of the test is this:

    hdsptr_->get4(subnet_id, IOAddress("192.0.2.5"));

Did you mean to do something more?


GenericHostDataSourceTest::compareOptions()

Related to the issue above, this test does NOT check cfg1 and cfg2 have the same number of option spaces. If cfg2 is a subset of cfg1 the comparison will pass. If this intended behavior then it needs to be documented.


GenericHostDataSourceTest::initializeHost4()
GenericHostDataSourceTest::initializeHost6()

Shouldn't the increments of subnet4 and subnet6 be prefix not postfix?


Unit tests and cppcheck looked good under OS-X.

Last edited 19 months ago by tmark (previous) (diff)

comment:5 Changed 19 months ago by tmark

  • Owner changed from tmark to marcin

comment:6 in reply to: ↑ 4 ; follow-up: Changed 19 months ago by marcin

  • Add Hours to Ticket changed from 10 to 6
  • Owner changed from marcin to tmark
  • Total Hours changed from 65 to 71

Replying to tmark:

I can see you put a lot of thought and effort into this. Nicely done.

Thanks

First some general items:


Are we anticipating reusing the dhcp4_options and dhcp6_options tables to store global, subent, and class options? I'm assuming we are and that the following would be true:

  1. A global option would be class = NULL, subnet id = NULL, and host id = NULL
  2. A subnet option would be class = NULL, subnet id != NULL, and host id = NULL
  3. A class option would be class != NULL, subnet id = NULL, and host id = NULL

I'm wondering if we should add a "scope" column now that would G=global, C=class, S=subnet, H=host? That would give us a way to index/sort options by scope and make where clauses and constraints easier to write.

It would be better to do this now rather than later.

I wasn't very keen to do it now, but I eventually decided to do it. I think it will be necessary to facilitate cases like specifying options for subnets, classes etc. in the database. I want to minimize the impact on the users' databases when they migrate to new version of Kea. So, I thought that major schema changes should be made sooner than later. But, I didn't add any index for the scope_id field, because I am not sure what queries will use it and how.


I don't see the point of having MySqlHostDataSourceImpl?. MySqlDataHostDataSource? already
has a lot of direct knowledge of MySQL API details such as the bind arrays. We couldn't
realistically replace the impl with much that wouldn't also require changiing MySqlHostDataSource?. Using PIMPL here seems to add an extra layer with no real advantage. Am I missing something?

The only one remaining stuff specific to the implementation in the header file was a list of statements. I moved it to the implementation file. The MySqlDataSourceImpl? contains quite a lot of stuff very specific to MySqlHostDataSource?, so I didn't want to keep it in the header file.


Why are you using the DISTINCT keyword on bascially all of the host selects? Are you using this as a safeguard against bad data or are there specific case(s) where the data is valid and the queries would otherwise return duplicate rows? The distinct keyword compares each row by the entire row contents. If we don't need it, we should omit as it has performance implications.

I think this is a reminder from my early attempts to construct the query which will gather all the host specific information. At that time I believed it was required. I removed it now and the tests pass so it should be fine without it.


Now to the specifics:


src/lib/dhcp/libdhcp++.cc

LibDHCP::optionSpaceToVendorId(), you could combine the
content checks and range checks into single expressions, and like so:

    // Minimal content is "vendor-X" format 
    if ((option_space.size() < 8) || (option_space.substr(0,7) != "vendor-")) {
        return (0);
    }

and

    if ((check < 0) || (check > std::numeric_limits<uint32_t>::max())) {
        return (0);
    }

and reduce the scope of std::string x by moving it's declaration inside the the try block.

Just sayin ...

Made this change.


MySqlHostDataSourceImpl?;;checkError - We need to consider what we want to do about fatal DB errors here (see MySqlLeaseMgr::checkError()). If the server cannot talk to it's host datasource, what should it do? If it is the same source as the lease data source, then the latter should fail, but when they are different you could lose one but not the other.

We should probably discuss it more. But, I don't believe this really belong to this ticket?


MySqlHostDataSource::add()

  • line 2181 Comment says we'll call "addHost() code" but there's no such animal, just change it to Insert the host...

Changed.

  • We only set host_id if there is at least one reservation but otherwise will be zero, which means it will be wrong when inserting options for hosts with no reservations. You should simply set host_id when you declare it.

No. The host_id for options is set in !MySqlHostDataSourceImpl::addOptions. I am just trying to avoid invoking mysql_insert_id when it is not necessary.

  • I would also be tempted to move the block of code that inserts the IPv6Rervations to after the options are done (ie. just above the commit.)

Moved.


I would prefer you renamed addQuery to addStatement Query tends to refer to questions, i.e. selects() or things which return result sets, where as statements in SQL return an count of rows affected.

Renamed.


MySqlTransaction? -

It seems a litle odd for the MySqlConnection? class to implement commit() and rollback() while starting a transaction is implemented in MySqlTransaction?. MySqlConnection?() should implement a startTransacion() method and MySqlTransaction?'s constructor should invoke that. That way the MySQL details for starting, committing, or rolling back transactions are all in the same class.

Changed as suggested.


MysqlHostDataSourceImpl? constructor

Since we are using explicit transactions we should be turning
autocommit OFF not turning it on.

Disabled by default.

GenericHostDataSourceTest::testOptionsReservations4()

In this test though you use addTestOptions() to create the host to insert, which in fact
inserts a host with both v4 and v6 options. The v6 options are (correctly) not returned by the get4 calls. The only reason tests pass is because of the order of the arguments to compareHosts(). If you reverse the order like so:

    ASSERT_NO_FATAL_FAILURE(compareHosts(*hosts_by_addr.begin(), host));

the test fails. At first read, one wonders how this test passes. On the one hand it is a verification that get4() calls exclude V6 options, on the other hand it is unclear that you intended this. You need to at least document this behavior. Alternatively, you could alter addTestOptions to accept an argument of MySqlHostWithOptionsExchange::FetchedOptions?() and only create the options you intend.
(This also applies to testOptionsReservations6())

Yes. I updated the compareHosts function to check number of options within spaces. I also added parameter which controls which options are created for host.

Also, the last line of the test is this:

    hdsptr_->get4(subnet_id, IOAddress("192.0.2.5"));

Did you mean to do something more?

No, removed.


GenericHostDataSourceTest::compareOptions()

Related to the issue above, this test does NOT check cfg1 and cfg2 have the same number of option spaces. If cfg2 is a subset of cfg1 the comparison will pass. If this intended behavior then it needs to be documented.

Added the comparison for number of options.


GenericHostDataSourceTest::initializeHost4()
GenericHostDataSourceTest::initializeHost6()

Shouldn't the increments of subnet4 and subnet6 be prefix not postfix?

Changed, although this wasn't really the code I touched.


Unit tests and cppcheck looked good under OS-X.

Thanks.

comment:7 in reply to: ↑ 6 ; follow-up: Changed 19 months ago by tmark

Replying to marcin:

Replying to tmark:

I can see you put a lot of thought and effort into this. Nicely done.

Thanks

First some general items:


Are we anticipating reusing the dhcp4_options and dhcp6_options tables to store global, subent, and class options? I'm assuming we are and that the following would be true:

  1. A global option would be class = NULL, subnet id = NULL, and host id = NULL
  2. A subnet option would be class = NULL, subnet id != NULL, and host id = NULL
  3. A class option would be class != NULL, subnet id = NULL, and host id = NULL

I'm wondering if we should add a "scope" column now that would G=global, C=class, S=subnet, H=host? That would give us a way to index/sort options by scope and make where clauses and constraints easier to write.

It would be better to do this now rather than later.

I wasn't very keen to do it now, but I eventually decided to do it. I think it will be necessary to facilitate cases like specifying options for subnets, classes etc. in the database. I want to minimize the impact on the users' databases when they migrate to new version of Kea. So, I thought that major schema changes should be made sooner than later. But, I didn't add any index for the scope_id field, because I am not sure what queries will use it and how.

Thank you, I believe this was the right thing to do.


I don't see the point of having MySqlHostDataSourceImpl?. MySqlDataHostDataSource? already
has a lot of direct knowledge of MySQL API details such as the bind arrays. We couldn't
realistically replace the impl with much that wouldn't also require changiing MySqlHostDataSource?. Using PIMPL here seems to add an extra layer with no real advantage. Am I missing something?

The only one remaining stuff specific to the implementation in the header file was a list of statements. I moved it to the implementation file. The MySqlDataSourceImpl? contains quite a lot of stuff very specific to MySqlHostDataSource?, so I didn't want to keep it in the header file.

Hmm. Not sure I agree but I won't press the issue.


Why are you using the DISTINCT keyword on bascially all of the host selects? Are you using this as a safeguard against bad data or are there specific case(s) where the data is valid and the queries would otherwise return duplicate rows? The distinct keyword compares each row by the entire row contents. If we don't need it, we should omit as it has performance implications.

I think this is a reminder from my early attempts to construct the query which will gather all the host specific information. At that time I believed it was required. I removed it now and the tests pass so it should be fine without it.

Excellent.


Now to the specifics:


src/lib/dhcp/libdhcp++.cc

LibDHCP::optionSpaceToVendorId(), you could combine the
content checks and range checks into single expressions, and like so:

    // Minimal content is "vendor-X" format 
    if ((option_space.size() < 8) || (option_space.substr(0,7) != "vendor-")) {
        return (0);
    }

and

    if ((check < 0) || (check > std::numeric_limits<uint32_t>::max())) {
        return (0);
    }

and reduce the scope of std::string x by moving it's declaration inside the the try block.

Just sayin ...

Made this change.


MySqlHostDataSourceImpl?;;checkError - We need to consider what we want to do about fatal DB errors here (see MySqlLeaseMgr::checkError()). If the server cannot talk to it's host datasource, what should it do? If it is the same source as the lease data source, then the latter should fail, but when they are different you could lose one but not the other.

We should probably discuss it more. But, I don't believe this really belong to this ticket?

If not on this ticket, it certainly needs one of it's own or find the related ticket(s) and update them to include host data sources.


MySqlHostDataSource::add()

  • line 2181 Comment says we'll call "addHost() code" but there's no such animal, just change it to Insert the host...

Changed.

  • We only set host_id if there is at least one reservation but otherwise will be zero, which means it will be wrong when inserting options for hosts with no reservations. You should simply set host_id when you declare it.

No. The host_id for options is set in !MySqlHostDataSourceImpl::addOptions. I am just trying to avoid invoking mysql_insert_id when it is not necessary.

Ok that's pretty subtle, I didn't catch it before.

It is my understanding that mysql_insert_id() returns a value that is set by the last insert or update and is kept on the client side of the session. It's akin to calling mysql_affected_rows() or mysql_errno(). Invoking this function does not result in an exchange with server. You could just always get the value after inserting the host and simplify the other methods to require a non-zero value for host_id. I think this would be a cleaner all around.

If you disagree with this, then perhaps you should make addRerv() behave as addOptions() and fetch the id if it's zero. This would make things more consistently implemented.

  • I would also be tempted to move the block of code that inserts the IPv6Rervations to after the options are done (ie. just above the commit.)

Moved.


I would prefer you renamed addQuery to addStatement Query tends to refer to questions, i.e. selects() or things which return result sets, where as statements in SQL return an count of rows affected.

Renamed.


MySqlTransaction? -

It seems a litle odd for the MySqlConnection? class to implement commit() and rollback() while starting a transaction is implemented in MySqlTransaction?. MySqlConnection?() should implement a startTransacion() method and MySqlTransaction?'s constructor should invoke that. That way the MySQL details for starting, committing, or rolling back transactions are all in the same class.

Changed as suggested.


MysqlHostDataSourceImpl? constructor

Since we are using explicit transactions we should be turning
autocommit OFF not turning it on.

Disabled by default.

GenericHostDataSourceTest::testOptionsReservations4()

In this test though you use addTestOptions() to create the host to insert, which in fact
inserts a host with both v4 and v6 options. The v6 options are (correctly) not returned by the get4 calls. The only reason tests pass is because of the order of the arguments to compareHosts(). If you reverse the order like so:

    ASSERT_NO_FATAL_FAILURE(compareHosts(*hosts_by_addr.begin(), host));

the test fails. At first read, one wonders how this test passes. On the one hand it is a verification that get4() calls exclude V6 options, on the other hand it is unclear that you intended this. You need to at least document this behavior. Alternatively, you could alter addTestOptions to accept an argument of MySqlHostWithOptionsExchange::FetchedOptions?() and only create the options you intend.
(This also applies to testOptionsReservations6())

Yes. I updated the compareHosts function to check number of options within spaces. I also added parameter which controls which options are created for host.

Also, the last line of the test is this:

    hdsptr_->get4(subnet_id, IOAddress("192.0.2.5"));

Did you mean to do something more?

No, removed.


GenericHostDataSourceTest::compareOptions()

Related to the issue above, this test does NOT check cfg1 and cfg2 have the same number of option spaces. If cfg2 is a subset of cfg1 the comparison will pass. If this intended behavior then it needs to be documented.

Added the comparison for number of options.


GenericHostDataSourceTest::initializeHost4()
GenericHostDataSourceTest::initializeHost6()

Shouldn't the increments of subnet4 and subnet6 be prefix not postfix?

Changed, although this wasn't really the code I touched.

Alas, that's never an excuse is it? ;)


Unit tests and cppcheck looked good under OS-X.

Thanks.

All other changes are fine.

comment:8 Changed 19 months ago by tmark

  • Add Hours to Ticket changed from 6 to 2
  • Owner changed from tmark to marcin
  • Total Hours changed from 71 to 73

comment:9 in reply to: ↑ 7 Changed 19 months ago by marcin

  • Owner changed from marcin to tmark
  • Total Hours changed from 73 to 75

Replying to tmark:

Replying to marcin:

Replying to tmark:

I can see you put a lot of thought and effort into this. Nicely done.

Thanks

First some general items:


Are we anticipating reusing the dhcp4_options and dhcp6_options tables to store global, subent, and class options? I'm assuming we are and that the following would be true:

  1. A global option would be class = NULL, subnet id = NULL, and host id = NULL
  2. A subnet option would be class = NULL, subnet id != NULL, and host id = NULL
  3. A class option would be class != NULL, subnet id = NULL, and host id = NULL

I'm wondering if we should add a "scope" column now that would G=global, C=class, S=subnet, H=host? That would give us a way to index/sort options by scope and make where clauses and constraints easier to write.

It would be better to do this now rather than later.

I wasn't very keen to do it now, but I eventually decided to do it. I think it will be necessary to facilitate cases like specifying options for subnets, classes etc. in the database. I want to minimize the impact on the users' databases when they migrate to new version of Kea. So, I thought that major schema changes should be made sooner than later. But, I didn't add any index for the scope_id field, because I am not sure what queries will use it and how.

Thank you, I believe this was the right thing to do.


I don't see the point of having MySqlHostDataSourceImpl?. MySqlDataHostDataSource? already
has a lot of direct knowledge of MySQL API details such as the bind arrays. We couldn't
realistically replace the impl with much that wouldn't also require changiing MySqlHostDataSource?. Using PIMPL here seems to add an extra layer with no real advantage. Am I missing something?

The only one remaining stuff specific to the implementation in the header file was a list of statements. I moved it to the implementation file. The MySqlDataSourceImpl? contains quite a lot of stuff very specific to MySqlHostDataSource?, so I didn't want to keep it in the header file.

Hmm. Not sure I agree but I won't press the issue.


Why are you using the DISTINCT keyword on bascially all of the host selects? Are you using this as a safeguard against bad data or are there specific case(s) where the data is valid and the queries would otherwise return duplicate rows? The distinct keyword compares each row by the entire row contents. If we don't need it, we should omit as it has performance implications.

I think this is a reminder from my early attempts to construct the query which will gather all the host specific information. At that time I believed it was required. I removed it now and the tests pass so it should be fine without it.

Excellent.


Now to the specifics:


src/lib/dhcp/libdhcp++.cc

LibDHCP::optionSpaceToVendorId(), you could combine the
content checks and range checks into single expressions, and like so:

    // Minimal content is "vendor-X" format 
    if ((option_space.size() < 8) || (option_space.substr(0,7) != "vendor-")) {
        return (0);
    }

and

    if ((check < 0) || (check > std::numeric_limits<uint32_t>::max())) {
        return (0);
    }

and reduce the scope of std::string x by moving it's declaration inside the the try block.

Just sayin ...

Made this change.


MySqlHostDataSourceImpl?;;checkError - We need to consider what we want to do about fatal DB errors here (see MySqlLeaseMgr::checkError()). If the server cannot talk to it's host datasource, what should it do? If it is the same source as the lease data source, then the latter should fail, but when they are different you could lose one but not the other.

We should probably discuss it more. But, I don't believe this really belong to this ticket?

If not on this ticket, it certainly needs one of it's own or find the related ticket(s) and update them to include host data sources.


MySqlHostDataSource::add()

  • line 2181 Comment says we'll call "addHost() code" but there's no such animal, just change it to Insert the host...

Changed.

  • We only set host_id if there is at least one reservation but otherwise will be zero, which means it will be wrong when inserting options for hosts with no reservations. You should simply set host_id when you declare it.

No. The host_id for options is set in !MySqlHostDataSourceImpl::addOptions. I am just trying to avoid invoking mysql_insert_id when it is not necessary.

Ok that's pretty subtle, I didn't catch it before.

It is my understanding that mysql_insert_id() returns a value that is set by the last insert or update and is kept on the client side of the session. It's akin to calling mysql_affected_rows() or mysql_errno(). Invoking this function does not result in an exchange with server. You could just always get the value after inserting the host and simplify the other methods to require a non-zero value for host_id. I think this would be a cleaner all around.

If you disagree with this, then perhaps you should make addRerv() behave as addOptions() and fetch the id if it's zero. This would make things more consistently implemented.

  • I would also be tempted to move the block of code that inserts the IPv6Rervations to after the options are done (ie. just above the commit.)

Moved.


I would prefer you renamed addQuery to addStatement Query tends to refer to questions, i.e. selects() or things which return result sets, where as statements in SQL return an count of rows affected.

Renamed.


MySqlTransaction? -

It seems a litle odd for the MySqlConnection? class to implement commit() and rollback() while starting a transaction is implemented in MySqlTransaction?. MySqlConnection?() should implement a startTransacion() method and MySqlTransaction?'s constructor should invoke that. That way the MySQL details for starting, committing, or rolling back transactions are all in the same class.

Changed as suggested.


MysqlHostDataSourceImpl? constructor

Since we are using explicit transactions we should be turning
autocommit OFF not turning it on.

Disabled by default.

GenericHostDataSourceTest::testOptionsReservations4()

In this test though you use addTestOptions() to create the host to insert, which in fact
inserts a host with both v4 and v6 options. The v6 options are (correctly) not returned by the get4 calls. The only reason tests pass is because of the order of the arguments to compareHosts(). If you reverse the order like so:

    ASSERT_NO_FATAL_FAILURE(compareHosts(*hosts_by_addr.begin(), host));

the test fails. At first read, one wonders how this test passes. On the one hand it is a verification that get4() calls exclude V6 options, on the other hand it is unclear that you intended this. You need to at least document this behavior. Alternatively, you could alter addTestOptions to accept an argument of MySqlHostWithOptionsExchange::FetchedOptions?() and only create the options you intend.
(This also applies to testOptionsReservations6())

Yes. I updated the compareHosts function to check number of options within spaces. I also added parameter which controls which options are created for host.

Also, the last line of the test is this:

    hdsptr_->get4(subnet_id, IOAddress("192.0.2.5"));

Did you mean to do something more?

No, removed.


GenericHostDataSourceTest::compareOptions()

Related to the issue above, this test does NOT check cfg1 and cfg2 have the same number of option spaces. If cfg2 is a subset of cfg1 the comparison will pass. If this intended behavior then it needs to be documented.

Added the comparison for number of options.


GenericHostDataSourceTest::initializeHost4()
GenericHostDataSourceTest::initializeHost6()

Shouldn't the increments of subnet4 and subnet6 be prefix not postfix?

Changed, although this wasn't really the code I touched.

Alas, that's never an excuse is it? ;)


Unit tests and cppcheck looked good under OS-X.

Thanks.

All other changes are fine.

Ok, I made the requested changes, including mysql_insert_id being always invoked and terminating the application when connection to MySQL database is lost, just like we do for lease database connection.

comment:10 follow-up: Changed 19 months ago by tomek

I was just looking at the schema when responding to some post to kea-dev and looked at this branch.

Do you think the changes implemented in this ticket require the schema to be updated to 4.3?

Can you correct the comments for values inserted into host_identifier_type? They still mention IA types there.

comment:11 in reply to: ↑ 10 Changed 19 months ago by marcin

Replying to tomek:

I was just looking at the schema when responding to some post to kea-dev and looked at this branch.

Do you think the changes implemented in this ticket require the schema to be updated to 4.3?

Yes, we will have to upgrade to 4.3 but that will be after we apply all changes to the schema for 1.1.0 release. So, this task is deferred until we know that we are not going to introduce any more changes.

Can you correct the comments for values inserted into host_identifier_type? They still mention IA types there.

Yes.

comment:12 Changed 19 months ago by tmark

  • Add Hours to Ticket changed from 2 to .5
  • Owner changed from tmark to marcin
  • Total Hours changed from 75 to 75.5

Marcin, the changes all look good. You are clear to merge.

comment:13 Changed 19 months ago by marcin

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

Merged with commit b8a306a27d1cae03f6bc5223c30806f5cd1b64f4

Note: See TracTickets for help on using tickets.