[trac703] Update document and comments

src/lib/asiolink/io_fetch.h

@@ -116,22 +116,19 @@ public:
     ///
     /// Creates the object that will handle the upstream fetch.
     ///
-    /// TODO: Need to randomise the source port
-    ///
     /// \param protocol Fetch protocol, either IOFetch::TCP or IOFetch::UDP
     /// \param service I/O Service object to handle the asynchronous
-    ///     operations.
+    ///        operations.
     /// \param question DNS question to send to the upstream server.
-    /// \param buff Output buffer into which the response (in wire format)
-    ///     is written (if a response is received).
-    /// \param cb Callback object containing the callback to be called
-    ///     when we terminate.  The caller is responsible for managing this
-    ///     object and deleting it if necessary.
     /// \param address IP address of upstream server
     /// \param port Port to which to connect on the upstream server
-    /// (default = 53)
+    /// \param buff Output buffer into which the response (in wire format)
+    ///        is written (if a response is received).
+    /// \param cb Callback object containing the callback to be called when we
+    ///        terminate.  The caller is responsible for managing this object
+    ///        and deleting it if necessary.
     /// \param wait Timeout for the fetch (in ms).  The default value of
-    ///     -1 indicates no timeout.
+    ///     -  1 indicates no timeout.
     IOFetch(Protocol protocol, IOService& service,
         const isc::dns::Question& question, const IOAddress& address,
         uint16_t port, isc::dns::OutputBufferPtr& buff, Callback* cb,
@@ -141,8 +138,6 @@ public:
     ///
     /// Creates the object that will handle the upstream fetch.
     ///
-    /// TODO: Need to randomise the source port
-    ///
     /// \param protocol Fetch protocol, either IOFetch::TCP or IOFetch::UDP
     /// \param service I/O Service object to handle the asynchronous
     ///     operations.

tests/tools/badpacket/README

@@ -1,12 +1,11 @@
 "badpacket" is a tool intended to test that a nameserver can cope with
 incorrectly-formatted DNS messages.
 
-This particular incarnation of the tool allows the flags field of a DNS message
-(the third and fourth bytes) to be set to any bit combination (including ones
-that invalid in a query).  As well as setting the bits to a particular
-combination, it is possible to specify ranges for bit/field values; when this
-is done, the tool will send a set of packets so that each combination of flag
-bits is checked.
+This particular incarnation of the tool allows various fields in the DNS
+message to be set to any value (including to values that invalidate the query).
+As well as setting individual values, it is possible to specify ranges; when
+this is done, the tool will send a set of packets so that each combination
+of values is checked.
 
 To illustrate this, consider the following command:
 
@@ -15,11 +14,11 @@ badpacket --address 192.0.2.21 --port 5301 --aa 0-1 --cd 1
 
 (The command has been split across two lines for clarity.)
 
-The address and port flags are self-evident.  The other flags specify settings
-for the AA bit (0 and 1), CD bit (always 1) and the RCODE field (0, 1, 2). (The
-remaining fields are not specified, so will always be zero.)  There are six
-combinations of these values, so six packets will sent to the remote server with
-the following settings:
+The address and port flags are self-evident.  The other flags specify
+settings for the AA bit (0 and 1), CD bit (always 1) and the RCODE field
+(0, 1, 2). (The remaining fields in the flags word are not specified, so
+will always be zero.)  There are six combinations of these values, so six
+packets will sent to the remote server with the following settings:
 
 AA RCODE  CD Rest
 0    0    1   0
@@ -32,20 +31,23 @@ AA RCODE  CD Rest
 Each packet will cause a line to be output to stdout, which will have the
 following form:
 
-SUCCESS: (QR:0 OP:0 AA:0 TC:0 RD:0 RA:0 Z:0 AD:0 CD:1 RC:0)
-         (qr:1 op:0 aa:0 tc:0 rd:0 ra:1 z:0 ad:0 cd:1 rc:0)
+SUCCESS: (QR:0 OP:0 AA:0 TC:0 RD:0 RA:0 ...
+         (qr:1 op:0 aa:0 tc:0 rd:0 ra:1 ...
 
-(Again the text has been split across two lines for clarity.)
+(Again the text has been split across two lines for clarity. Also, the full
+list of fields has been truncated.)
 
 Each lines contains a status (SUCCESS indicates that a response was received,
 regardless of the contents of the response), the state of the fields in the
 flags word of the packet sent (in upper-case letters) and the state of the
 fields in the flags word of the response (in lower-case letters).
 
-TODO: At the moment the tool is limited to just alerting the flags field.
-Future work should extend the program to other bad packets. Ideas are:
+TODO: At the moment the tool is limited to:
+* Setting values in the flags field.
+* Setting section counts to arbitrary values.
+* Extending or truncating the DNS message size.
 
-* Flasify the values in the various count fields
+Additional options needed are:
 * Add data to sections that should be empty.
 * Deliberately mangle the names placed in the message sections (e.g. by altering
-  the label count fields).
+  the label count fields).

tests/tools/badpacket/badpacket.cc

@@ -27,16 +27,18 @@
 /// printing the packet settings and the response received.  The principle
 /// raison d'etre for this is to check if a bad packet will cause a crash.
 ///
-/// This particular version of the code allows a set of ranges to be set for
-/// each field in the "flags" word (the third and fourth bytes) of a DNS
-/// message. (Most of the flags are single-bit values, so the range is just 0-1.
-/// The OPCODE and RCODE are both four bits wide, so the range is 0-15.)  The
-/// program then sends packets containing each combination of values.
+/// This particular version of the code allows a set of ranges to be set for:
+/// - The flags fields in the message.
+/// - The section count fields, regardless of what is in the section.
+/// - The message size: the message can be truncated or extended.
 ///
-/// TODO: Extend the program to other bad values.
-/// Examples of this would be to make the count fields invalid, to add data
-/// to sections that should be empty, and to deliberately mangle the names in
-/// these sections.
+/// The program then sends a set of packets corresponding to all combinations
+/// of values in the ranges selected.
+
+// TODO: Extend to cover:
+// - Mangling the QNAME
+// - Adding names to the message sections
+// - Adding EDNS0 RR and magling the fields there
 
 using namespace isc::badpacket;
 
@@ -44,10 +46,11 @@ using namespace isc::badpacket;
 int main(int argc, char* argv[]) {
 
     try {
+        // Parse command
         CommandOptions options;
         options.parse(argc, argv);
 
-        // Construct the scan object and perform the scan.
+        // Send the sequence of messages
         Scan scanner;
         scanner.scan(options);
     } catch (isc::Exception& e) {

tests/tools/badpacket/command_options.cc

@@ -33,23 +33,39 @@ using namespace isc;
 namespace isc {
 namespace badpacket {
 
-/// Parses the command-line options and returns the results in an Options
-/// structure.  If the 
+// Reset stored values to the defaults.
+void
+CommandOptions::reset() {
+    address_ = "127.0.0.1";
+    port_ = 53;
+    timeout_ = 500;
+    qname_ = "www.example.com";
+
+    for (int i = 0; i < OptionInfo::SIZE; ++i) {
+        options_[i].minimum = OptionInfo::defval(i);
+        options_[i].maximum = OptionInfo::defval(i);
+        options_[i].present = false;
+    }
+}
+
+/// Parses the command-line options and records the results.
 void
 CommandOptions::parse(int argc, char* const argv[]) {
 
     // Set up options for processing.  The declaration of the string constants
-    // as mutable arrays and putting the string variable into the "option"
+    // as mutable arrays and putting the string variable into the "longopts"
     // structure (as opposed to just putting a string literal there) is
-    // occasioned by a version of solaris which declares the first field
-    // as "char*" (instead of the correct "const char*").
+    // occasioned by a version of solaris which declares the first field as
+    // "char*" (instead of the correct "const char*").
+
+    // General options.
     char HELP[] = {"help"};
     char VERSION[] = {"version"};
     char ADDRESS[] = {"address"};
     char PORT[] = {"port"};
     char TIMEOUT[] = {"timeout"};
 
-    // Settings for options in the flags field
+    // Settings for options in the flags field.
     char QR[] = {"qr"};
     char OP[] = {"op"};
     char AA[] = {"aa"};
@@ -96,7 +112,7 @@ CommandOptions::parse(int argc, char* const argv[]) {
     const char* shortopts = "hva:p:t:Q:O:A:T:D:R:Z:U:C:E:Y:W:H:I:M:";
 
 
-    // Set variables to defaults before parsing
+    // Set record of options to defaults before parsing
     reset();
 
     // Process command line
@@ -148,13 +164,18 @@ CommandOptions::parse(int argc, char* const argv[]) {
         }
     }
 
-    // Pick up a parameter if there is one (and ignore excess parameters).
+    // Pick up a parameter if there is one (and report excess).
     if (optind < argc) {
         qname_ = argv[optind++];
     }
+
+    if (optind < argc) {
+        isc_throw(isc::InvalidParameter,
+                  "only a single (optional) parameter may be specified on the command line");
+    }
 }
 
-/// \brief Print usage information
+// Print usage information.
 void
 CommandOptions::usage() {
     cout << "Usage: badpacket [options] query\n"
@@ -226,23 +247,23 @@ CommandOptions::usage() {
             ;
 }
 
-/// \brief Print version information
+// Print version information,
 void
 CommandOptions::version() {
     cout << BADPACKET_VERSION << "\n";
 }
 
-// Process single flag
+// Process single flag that can be stored in the options_ member.
 void
 CommandOptions::processOptionValue(int c, const char* value) {
 
-    // Get values for this option
+    // Get values for this option.
     int index = OptionInfo::getIndex(c);
     const char* name = OptionInfo::name(index);
     uint32_t minval = OptionInfo::minval(index);
     uint32_t maxval = OptionInfo::maxval(index);
 
-    // Split the string up into one or two tokens
+    // Split the string up into one or two tokens.
     vector<string> tokens = isc::strutil::tokens(string(value), "-");
     if ((tokens.size() < 1) || (tokens.size() > 2)) {
         isc_throw(isc::BadValue, "value given for " << name << " is '" << value <<
@@ -267,7 +288,7 @@ CommandOptions::processOptionValue(int c, const char* value) {
         swap(options_[index].minimum, options_[index].maximum);
     }
 
-    // Check that tokens lie inside the allowed ranges
+    // Check that tokens lie inside the allowed ranges.
     if ((tokens.size() == 2) &&
         ((options_[index].minimum < OptionInfo::minval(index)) || (options_[index].maximum > maxval))) {
         isc_throw(isc::BadValue, "the value of " << options_[index].minimum <<
@@ -287,7 +308,9 @@ CommandOptions::processOptionValue(int c, const char* value) {
     options_[index].present = true;
 }
 
-// Minimum and maximum value of the flag
+// Return information about the option - minimum and maximum values and whether
+// it was actually specified. (Note that if it wasn't, the maximum and minimum
+// values will have been set to the default recorded in the OptionInfo class.)
 uint32_t
 CommandOptions::minimum(int index) const {
     OptionInfo::checkIndex(index);

tests/tools/badpacket/command_options.h

@@ -29,24 +29,25 @@ namespace badpacket {
 /// This class is responsible for parsing the command-line and storing the
 /// specified options.
 ///
-/// Each option setting the state of one of the fields in the flags word in the
-/// DNS packet can be specified as either:
+/// Some of the options perform general control (like setting the address of the
+/// nameserver under test, while the rest set values in the DNS message being
+/// sent.  Each of the latter options can be specified as either:
 ///
 /// - \c --option value
 /// - \c --option value1-value2
 ///
 /// Either way, two values are extracted the low value and the high value (in
-/// the former case, both are the same).  The values are stored in an array
-/// and can be returned on request.
+/// the former case, both are the same).  The values are stored and can be
+/// returned on request.
 ///
-/// For simplicity, the class also takes care of the --help and --version flags,
+/// For simplicity, the class takes care of the --help and --version flags,
 /// each of which will cause a message to be printed to stdout and the program
 /// to terminate.
 
 class CommandOptions {
 public:
 
-    /// \brief CommandOptions Constructor
+    /// \brief Default Constructor
     ///
     /// Set values to defaults.
     CommandOptions() {
@@ -55,6 +56,12 @@ public:
 
     /// \brief Return minimum value for option
     ///
+    /// Applicable only to an option affecting a field in the message, this
+    /// method returns the minimum value that was given on the command line.
+    /// (If only a single value was given, it will be that value returned.)
+    /// If the option was not specified on the command line, the default value
+    /// set in the OptionsInfo class will be returned.
+    ///
     /// \param index Index of the command-line option.
     ///
     /// \return uint32_t holding the minimum value given (or the default if
@@ -63,13 +70,19 @@ public:
 
     /// \brief Return maximum value for option
     ///
+    /// Applicable only to an option affecting a field in the message, this
+    /// method returns the maximum value that was given on the command line.
+    /// (If only a single value was given, it will be that value returned.)
+    /// If the option was not specified on the command line, the default value
+    /// set in the OptionsInfo class will be returned.
+    ///
     /// \param index Index of the command-line option.
     ///
     /// \return uint32_t holding the maximum value given (or the default if
     ///         the option was not specified on the command line).
     uint32_t maximum(int index) const;
 
-    /// \brief Return if option was given on command line
+    /// \brief Reports if option was given on command line
     ///
     /// \param index Index of the command-line option.
     ///
@@ -77,17 +90,17 @@ public:
     bool present(int index) const;
 
 
-    /// \brief Return Target Address
+    /// \brief Return target address
     std::string getAddress() const {
         return address_;
     }
 
-    /// \brief Return Target Port
+    /// \brief Return target port
     uint16_t getPort() const {
         return port_;
     }
 
-    /// \brief Return Timeout
+    /// \brief Return timeout
     int getTimeout() const {
         return timeout_;
     }
@@ -97,21 +110,12 @@ public:
         return qname_;
     }
 
-    /// \brief Reset To Defaults
-    void reset() {
-        address_ = "127.0.0.1";
-        port_ = 53;
-        timeout_ = 500;
-        qname_ = "www.example.com";
-
-        for (int i = 0; i < OptionInfo::SIZE; ++i) {
-            options_[i].minimum = OptionInfo::defval(i);
-            options_[i].maximum = OptionInfo::defval(i);
-            options_[i].present = false;
-        }
-    }
+    /// \brief Reset to defaults
+    ///
+    /// Resets the CommandOptions object to default values.
+    void reset();
 
-    /// \brief Parse Command Line
+    /// \brief Parse command line
     ///
     /// Parses the command line and stores the selected options.  The parsing
     /// also handles the --help and --version commands: both of these will cause
@@ -122,15 +126,13 @@ public:
     /// \param argv Argument value array passed to main().
     void parse(int argc, char* const argv[]);
 
-    /// \brief Print Usage Information And Exit Program
+    /// \brief Print usage information and exit program
     void usage();
 
-    /// \brief Print Version Information And Exit Program
+    /// \brief Print version information and exit program
     void version();
 
-    // The following are protected to aid testing
-
-protected:
+private:
     /// \brief Process Option Value
     ///
     /// Processes a specific command-line option, interpreting the value and
@@ -143,13 +145,11 @@ protected:
 
     // Member variables
 
-private:
     struct {
         uint32_t    minimum;        ///< Minimum value specified
         uint32_t    maximum;        ///< Maximum value specified
         bool        present;        ///< true if specified on command line
     } options_[OptionInfo::SIZE];   ///< Information about command options
-                                    ///< Value of options (minimum and maximum)
     std::string     address_;       ///< Address to where query is sent
     uint16_t        port_;          ///< Target port
     int             timeout_;       ///< Timeout for query

tests/tools/badpacket/header_flags.h

@@ -34,19 +34,19 @@ public:
         reset();
     }
 
-    /// \brief Reset Values to Zero
+    /// \brief Reset values to zero
     ///
     /// Clears all flags.
     void reset() {
         setValue(0);
     }
 
-    /// \brief Get Header Flags as 16-bit Value
+    /// \brief Get header flags as 16-bit value
     uint16_t getValue() const {
         return (flags_);
     }
 
-    /// \brief Set Header Flags as 16-Bit Value
+    /// \brief Set header flags as 16-bit value
     ///
     /// \param value 16-bit value to put into object as representing the
     ///        header flags.
@@ -54,7 +54,7 @@ public:
         flags_ = value;
     }
 
-    /// \brief Get Field
+    /// \brief Get field
     ///
     /// Return the value of a bit field in the flags word.
     ///
@@ -66,18 +66,18 @@ public:
         return ((flags_ & OptionInfo::mask(index)) >> OptionInfo::offset(index));
     }
 
-    /// \brief Set Field
+    /// \brief Set field
     ///
     /// Sets the value of a bit field.
     ///
     /// \param int Index of the bit field in the OptionInfo data structure
     /// \param value Value to set.  If the value is more than the field can
-    ///        hold, it is set to the maximum.
+    ///        hold, a BadValue exception is thrown.
     void set(int index, uint16_t value) {
-        // Declare an OptionInfo object for brevity
-        OptionInfo o;
 
-        // Ensure in range
+        // Declare an OptionInfo object for brevity and check the index is
+        // valid.
+        OptionInfo o;
         o.checkIndex(index);
 
         // Ensure the value is within limits and throw an exception if not. (This

tests/tools/badpacket/option_info.cc

@@ -59,7 +59,7 @@ namespace badpacket {
 int
 OptionInfo::getIndex(int c) {
     for (int i = 0; i < SIZE; ++i) {
-        if (c == option_information[i].short_form) {
+        if (option_information[i].short_form == c) {
             return (i);
         }
     }

tests/tools/badpacket/option_info.h

@@ -27,20 +27,22 @@ namespace badpacket {
 /// that require values and which control data put in the DNS message sent to
 /// the remote system.
 ///
-/// Currently all of these options correspond to fields in the flags word of the
-/// DNS message header, so the information includes details about the position
-/// of the fields and an appropriate bit mask.
+/// Some of the fields are no applicable to all options.  For example, some of
+/// the options correspond to fields in the flags word of the DNS message
+/// header, so the information includes details about the position of the fields
+/// and an appropriate bit mask.
 ///
 /// Note that the class does not hold values specified on the command line - it
-/// only holds information about command-line options.
+/// only holds information about the available options.
 
 class OptionInfo {
 public:
 
     /// \brief Array Indexes
     ///
-    /// The data for the flags options are held in an array.  Although an enum,
-    /// only the numeric values are used - they are indexes into arrays.
+    /// The data for the flags options are held in an array.  Although declared
+    /// as an enum, only the numeric values are used as they are the indexes
+    /// into the array.
     enum Index {
         FLAGS_START = 0,    // Start of flags field codes
         QR = 0,             // Query/response
@@ -66,15 +68,15 @@ public:
         SIZE = 15           // Number of index values
     };
 
-    /// \brief Option Parameters
+    /// \brief Option parameters
     ///
     /// Defines a structure that holds information associated with each of the
-    /// flags field command options.  Note all members of the structure are
-    /// relevant to all options
+    /// flags field command options.  Not all members of the structure are
+    /// relevant to all options.
     struct Parameter {
         const char      short_form;     // Short form of the command switch
         const char*     long_form;      // Long form of the command switch
-        int             word;           // Byte offset of word in the header
+        int             word;           // Byte offset of word in message header
         uint16_t        mask;           // Bit mask of field in the flags word
         int             offset;         // Offset of field in flags word
         uint32_t        defval;         // Default value
@@ -84,14 +86,15 @@ public:
 
     /// \brief Return index for command option
     ///
-    /// Given the short form of a switch, return the index into the options
-    /// array.
+    /// Given the short form of a command-line option, return the index in the
+    /// options array corresponding to that option.
     ///
     /// \param c The character that is the short form of the command line option.
-    ///        An 'int' is used as the value passed will be the return vaue from
-    ///        'getopt()' (or equivalent) which is an int.
+    ///        An 'int' is used as the value passed will be the return value
+    ///        from 'getopt()' (or equivalent) which is an int.  If the
+    ///        character is not found, an exception will be thrown.
     ///
-    /// \return A valid index value (else an exception is thrown).
+    /// \return A valid index value.
     static int getIndex(int c);
 
     /// \brief Return long form of command switch for this field
@@ -108,7 +111,7 @@ public:
     ///
     /// \param index A valid index (one of the values in the 'Index' enum).
     ///
-    /// \return The offset in the header foe this datum.
+    /// \return The offset in the header for this datum.
     static int word(int index);
 
     /// \brief Return mask associated with switch field
@@ -116,32 +119,34 @@ public:
     /// \param index A valid index (one of the values in the 'Index' enum).
     ///
     /// \return The mask for this particular option in the DNS message flags
-    ///         field.
+    ///         word.  The returned value is only valid for options that
+    ///         correspond to fields in the flags word.
     static uint16_t mask(int index);
 
-    /// \brief Return offset associated with switch field
+    /// \brief Return offset associated with option field
     ///
     /// \param index A valid index (one of the values in the 'Index' enum).
     ///
     /// \return The offset of the field corresponding to this option in the DNS
-    ///         message flags field.
+    ///         message flags field.  The returned value is only valid for
+    ///         options that correpond to fields in the flags word.
     static int offset(int index);
 
-    /// \brief Return minimum allowed value of field
+    /// \brief Return minimum allowed value of an option
     ///
     /// \param index A valid index (one of the values in the 'Index' enum).
     ///
-    /// \return Minimum allowed value for this option.  This is usually 0.
+    /// \return Minimum allowed value for this option.
     static uint32_t minval(int index);
 
-    /// \brief Return default value of a field
+    /// \brief Return default value of an option
     ///
     /// \param index A valid index (one of the values in the 'Index' enum).
     ///
     /// \return Default value for this option
     static uint32_t defval(int index);
 
-    /// \brief Return maximum allowed value of field
+    /// \brief Return maximum allowed value of an option
     ///
     /// \param index A valid index (one of the values in the 'Index' enum).
     ///
@@ -155,7 +160,6 @@ public:
     /// correspond to one of the valid indexes in the 'Index' enum.
     ///
     /// \param index An index value.
-    ///
     static void checkIndex(int i) {
         if ((i < 0) || (i >= SIZE)) {
             isc_throw(isc::OutOfRange, "option index must be in the range "

tests/tools/badpacket/scan.cc

@@ -99,7 +99,9 @@ Scan::iterateFlags(OutputBufferPtr& msgbuf, const CommandOptions& options,
     } else {
 
         // Index is not valid, so we have recursed enough to process all the
-        // flags fields.  Set the value in the message header and recurse.
+        // flags fields.  Set the value in the message header and call the next
+        // stage in the processing.
+        //
         // (In the next statement, the "word" offset of in the header is the
         // same for all flags options, so the value for an arbitrary field
         // (QR) has been used.)
@@ -140,8 +142,8 @@ Scan::iterateCount(OutputBufferPtr& msgbuf, const CommandOptions& options,
             }
         } else {
 
-            // Not specified on command line, so leave the default value and
-            // and process the next index.
+            // Not specified on command line, so do change anything and process
+            // the next index.
             iterateCount(msgbuf, options, (index + 1), maxindex);
         }
     } else {
@@ -152,7 +154,7 @@ Scan::iterateCount(OutputBufferPtr& msgbuf, const CommandOptions& options,
     }
 }
 
-// Size the message
+// Alter the message size.
 void
 Scan::sizeMessage(OutputBufferPtr& msgbuf, const CommandOptions& options) {
 
@@ -185,9 +187,8 @@ Scan::sizeMessage(OutputBufferPtr& msgbuf, const CommandOptions& options) {
 // Perform the message exchange for a single combination command options.
 void
 Scan::scanOne(isc::dns::OutputBufferPtr& msgbuf, const CommandOptions& options) {
-
-    // Store the interpretation of the flags field.
-    string fields = getFields(msgbuf);
+    // Store the interpretation of outgoing message.
+    string fields = string("(") + getFields(msgbuf) + string(")");
 
     // Do the I/O, waiting for a reply
     OutputBufferPtr replybuf(new OutputBuffer(512));
@@ -201,7 +202,7 @@ Scan::scanOne(isc::dns::OutputBufferPtr& msgbuf, const CommandOptions& options)
             status = "SUCCESS";
 
             // Parse the reply and get the fields
-            returned = getFields(replybuf);
+            returned = string("(") + getFields(replybuf) + string(")");
             lowercase(returned);
         }
         break;
@@ -219,18 +220,17 @@ Scan::scanOne(isc::dns::OutputBufferPtr& msgbuf, const CommandOptions& options)
     }
 
     // ... and output the result
-    cout << status << ": (" << fields << ") (" << returned << ")\n";
+    cout << status << ": " << fields << " " << returned << "\n";
 }
 
 // Get interpretation of the message fields.
-//
-// This takes the second and third bytes of the passed buffer and interprets
-// the values.  A summary string listing them is returned.
 std::string
 Scan::getFields(isc::dns::OutputBufferPtr& msgbuf) {
-    HeaderFlags flags;
 
-    // Extract the flags field from the buffer
+    // Header flags. (Note that all come from the same word in the message, so
+    // using the word offset for the QR flag as the position in the buffer from
+    // which to extract the values is valid.)
+    HeaderFlags flags;
     InputBuffer inbuf(msgbuf->getData(), msgbuf->getLength());
     inbuf.setPosition(OptionInfo::word(OptionInfo::QR));
     flags.setValue(inbuf.readUint16());
@@ -248,8 +248,7 @@ Scan::getFields(isc::dns::OutputBufferPtr& msgbuf) {
         " CD:" << flags.get(OptionInfo::CD) <<
         " RC:" << flags.get(OptionInfo::RC);
 
-    // Section count fields
-
+    // Section count fields.
     inbuf.setPosition(OptionInfo::word(OptionInfo::QC));
     os << std::dec << std::uppercase <<
         " QC:" << inbuf.readUint16();
@@ -266,14 +265,14 @@ Scan::getFields(isc::dns::OutputBufferPtr& msgbuf) {
     os << std::dec << std::uppercase <<
         " DC:" << inbuf.readUint16();
 
-    // ... and message size
+    // ... and message size.
     os << std::dec << std::uppercase <<
         " MS:" << msgbuf->getLength();
 
     return (os.str());
 }
 
-// Perform the I/O.
+// Perform the I/O to the nameserver.
 void
 Scan::performIO(OutputBufferPtr& sendbuf, OutputBufferPtr& recvbuf,
                 const CommandOptions& options)
@@ -293,7 +292,7 @@ Scan::performIO(OutputBufferPtr& sendbuf, OutputBufferPtr& recvbuf,
     service_->run();
 }
 
-// I/O Callback.  Called when the message exchange compltes or times out.
+// I/O Callback.  Called when the message exchange completes or times out.
 void
 Scan::operator()(IOFetch::Result result) {
 
@@ -305,7 +304,5 @@ Scan::operator()(IOFetch::Result result) {
     service_->stop();
 }
 
-
-
 } // namespace test
 } // namespace isc

tests/tools/badpacket/scan.h

@@ -33,10 +33,9 @@ namespace badpacket {
 
 /// \brief Field Scan
 ///
-/// This class implements a field scan.  Given a CommandOptions object, it
-/// will cycle through combinations of the given options, sending and
-/// receiving packets.  For each packet exchange, a summary is written to
-/// stdout.
+/// This class implements a field scan.  Given a CommandOptions object, it will
+/// cycle through combinations of the given options, sending and receiving
+/// messages. For each packet exchange, a summary is written to stdout.
 
 class Scan : public asiolink::IOFetch::Callback {
 public:
@@ -47,6 +46,8 @@ public:
 
     /// \brief Run Scan
     ///
+    /// Actually performs the scan for the combination of options.
+    ///
     /// \param options Command-line options
     void scan(const CommandOptions& options);
 
@@ -150,7 +151,7 @@ private:
 
     /// \brief Scan One Value
     ///
-    /// Performs one exchange of packets with the remote nameserver, sending
+    /// Performs one exchange of messages with the remote nameserver, sending
     /// the specified message.
     ///
     /// \param msgbuf Message that will be sent to the remote nameserver.  The
@@ -175,7 +176,7 @@ private:
 
     /// \brief Get Fields
     ///
-    /// Interprets the flags fields in a DNS message and converts them to a
+    /// Interprets the fields in a DNS message and converts them to a brief
     /// textual format.
     ///
     /// \param msg Message for which the header is value

tests/tools/badpacket/tests/command_options_unittest.cc

@@ -50,7 +50,7 @@ public:
     /// \brief Checks the minimum and maximum value specified for an option
     ///
     /// Checks the values for one of the options whose values are stored in the
-    /// class's options_) array.
+    /// class's options_ array.
     ///
     /// \param index Index of the option in the limits_ array
     /// \param minval Expected minimum value
@@ -62,8 +62,8 @@ public:
 
     /// \brief Checks that all options are at default values
     ///
-    /// Checks that all options have both their maximum and minimum set
-    /// to the default values.
+    /// Checks that all options have both their maximum and minimum set to the
+    /// default values.
     ///
     /// \param except Index not to check. (This allows options not being tested
     ///        to be checked to see that they are at the default value.)  As all
@@ -100,7 +100,7 @@ public:
 
         // Check the results.  Everything should be at the defaults except for
         // the specified option, where the minimum and maximum should be as
-        // given.
+        // specified.
         checkDefaultOtherValues();
         checkDefaultLimitsValues(index);
         checkValuePair(index, minval, maxval);
@@ -108,7 +108,7 @@ public:
 
     /// \brief Check invalid command option
     ///
-    /// Passed a command with an invalid value, which that the parsing throws
+    /// Passed a command with an invalid value, checks that the parsing throws
     /// a BadValue exception.
     ///
     /// \param optflag Option flag (in the form '--option')
@@ -177,7 +177,7 @@ public:
     }
 };
 
-// Check that each of the options will be recognised
+// Check that each of the non-message options will be recognised
 
 TEST_F(CommandOptionsTest, address) {
     const char* argv[] = {"badpacket",  "--address", "192.0.2.1"};
@@ -231,7 +231,8 @@ TEST_F(CommandOptionsTest, parameter) {
     checkDefaultLimitsValues();
 }
 
-// The various tests of the different flags
+// Test options representing the flags fields.
+
 TEST_F(CommandOptionsTest, qr) {
     checkOneBitField(OptionInfo::QR, "--qr");
 }

tests/tools/badpacket/tests/header_flags_unittest.cc

@@ -21,243 +21,77 @@
 
 using namespace isc::badpacket;
 
-
 // Test fixture class
 
 class HeaderFlagsTest : public ::testing::Test {
 public:
     HeaderFlagsTest() {}
-};
-
-// Convenience function to check that all values are zero
-void
-checkZero(const HeaderFlags& flags) {
-    EXPECT_EQ(0, flags.get(OptionInfo::QR));
-    EXPECT_EQ(0, flags.get(OptionInfo::OP));
-    EXPECT_EQ(0, flags.get(OptionInfo::AA));
-    EXPECT_EQ(0, flags.get(OptionInfo::TC));
-    EXPECT_EQ(0, flags.get(OptionInfo::RD));
-    EXPECT_EQ(0, flags.get(OptionInfo::RA));
-    EXPECT_EQ(0, flags.get(OptionInfo::Z));
-    EXPECT_EQ(0, flags.get(OptionInfo::AD));
-    EXPECT_EQ(0, flags.get(OptionInfo::CD));
-    EXPECT_EQ(0, flags.get(OptionInfo::RC));
 
-    EXPECT_EQ(0, flags.getValue());
-}
 
+    /// \brief Checks that all options are zero
+    ///
+    /// Checks that all flags options are set to zero.
+    ///
+    /// \param flags Flags structure to check.
+    /// \param except Index not to check. (This allows options not being tested
+    ///        to be checked to see that they are at the default value.)  As all
+    ///        index values are positive, a negative value means check
+    ///        everything.
+    void checkZero(const HeaderFlags& flags, int except = -1) {
+
+        // Check individual fields
+        for (int i = OptionInfo::FLAGS_START; i < OptionInfo::FLAGS_END; ++i) {
+            if (i != except) {
+                EXPECT_EQ(0, flags.get(i));
+            }
+        }
+
+        // ... and check the composite
+        if (except == -1) {
+            EXPECT_EQ(0, flags.getValue());
+        } else {
+            EXPECT_NE(0, flags.getValue());
+        }
+    }
+
+    /// \brief Check Option
+    ///
+    /// Checks that an option will only set the appropriate bits in the flags
+    /// field.
+    ///
+    /// \param index Index of the flags field to check.
+    /// \param maxval Maximum value of the header field.
+    void checkOption(int index, uint32_t maxval) {
+
+        // Create header flags and check initialized properly.
+        HeaderFlags flags;
+        checkZero(flags);
+
+        // Check we can set field to maximum.
+        flags.set(index, maxval);
+        EXPECT_EQ(maxval, flags.get(index));
+        checkZero(flags, index);
+
+        // Check we can reset it to zero.
+        flags.set(index, 0);
+        checkZero(flags);
+    }
+};
 
 // Set of tests to check that setting a bit only sets that bit and nothing
 // else.
 
-TEST_F(HeaderFlagsTest, QRfield) {
-    HeaderFlags flags;
-    checkZero(flags);
-
-    flags.set(OptionInfo::QR, 1);
-    EXPECT_EQ(1, flags.get(OptionInfo::QR));
-    EXPECT_EQ(0, flags.get(OptionInfo::OP));
-    EXPECT_EQ(0, flags.get(OptionInfo::AA));
-    EXPECT_EQ(0, flags.get(OptionInfo::TC));
-    EXPECT_EQ(0, flags.get(OptionInfo::RD));
-    EXPECT_EQ(0, flags.get(OptionInfo::RA));
-    EXPECT_EQ(0, flags.get(OptionInfo::Z));
-    EXPECT_EQ(0, flags.get(OptionInfo::AD));
-    EXPECT_EQ(0, flags.get(OptionInfo::CD));
-    EXPECT_EQ(0, flags.get(OptionInfo::RC));
-    EXPECT_NE(0, flags.getValue());
-
-    flags.set(OptionInfo::QR, 0);
-    checkZero(flags);
-}
-
-TEST_F(HeaderFlagsTest, OPfield) {
-    HeaderFlags flags;
-    checkZero(flags);
-
-    flags.set(OptionInfo::OP, 15);
-    EXPECT_EQ(0, flags.get(OptionInfo::QR));
-    EXPECT_EQ(15, flags.get(OptionInfo::OP));
-    EXPECT_EQ(0, flags.get(OptionInfo::AA));
-    EXPECT_EQ(0, flags.get(OptionInfo::TC));
-    EXPECT_EQ(0, flags.get(OptionInfo::RD));
-    EXPECT_EQ(0, flags.get(OptionInfo::RA));
-    EXPECT_EQ(0, flags.get(OptionInfo::Z));
-    EXPECT_EQ(0, flags.get(OptionInfo::AD));
-    EXPECT_EQ(0, flags.get(OptionInfo::CD));
-    EXPECT_EQ(0, flags.get(OptionInfo::RC));
-    EXPECT_NE(0, flags.getValue());
-
-    flags.set(OptionInfo::OP, 0);
-    checkZero(flags);
-}
-
-TEST_F(HeaderFlagsTest, AAfield) {
-    HeaderFlags flags;
-    checkZero(flags);
-
-    flags.set(OptionInfo::AA, 1);
-    EXPECT_EQ(0, flags.get(OptionInfo::QR));
-    EXPECT_EQ(0, flags.get(OptionInfo::OP));
-    EXPECT_EQ(1, flags.get(OptionInfo::AA));
-    EXPECT_EQ(0, flags.get(OptionInfo::TC));
-    EXPECT_EQ(0, flags.get(OptionInfo::RD));
-    EXPECT_EQ(0, flags.get(OptionInfo::RA));
-    EXPECT_EQ(0, flags.get(OptionInfo::Z));
-    EXPECT_EQ(0, flags.get(OptionInfo::AD));
-    EXPECT_EQ(0, flags.get(OptionInfo::CD));
-    EXPECT_EQ(0, flags.get(OptionInfo::RC));
-    EXPECT_NE(0, flags.getValue());
-
-    flags.set(OptionInfo::AA, 0);
-    checkZero(flags);
-}
-
-TEST_F(HeaderFlagsTest, TCfield) {
-    HeaderFlags flags;
-    checkZero(flags);
-
-    flags.set(OptionInfo::TC, 1);
-    EXPECT_EQ(0, flags.get(OptionInfo::QR));
-    EXPECT_EQ(0, flags.get(OptionInfo::OP));
-    EXPECT_EQ(0, flags.get(OptionInfo::AA));
-    EXPECT_EQ(1, flags.get(OptionInfo::TC));
-    EXPECT_EQ(0, flags.get(OptionInfo::RD));
-    EXPECT_EQ(0, flags.get(OptionInfo::RA));
-    EXPECT_EQ(0, flags.get(OptionInfo::Z));
-    EXPECT_EQ(0, flags.get(OptionInfo::AD));
-    EXPECT_EQ(0, flags.get(OptionInfo::CD));
-    EXPECT_EQ(0, flags.get(OptionInfo::RC));
-    EXPECT_NE(0, flags.getValue());
-
-    flags.set(OptionInfo::TC, 0);
-    checkZero(flags);
-}
-
-TEST_F(HeaderFlagsTest, RDfield) {
-    HeaderFlags flags;
-    checkZero(flags);
-
-    flags.set(OptionInfo::RD, 1);
-    EXPECT_EQ(0, flags.get(OptionInfo::QR));
-    EXPECT_EQ(0, flags.get(OptionInfo::OP));
-    EXPECT_EQ(0, flags.get(OptionInfo::AA));
-    EXPECT_EQ(0, flags.get(OptionInfo::TC));
-    EXPECT_EQ(1, flags.get(OptionInfo::RD));
-    EXPECT_EQ(0, flags.get(OptionInfo::RA));
-    EXPECT_EQ(0, flags.get(OptionInfo::Z));
-    EXPECT_EQ(0, flags.get(OptionInfo::AD));
-    EXPECT_EQ(0, flags.get(OptionInfo::CD));
-    EXPECT_EQ(0, flags.get(OptionInfo::RC));
-    EXPECT_NE(0, flags.getValue());
-
-    flags.set(OptionInfo::RD, 0);
-    checkZero(flags);
-}
-
-TEST_F(HeaderFlagsTest, RAfield) {
-    HeaderFlags flags;
-    checkZero(flags);
-
-    flags.set(OptionInfo::RA, 1);
-    EXPECT_EQ(0, flags.get(OptionInfo::QR));
-    EXPECT_EQ(0, flags.get(OptionInfo::OP));
-    EXPECT_EQ(0, flags.get(OptionInfo::AA));
-    EXPECT_EQ(0, flags.get(OptionInfo::TC));
-    EXPECT_EQ(0, flags.get(OptionInfo::RD));
-    EXPECT_EQ(1, flags.get(OptionInfo::RA));
-    EXPECT_EQ(0, flags.get(OptionInfo::Z));
-    EXPECT_EQ(0, flags.get(OptionInfo::AD));
-    EXPECT_EQ(0, flags.get(OptionInfo::CD));
-    EXPECT_EQ(0, flags.get(OptionInfo::RC));
-    EXPECT_NE(0, flags.getValue());
-
-    flags.set(OptionInfo::RA, 0);
-    checkZero(flags);
-}
-
-TEST_F(HeaderFlagsTest, Zfield) {
-    HeaderFlags flags;
-    checkZero(flags);
-
-    flags.set(OptionInfo::Z, 1);
-    EXPECT_EQ(0, flags.get(OptionInfo::QR));
-    EXPECT_EQ(0, flags.get(OptionInfo::OP));
-    EXPECT_EQ(0, flags.get(OptionInfo::AA));
-    EXPECT_EQ(0, flags.get(OptionInfo::TC));
-    EXPECT_EQ(0, flags.get(OptionInfo::RD));
-    EXPECT_EQ(0, flags.get(OptionInfo::RA));
-    EXPECT_EQ(1, flags.get(OptionInfo::Z));
-    EXPECT_EQ(0, flags.get(OptionInfo::AD));
-    EXPECT_EQ(0, flags.get(OptionInfo::CD));
-    EXPECT_EQ(0, flags.get(OptionInfo::RC));
-    EXPECT_NE(0, flags.getValue());
-
-    flags.set(OptionInfo::Z, 0);
-    checkZero(flags);
-}
-
-TEST_F(HeaderFlagsTest, ADfield) {
-    HeaderFlags flags;
-    checkZero(flags);
-
-    flags.set(OptionInfo::AD, 1);
-    EXPECT_EQ(0, flags.get(OptionInfo::QR));
-    EXPECT_EQ(0, flags.get(OptionInfo::OP));
-    EXPECT_EQ(0, flags.get(OptionInfo::AA));
-    EXPECT_EQ(0, flags.get(OptionInfo::TC));
-    EXPECT_EQ(0, flags.get(OptionInfo::RD));
-    EXPECT_EQ(0, flags.get(OptionInfo::RA));
-    EXPECT_EQ(0, flags.get(OptionInfo::Z));
-    EXPECT_EQ(1, flags.get(OptionInfo::AD));
-    EXPECT_EQ(0, flags.get(OptionInfo::CD));
-    EXPECT_EQ(0, flags.get(OptionInfo::RC));
-    EXPECT_NE(0, flags.getValue());
-
-    flags.set(OptionInfo::AD, 0);
-    checkZero(flags);
-}
-
-TEST_F(HeaderFlagsTest, CDfield) {
-    HeaderFlags flags;
-    checkZero(flags);
-
-    flags.set(OptionInfo::CD, 1);
-    EXPECT_EQ(0, flags.get(OptionInfo::QR));
-    EXPECT_EQ(0, flags.get(OptionInfo::OP));
-    EXPECT_EQ(0, flags.get(OptionInfo::AA));
-    EXPECT_EQ(0, flags.get(OptionInfo::TC));
-    EXPECT_EQ(0, flags.get(OptionInfo::RD));
-    EXPECT_EQ(0, flags.get(OptionInfo::RA));
-    EXPECT_EQ(0, flags.get(OptionInfo::Z));
-    EXPECT_EQ(0, flags.get(OptionInfo::AD));
-    EXPECT_EQ(1, flags.get(OptionInfo::CD));
-    EXPECT_EQ(0, flags.get(OptionInfo::RC));
-    EXPECT_NE(0, flags.getValue());
-
-    flags.set(OptionInfo::CD, 0);
-    checkZero(flags);
-}
-
-TEST_F(HeaderFlagsTest, RCfield) {
-    HeaderFlags flags;
-    checkZero(flags);
-
-    flags.set(OptionInfo::RC, 15);
-    EXPECT_EQ(0, flags.get(OptionInfo::QR));
-    EXPECT_EQ(0, flags.get(OptionInfo::OP));
-    EXPECT_EQ(0, flags.get(OptionInfo::AA));
-    EXPECT_EQ(0, flags.get(OptionInfo::TC));
-    EXPECT_EQ(0, flags.get(OptionInfo::RD));
-    EXPECT_EQ(0, flags.get(OptionInfo::RA));
-    EXPECT_EQ(0, flags.get(OptionInfo::Z));
-    EXPECT_EQ(0, flags.get(OptionInfo::AD));
-    EXPECT_EQ(0, flags.get(OptionInfo::CD));
-    EXPECT_EQ(15, flags.get(OptionInfo::RC));
-    EXPECT_NE(0, flags.getValue());
-
-    flags.set(OptionInfo::RC, 0);
-    checkZero(flags);
+TEST_F(HeaderFlagsTest, fields) {
+    checkOption(OptionInfo::QR, 1);
+    checkOption(OptionInfo::OP, 15);
+    checkOption(OptionInfo::AA, 1);
+    checkOption(OptionInfo::TC, 1);
+    checkOption(OptionInfo::RD, 1);
+    checkOption(OptionInfo::RA, 1);
+    checkOption(OptionInfo::Z,  1);
+    checkOption(OptionInfo::AD, 1);
+    checkOption(OptionInfo::CD, 1);
+    checkOption(OptionInfo::RC, 15);
 }
 
 // Check that the correct bits are set