Opened 4 years ago

Closed 4 years ago

#4216 closed defect (complete)

MySQL Database Log IDs are used in more than one place

Reported by: tmark Owned by: tmark
Priority: medium Milestone: Kea1.0
Component: Unclassified Version: git
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

Several log IDs such as DHCPSRV_MYSQL_DB are used for both lease database code AND host storage code. Log IDs are supposed to be used in exactly one place to eliminate confusion and ambiguity. As it stands now, if one configures host storage and lease storage to MySQL the log output confusing, as it makes it appear that the lease database is closed and reopened twice.

Somehow this got past reviews. Shame on us. Here are some, may not be all:

host_data_source_factory.cc:        LOG_ERROR(dhcpsrv_logger, DHCPSRV_NOTYPE_DB).arg(dbaccess);
host_data_source_factory.cc:        LOG_INFO(dhcpsrv_logger, DHCPSRV_MYSQL_DB).arg(redacted);
host_data_source_factory.cc:        LOG_INFO(dhcpsrv_logger, DHCPSRV_PGSQL_DB).arg(redacted);
host_data_source_factory.cc:    LOG_ERROR(dhcpsrv_logger, DHCPSRV_UNKNOWN_DB).arg(parameters[type]);

Subtickets

Change History (7)

comment:1 Changed 4 years ago by tmark

  • Milestone changed from Kea-proposed to Kea1.0
  • Owner set to tmark
  • Status changed from new to assigned

comment:2 Changed 4 years ago by tmark

  • Owner changed from tmark to Unassigned
  • Status changed from assigned to reviewing

I replaced the reused messages with new messages specific to hosts database operations

comment:3 Changed 4 years ago by marcin

  • Owner changed from Unassigned to marcin

comment:4 follow-up: Changed 4 years ago by marcin

  • Owner changed from marcin to tmark

Reviewed commit 2c413e04e0d5ad8f742fa7e519232edea6a50e54

src/lib/dhcpsrv/host_data_source_factory.cc
It is odd that for some errors we log an error and then throw an exception. I suggest that we do one of those. Otherwise we'll be logging the same error twice. For example:

    if (parameters.find(type) == parameters.end()) {
        LOG_ERROR(dhcpsrv_logger, DHCPSRV_HOSTDB_NOTYPE).arg(dbaccess);
        isc_throw(InvalidParameter, "Host database configuration does not "
                  "contain the 'type' keyword");
    }

Why not simply remove LOG_ERROR and throw only?

One more place were we do it is logging of DHCPSRV_UNKNOWN_HOST_DB.

DHCPSRV_MYSQL_HOST_DB_GET_VERSION: I think it should be extended to say obtaining schema version information for hosts database?

Don't you think this requires a changelog entry, as it is a user visible change?

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

  • Owner changed from tmark to marcin

Replying to marcin:

Reviewed commit 2c413e04e0d5ad8f742fa7e519232edea6a50e54

src/lib/dhcpsrv/host_data_source_factory.cc
It is odd that for some errors we log an error and then throw an exception. I suggest that we do one of those. Otherwise we'll be logging the same error twice. For example:

    if (parameters.find(type) == parameters.end()) {
        LOG_ERROR(dhcpsrv_logger, DHCPSRV_HOSTDB_NOTYPE).arg(dbaccess);
        isc_throw(InvalidParameter, "Host database configuration does not "
                  "contain the 'type' keyword");
    }

Why not simply remove LOG_ERROR and throw only?

One more place were we do it is logging of DHCPSRV_UNKNOWN_HOST_DB.

As discusssed on jabber, I agree and have removed logging from the exception throwing conditions. I also did some minor clean up of the method itself.

DHCPSRV_MYSQL_HOST_DB_GET_VERSION: I think it should be extended to say obtaining schema version information for hosts database?

Done.

Don't you think this requires a changelog entry, as it is a user visible change?

How's this:

1xxx.   [func]      tmark
    Assigned unique log message IDs to log messages issued from
    MySQL hosts storage operations, eliminating resuse of lease
    database message IDs.
    (Trac #4206, git TBD)

comment:6 Changed 4 years ago by marcin

  • Owner changed from marcin to tmark

Reviewed commit dea9eb2c42f6cb42fac36aece6661657119f554b

Your changes as well as the changelog look fine. Please merge.

comment:7 Changed 4 years ago by tmark

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

Changes merged with git # f0e37cd6d45537971a730c68187011dce41217cc
Added ChangeLog entry 1071.

Ticket is complete.

Note: See TracTickets for help on using tickets.