Opened 7 years ago

Closed 7 years ago

#2546 closed defect (fixed)

DHCP Lease Manager - miscellaneous updates

Reported by: stephen Owned by: stephen
Priority: medium Milestone: Sprint-DHCP-20121213
Component: dhcp 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 stephen)

Implementation of the MySQL backend for DHCP V4 (ticket #2404) suggested a number of changes to LeaseMgr and related classes:

  • Remove call to access a lease by address and subnet ID.
  • Define a ClientIdPtr type.
  • Combine deleteLease4() and deleteLease6() into a single method

Subtickets

Change History (11)

comment:1 Changed 7 years ago by stephen

  • Description modified (diff)

comment:2 Changed 7 years ago by stephen

  • Milestone changed from DHCP Outstanding Tasks to Sprint-DHCP-20121213
  • Owner set to stephen
  • Status changed from new to assigned

comment:3 Changed 7 years ago by stephen

  • Status changed from assigned to accepted

comment:4 Changed 7 years ago by stephen

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

Other changes on this ticket were:

  • Correcting issues that cppcheck had noted:
    • Adding "const" to methods that don't alter the object in question
    • Correcting "possible inefficiencies" in determining whether a container is empty.
    • Fully initializing fields in a constructor.
    • Triplet::operator=() should return a reference.
  • Fixed a number of Doxygen warnings

A number of files have been changed, but in most cases the changes are minor.

comment:5 Changed 7 years ago by marcin

  • Owner changed from UnAssigned to marcin

comment:6 Changed 7 years ago by marcin

  • Owner changed from marcin to stephen

There are still some legitimate warnings being reported by cppcheck:

src/lib/dhcp/pkt4.cc: line 238: redundantCopy: Buffer 'chaddr_' is being written before its old content has been used.

src/lib/dhcp/pkt4.cc: line 260: redundantCopy: Buffer 'file_' is being written before its old content has been used.

src/lib/dhcp/pkt4.cc: line 248: redundantCopy: Buffer 'sname_' is being written before its old content has been used.

src/lib/dhcpsrv/alloc_engine.cc: line 70: Possible inefficient checking for 'pools' emptiness.

src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc: line 472: Function parameter 'leases' should be passed by reference.

I recall that the last one might have been fixed on master already.


There is also one cppcheck warning that does not seem to be valid:
src/bin/dhcp6/tests/config_parser_unittest.cc: line 100: Variable 'first' is assigned a value that is never used.

The value is used in the subsequent iterations of the loop and could be suppressed.


Other changes seem to be ok.

comment:7 Changed 7 years ago by stephen

  • Owner changed from stephen to marcin

All changed fixed in commit apart from:

There is also one cppcheck warning that does not seem to be valid:
src/bin/dhcp6/tests/config_parser_unittest.cc: line 100: Variable 'first' is assigned a value that is never used.

I guess that cppcheck does not recognise that BOOST_FOREACH encompasses a loop.

Suggested ChangeLog entry is:

xxx.	[func]		stephen
	Miscellaneous fixes to DHCP code including rationalizing some methods in
        LeaseMgr and resolving some cppcheck/Doxygen issue.
	(Trac #2546, git xxx)

comment:8 Changed 7 years ago by marcin

  • Owner changed from marcin to stephen

I verified your fixes with cppcheck and all listed issues are now gone.

There are some little things that I had not found earlier:
pkt4.cc

Also, the date in the copyright header of this file should be updated after file modification.

setHWAddr: variable name macAddr is against BIND10 coding guidelines. It should be mac_addr.

setSname: this is loosely related to your changes but appears to me that the use of const uint8_t* is unsafe. You refer to sname[0] while there is no guarantee that it sname is a valid pointer to a string.

ChangeLog
Shouldn't that be ''resolving some cppcheck/Doxygen issues '' # 's' at the end?

Also, I would categorize it as "bug fix" rather than "functional" change. In such case it should be [bug] rather than [func] in the ChangeLog entry header.

comment:9 Changed 7 years ago by stephen

  • Owner changed from stephen to marcin

Also, the date in the copyright header of this file should be updated after file modification.

Done

setHWAddr: variable name macAddr is against BIND10 coding guidelines. It should be mac_addr.

Changed.

setSname: this is loosely related to your changes but appears to me that the use of const uint8_t* is unsafe. You refer to sname[0] while there is no guarantee that it sname is a valid pointer to a string.

The problem was there already in the memcpy, but all the same, good catch. I've added a check for the pointer being null and throw an exception if so. The same check has been added to the constructor.

ChangeLog

All points valid, will alter when I write the log entry.

comment:10 Changed 7 years ago by marcin

  • Owner changed from marcin to stephen

All ok. Please merge.

comment:11 Changed 7 years ago by stephen

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

Merged in commit 0140368ed066c722e5d11d7f9cf1c01462cf7e13

Note: See TracTickets for help on using tickets.