Opened 8 years ago

Closed 8 years ago

#1265 closed task (complete)

DHCP benchmarking utility: send/receive DHCP packets

Reported by: stephen Owned by: johnd
Priority: medium Milestone: Sprint-DHCP-20111221
Component: perfdhcp 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

Code (+ tests) to send and receive DHCP packets.

As the idea of the benchmarking utility is that it behaves like a relay agent, it can use simple socket I/O instead of needing the complexity to handle broadcasts.

Subtickets

Change History (7)

comment:1 Changed 8 years ago by tomek

  • Component changed from dhcp to perfdhcp

comment:2 Changed 8 years ago by johnd

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

comment:3 Changed 8 years ago by johnd

  • Owner changed from johnd to stephen
  • Status changed from assigned to reviewing

Added code for network IO and tests

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

  • Owner changed from stephen to johnd

Reviewed commit 809dc75718908b39668046c2d3db8bfb51947ecc

General
Once #1324 is committed, you should merge the master branch into this branch to pick up the final version of the command line processor.

Question about the name of the files perfdhcp.* - "perfdhcp" is the name of the utility. These functions are solely related to the I/O. Unless everything else (e.g. as statistics accumulation and output) is going to go into this file - which would be a bad idea - I suggest the files be renamed to something like dhcp_io.*

tests/tools/perfdhcp/perfdhcp.h
Some argument names have the same format (lowercase first word, Capitalised second word) as the function names. Others are lower-case words separated by underscores. They should be consistent and be lowercase. There is no firm guide but typically full words are separated by underscores (e.g. send_port) whereas others - usually if they include abbreviations - may be concatendated together (e.g. msgsize).

The same rules for argument names apply to local variables (i.e. lower-case).

In the function headers, remove the "globals" section - that is a detail of the implementation and not of interest to any caller of the function. (Also, they aren't global variables - they have file-limited scope).

Remember that you need to remove the #define when this is merged with the code in #1324.

It would be worth learning Doxygen to comment your headers. Only a few tags are needed e.g.

/**
 * Send a DHCP packet.
 *
 * \param dhcp_packet DHCP message to send.
 * \param num_octets Size of message.
 *
 * \return 1 on success.  If the send fails (in full or part), an error
 *         message is printed to stderr and 0 is returned.
 */

Typo in header of dhcpReceive - "reeive".

tests/tools/perfdhcp/perfdhcp.cc
This should be a .c file.

Would prefer that the declarations of the static variables are one per line.

If you put the main dhcp* functions after the static helper functions in the file, you don't need to declare the latter.

All error messages include the program name and output text of the form

<program-name>: <error-message>

Why not create a "reportError()" function (in a separate module, with its own header file) that eliminates the need to have PROGNAME scattered all over the place, e.g.

void reportError(const char* format, ...) {
    va_list ap;

    va_start(ap, format);
    fprintf(stderr, "%s: ", PROGNAME);
    vfprintf(stderr, format, ap);
    fprintf(stderr, "\n");
    va_end(ap);
}

If there is a function header in the .h file, there is no need to repeat it in the .c file. A short one or two-line summary of what it does will be enough.

dhcpSetup(): The variable v6 is declared as equal to the result of isV6(). It is used in the next line then ignored as the following test uses isV6() directly. It might as well be removed.

dhcpSetup(): What happens if "isV6()" is true and localAddr is null? NULL is then passed to socketSetup - is this OK? (If so, a comment is needed to explain why.)

dhcpReceive(): The "if" test for msgsize equal to zero should use braces, even though there is a single statement.

socketSetup(): If the address family is V6 and the local address is null, it is not clear what the resulting memcpy and assignments are going - a comment would be helpful.

socketSetup(): at the point where a diagnostic message is printed ("creating socket from addrinfo"), the address information is printed twice.

getAddrs header: strictly speaking, PROGNAME is not a global, it's a defined symbol here.

getAdxdrs header: can "hostname" also be an IPV6 address?

getAddrs(): why not call it getAddress() - it's only two characters more.

tests/tools/perfdhcp/tests/sendreceive_unittest.cc
I would not use "do" as the name of a test (it is a reserved word).

Add a comment to the effect that "procArgs" is called to set up the desired options.

There should be two tests - one for IPv4 and one for IPv6.

The receive buffer should be much larger than the message sent. recvfrom() can truncate a returned message to fit the buffer; if the buffer size is equal to the size of the message sent, you don't know whether some additional data has been sent but was then truncated.

Last edited 8 years ago by stephen (previous) (diff)

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

  • Owner changed from johnd to stephen

Replying to stephen:

Reviewed commit 809dc75718908b39668046c2d3db8bfb51947ecc

Remember that you need to remove the #define when this is merged with the code in #1324.

I made PROGNAME a global again, since it's used in more than one module.

It would be worth learning Doxygen to comment your headers. Only a few tags are needed e.g.

I'll do that.

comment:6 Changed 8 years ago by stephen

  • Owner changed from stephen to johnd

Reviewed commit b12941d14b2b30fedb0543c33c551b780ed6340b

I made PROGNAME a global again, since it's used in more than one module.

It could just be static const char* PROGNAME = "perfdhcp"; in the header - it would save the extern declaration and the chasing through the modules to find out where it is declared. The odd few bytes wasted by multiple declarations are neither here nor there.

tests/tools/perfdhcp/misc.h
The name of the file is a bit generic, but let's see what else goes in there before changing it.

reporterr() needs a function header.

tests/tools/perfdhcp/netio.c

If you put the main dhcp* functions after the static helper functions in the file, you don't need to declare the latter.

Any reason not to do this?

socketSetup(): True that local_addr can only be null if the address family is IPV6, this is something that could perhaps be checked by an assertion (or a test that aborts with an error message). The function is only called once and it would minimise the possibility of programming errors.

addrPortToNames(): What is the "TODO" about - what is gai_strerror()?

netShutdown(): This shuts down the connection started by dhcpSetup(); dhcpShutdown() would be a better name for it.

tests/tools/perfdhcp/tests/sendreceive_unittest.c
The code in the V4 and V6 test is identical apart from the message and the command line. The common code could be extracted into a separate function. To identify which test produces the errors, use the SCOPED_TRACE macro, with the message being used for the test as the argument, e.g.

static void common_test(const char* argv[], int argvsize, const char* message) {
    SCOPED_TRACE(message);
           :
}

TEST(SendReceiveTest, v4) {
    const char* argv [] = {"perfdhcp", "127.0.0.1"};
    common_test(argv, (sizeof(argv) / sizeof(char*)), "This is the V4 test");
}

TEST(SendReceiveTest, v6) {
    const char* argv [] = {"perfdhcp", "-6", "::1"};
    common_test(argv, (sizeof(argv) / sizeof(char*)), "This is the V6 test");
}

(Don't forget to replace sizeof(message) with strlen(message) in the code in common_test.)

numOctets should be num_octets (variable naming).

comment:7 Changed 8 years ago by stephen

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

This task has been subsumed into #1450

Note: See TracTickets for help on using tickets.