[2040] Changes after review

tests/tools/dhcp-ubench/benchmark.cc

@@ -36,12 +36,11 @@ uBenchmark::uBenchmark(uint32_t iterations, const std::string& dbname,
                        const std::string& pass /* = "" */)
     :num_(iterations), sync_(sync), verbose_(verbose),
      hostname_(host), user_(user), passwd_(pass), dbname_(dbname),
-     compiled_stmt_(true)
+     hitratio_(0.9f), compiled_stmt_(true)
 {
     /// @todo: make compiled statements a configurable parameter
 
-    /// @todo: convert this to user-configurable parameter
-    hitratio_ = 0.9f;
+    /// @todo: convert hitratio_ to user-configurable parameter
 
     memset(ts_, 0, sizeof(ts_));
 }
@@ -57,9 +56,10 @@ void uBenchmark::usage() {
     cout << " -u username - specifies MySQL user name (MySQL backend only)" << endl;
     cout << " -p password - specifies MySQL passwod (MySQL backend only)" << endl;
     cout << " -f name - database or filename (MySQL, SQLite and memfile)" << endl;
-    cout << " -n integer - number of test repetitions (MySQL, SQLite and memfile)" << endl;
+    cout << " -n integer - number of test iterations (MySQL, SQLite and memfile)" << endl;
     cout << " -s yes|no - synchronous/asynchronous operation (MySQL, SQLite and memfile)" << endl;
     cout << " -v yes|no - verbose mode (MySQL, SQLite and memfile)" << endl;
+    cout << " -c yes|no - compiled statements (MySQL and SQLite)" << endl;
 
     exit(EXIT_FAILURE);
 }
@@ -85,9 +85,10 @@ void uBenchmark::parseCmdline(int argc, char* const argv[]) {
             break;
         case 'n':
             try {
-                num_ = boost::lexical_cast<int>(optarg);
+                num_ = boost::lexical_cast<unsigned int>(optarg);
             } catch (const boost::bad_lexical_cast &) {
-                cerr << "Failed to iterations (-n option)." << endl;
+                cerr << "Failed to parse number of iterations (-n option):"
+                     << optarg << endl;
                 usage();
             }
             break;
@@ -111,9 +112,9 @@ void uBenchmark::failure(const char* operation) {
     throw string(operation);
 }
 
-void uBenchmark::print_clock(const std::string& operation, uint32_t num,
-                 const struct timespec& before,
-                 const struct timespec& after) {
+void uBenchmark::printClock(const std::string& operation, uint32_t num,
+                            const struct timespec& before,
+                            const struct timespec& after) {
     long int tv_sec = after.tv_sec - before.tv_sec;
 
     long int tv_nsec = after.tv_nsec - before.tv_nsec;
@@ -149,19 +150,19 @@ int uBenchmark::run() {
     try {
         connect();
 
-        ts_[0] = get_time();
+        ts_[0] = getTime();
 
         createLease4Test();
-        ts_[1] = get_time();
+        ts_[1] = getTime();
 
         searchLease4Test();
-        ts_[2] = get_time();
+        ts_[2] = getTime();
 
         updateLease4Test();
-        ts_[3] = get_time();
+        ts_[3] = getTime();
 
         deleteLease4Test();
-        ts_[4] = get_time();
+        ts_[4] = getTime();
 
         disconnect();
 
@@ -170,15 +171,15 @@ int uBenchmark::run() {
         return (-1);
     }
 
-    print_clock("Create leases4", num_, ts_[0], ts_[1]);
-    print_clock("Search leases4", num_, ts_[1], ts_[2]);
-    print_clock("Update leases4", num_, ts_[2], ts_[3]);
-    print_clock("Delete leases4", num_, ts_[3], ts_[4]);
+    printClock("Create leases4", num_, ts_[0], ts_[1]);
+    printClock("Search leases4", num_, ts_[1], ts_[2]);
+    printClock("Update leases4", num_, ts_[2], ts_[3]);
+    printClock("Delete leases4", num_, ts_[3], ts_[4]);
 
     return (0);
 }
 
-struct timespec uBenchmark::get_time() {
+struct timespec uBenchmark::getTime() {
     struct timespec ts;
 
 #ifdef __MACH__ // OS X does not have clock_gettime, use clock_get_time

tests/tools/dhcp-ubench/benchmark.h

@@ -67,11 +67,11 @@ public:
     ///
     /// That benchmark method will be called first.
     /// It is expected to create specific number of leases,
-    /// as specified by \ref Num_ parameter. Following
+    /// as specified by \ref num_ parameter. Following
     /// methods (searchLease4Test(), updateLease4Test(),
     /// and deleteLease4Test()) assume that lease creation
     /// is successful. The benchmark is expected to create leases
-    /// starting from BASE_ADDR4 and ending on BASE_ADDR4 + Num_.
+    /// starting from BASE_ADDR4 and ending on BASE_ADDR4 + num_.
     ///
     /// The implementation is provided by the DB-specific class.
     virtual void createLease4Test() = 0;
@@ -80,10 +80,10 @@ public:
     ///
     /// This is the second benchmark in a series of four.
     /// It is called after createLease4Test(), so it expects that the
-    /// database is populated with at least \ref Num_ leases.
-    /// It repeats search for a lease Num_ times.
+    /// database is populated with at least \ref num_ leases.
+    /// It repeats search for a lease num_ times.
     ///
-    /// The algorithm randomly picks a lease with HitRatio_ (typically 90%)
+    /// The algorithm randomly picks a lease with \ref hitratio_ (typically 90%)
     /// chance of finding a lease. During typical DHCP operation the server
     /// sometimes wants to check if specific lease is assigned or not and the
     /// lease is sometimes not present (e.g. when randomly trying to pick a new
@@ -98,15 +98,15 @@ public:
     ///
     /// This is the third benchmark in a series of four.
     /// It is called after createLease4Test(), so it expects that the
-    /// database is populated with at least \ref Num_ leases.
+    /// database is populated with at least \ref num_ leases.
     ///
     /// In a normal DHCP operation, search and update operations are used
     /// together, but for the benchmarking purposes they are executed
     /// separately here. Once a lease is found, it is being updated. Typically
     /// the update is just changing lease expiration timers, so that is what
-    /// the test does. It exploits the fact that there are Num_ leases
+    /// the test does. It exploits the fact that there are num_ leases
     /// in the database, so it picks randomly an address from
-    /// BASE_ADDR4 ... BASE_ADDR4+Num_ range and has a guarantee for the lease
+    /// BASE_ADDR4 ... BASE_ADDR4 + num_ range and has a guarantee for the lease
     /// to be present.
     ///
     /// The implementation is provided by the DB-specific class.
@@ -116,9 +116,9 @@ public:
     ///
     /// This is the last benchmark in a series of four.
     /// It is called after createLease4Test(), so it expects that the
-    /// database is populated with at least \ref Num_ leases.
+    /// database is populated with at least \ref num_ leases.
     ///
-    /// It is expected to iteratively delete all Num_ leases from
+    /// It is expected to iteratively delete all num_ leases from
     /// the database.
     ///
     /// The implementation is provided by the DB-specific class.
@@ -143,9 +143,9 @@ public:
     /// @param num number or iterations (used for statistics)
     /// @param before timestamp before execution
     /// @param after timestamp after execution
-    void print_clock(const std::string& operation, uint32_t num,
-                     const struct timespec& before,
-                     const struct timespec& after);
+    void printClock(const std::string& operation, uint32_t num,
+                    const struct timespec& before,
+                    const struct timespec& after);
 
     /// @brief Main benchmark execution routine
     ///
@@ -173,7 +173,7 @@ protected:
     void usage();
 
     /// @brief a wrapper around OS-specific method for getting time
-    struct timespec get_time();
+    struct timespec getTime();
 
     /// Number of operations (e.g. insert lease num times)
     uint32_t num_;

tests/tools/dhcp-ubench/memfile_ubench.cc

@@ -86,43 +86,43 @@ protected:
     void writeLease(Lease4Ptr lease);
 
     /// Name of the lease file.
-    std::string Filename_;
+    std::string filename_;
 
     /// should we do flush after each operation?
-    bool Sync_;
+    bool sync_;
 
     /// File handle to the open lease file.
-    FILE * File_;
+    FILE * file_;
 
     /// Hash table for IPv4 leases
     IPv4Hash ip4Hash_;
 };
 
 memfile_LeaseMgr::memfile_LeaseMgr(const std::string& filename, bool sync)
-    : Filename_(filename), Sync_(sync) {
-    File_ = fopen(filename.c_str(), "w");
-    if (!File_) {
+    : filename_(filename), sync_(sync) {
+    file_ = fopen(filename.c_str(), "w");
+    if (!file_) {
         throw "Failed to create file " + filename;
     }
 
 }
 
 memfile_LeaseMgr::~memfile_LeaseMgr() {
-    fclose(File_);
+    fclose(file_);
 }
 
 void memfile_LeaseMgr::writeLease(Lease4Ptr lease) {
-    fprintf(File_, "lease %d {\n  hw-addr ", lease->addr);
+    fprintf(file_, "lease %d {\n  hw-addr ", lease->addr);
     for (std::vector<uint8_t>::const_iterator it = lease->hwaddr.begin();
          it != lease->hwaddr.end(); ++it) {
-        fprintf(File_,"%02x:", *it);
+        fprintf(file_, "%02x:", *it);
     }
-    fprintf(File_, ";\n client-id ");
+    fprintf(file_, ";\n client-id ");
     for (std::vector<uint8_t>::const_iterator it = lease->client_id.begin();
          it != lease->client_id.end(); ++it) {
-        fprintf(File_, "%02x:", *it);
+        fprintf(file_, "%02x:", *it);
     }
-    fprintf(File_, ";\n  valid-lifetime %d;\n  recycle-time %d;\n"
+    fprintf(file_, ";\n  valid-lifetime %d;\n  recycle-time %d;\n"
             "  cltt %d;\n  pool-id %d;\n  fixed %s;  hostname %s;\n"
             "  fqdn_fwd %s;\n  fqdn_rev %s;\n};\n",
             lease->valid_lft, lease->recycle_time, (int)lease->cltt,
@@ -130,9 +130,9 @@ void memfile_LeaseMgr::writeLease(Lease4Ptr lease) {
             lease->hostname.c_str(), lease->fqdn_fwd?"true":"false",
             lease->fqdn_rev?"true":"false");
 
-    if (Sync_) {
-        fflush(File_);
-        fsync(fileno(File_));
+    if (sync_) {
+        fflush(file_);
+        fsync(fileno(file_));
     }
 }
 
@@ -219,21 +219,22 @@ void memfile_uBenchmark::createLease4Test() {
     bool fqdn_fwd = true;           // Let's pretend to do AAAA update
     bool fqdn_rev = true;           // Let's pretend to do PTR update
 
-    printf("CREATE:   ");
+    cout << "CREATE:   ";
 
     // While we could put the data directly into vector, I would like to
     // keep the code as  similar to other benchmarks as possible
-    for (uint8_t i = 0; i < hwaddr_len; i++) {
-        hwaddr_tmp[i] = 65 + i;
+    for (uint8_t i = 0; i < hwaddr_len; ++i) {
+        hwaddr_tmp[i] = 'A' + i; // let's make hwaddr consisting of letter
     }
-    vector<uint8_t> hwaddr(hwaddr_tmp, hwaddr_tmp + 19);
+    vector<uint8_t> hwaddr(hwaddr_tmp, hwaddr_tmp + hwaddr_len - 1);
 
     for (uint8_t i = 0; i < client_id_len; i++) {
-        client_id_tmp[i] = 33 + i;
+        client_id_tmp[i] = 33 + i; // 33 is being the first, non whitespace
+                                   // printable ASCII character
     }
-    vector<uint8_t> client_id(client_id_tmp, client_id_tmp + 19);
+    vector<uint8_t> client_id(client_id_tmp, client_id_tmp + client_id_len - 1);
 
-    for (uint32_t i = 0; i < num_; i++) {
+    for (uint32_t i = 0; i < num_; ++i) {
 
         cltt++;
 
@@ -254,13 +255,13 @@ void memfile_uBenchmark::createLease4Test() {
             failure("addLease() failed");
         } else {
             if (verbose_) {
-                printf(".");
+                cout << ".";
             }
         };
 
         addr++;
     }
-    printf("\n");
+    cout << endl;
 }
 
 void memfile_uBenchmark::searchLease4Test() {
@@ -268,25 +269,18 @@ void memfile_uBenchmark::searchLease4Test() {
         throw "No LeaseMgr instantiated.";
     }
 
-    // This formula should roughly find something a lease in 90% cases
-    float hitRatio = 0.5;
-
-    printf("RETRIEVE: ");
+    cout << "RETRIEVE: ";
 
     for (uint32_t i = 0; i < num_; i++) {
-        uint32_t x = BASE_ADDR4 + random() % int(num_ / hitRatio);
+        uint32_t x = BASE_ADDR4 + random() % int(num_ / hitratio_);
 
         Lease4Ptr lease = leaseMgr_->getLease(x);
         if (verbose_) {
-            if (lease) {
-                printf(".");
-            } else {
-                printf("X");
-            }
+            cout << (lease?".":"X");
         }
     }
 
-    printf("\n");
+    cout << endl;
 }
 
 void memfile_uBenchmark::updateLease4Test() {
@@ -294,7 +288,7 @@ void memfile_uBenchmark::updateLease4Test() {
         throw "No LeaseMgr instantiated.";
     }
 
-    printf("UPDATE:   ");
+    cout << "UPDATE:   ";
 
     time_t cltt = time(NULL);
 
@@ -309,11 +303,11 @@ void memfile_uBenchmark::updateLease4Test() {
             failure(tmp.str().c_str());
         }
         if (verbose_) {
-            printf(".");
+            cout << ".";
         }
     }
 
-    printf("\n");
+    cout << endl;
 }
 
 void memfile_uBenchmark::deleteLease4Test() {
@@ -321,7 +315,7 @@ void memfile_uBenchmark::deleteLease4Test() {
         throw "No LeaseMgr instantiated.";
     }
 
-    printf("DELETE:   ");
+    cout << "DELETE:   ";
 
     for (uint32_t i = 0; i < num_; i++) {
 
@@ -333,11 +327,11 @@ void memfile_uBenchmark::deleteLease4Test() {
             failure(tmp.str().c_str());
         }
         if (verbose_) {
-            printf(".");
+            cout << ".";
         }
     }
 
-    printf("\n");
+    cout << endl;
 }
 
 void memfile_uBenchmark::printInfo() {

tests/tools/dhcp-ubench/mysql_ubench.cc

@@ -101,7 +101,7 @@ void MySQL_uBenchmark::createLease4Test() {
     sprintf(cltt, "2012-07-11 15:43:00");
     cltt_len = strlen(cltt);
 
-    uint32_t pool_id = 1000;       // Let's use pools 0-99
+    uint32_t pool_id = 1000;       // Let's use pool-ids greater than zero
     bool fixed = false;
 
     char hostname[] = "foo";    // Will generate it dynamically
@@ -111,17 +111,18 @@ void MySQL_uBenchmark::createLease4Test() {
     bool fqdn_fwd = true;       // Let's pretend to do AAAA update
     bool fqdn_rev = true;       // Let's pretend to do PTR update
 
-    printf("CREATE:   ");
+    cout << "CREATE:   ";
 
-    for (uint8_t i = 0; i < 20; i++) {
-        hwaddr[i] = 65 + i;
+    for (uint8_t i = 0; i < hwaddr_len; i++) {
+        hwaddr[i] = 'A' + i; // let's make hwaddr consisting of letters
     }
-    hwaddr[19] = 0;
+    hwaddr[19] = 0; // make it is null-terminated
 
-    for (uint8_t i = 0; i < 128; i++) {
-        client_id[i] = 33 + i;
+    for (uint8_t i = 0; i < client_id_len; i++) {
+        client_id[i] = 33 + i; // 33 is being the first, non whitespace
+                               // printable ASCII character
     }
-    client_id[127] = 0;
+    client_id[127] = 0; // make it is null-terminated
 
     MYSQL_STMT * stmt = NULL;
     MYSQL_BIND bind[11]; // 11 parameters in the insert statement
@@ -262,7 +263,7 @@ void MySQL_uBenchmark::createLease4Test() {
         }
 
         if (verbose_) {
-            printf(".");
+            cout << ".";
         }
     }
 
@@ -272,7 +273,7 @@ void MySQL_uBenchmark::createLease4Test() {
         }
     }
 
-    printf("\n");
+    cout << endl;
 }
 
 void MySQL_uBenchmark::searchLease4Test() {
@@ -280,7 +281,7 @@ void MySQL_uBenchmark::searchLease4Test() {
         throw "Not connected to MySQL server.";
     }
 
-    printf("RETRIEVE: ");
+    cout << "RETRIEVE: ";
 
     uint32_t addr = 0;
 
@@ -348,12 +349,12 @@ void MySQL_uBenchmark::searchLease4Test() {
                 mysql_free_result(result);
 
                 if (verbose_) {
-                    printf("."); // hit
+                    cout << "."; // hit
                 }
 
             } else {
                 if (verbose_) {
-                    printf("x"); // miss
+                    cout << "x"; // miss
                 }
             }
         } else {
@@ -438,7 +439,7 @@ void MySQL_uBenchmark::searchLease4Test() {
 
             if (mysql_stmt_bind_result(stmt, response))
             {
-                printf("Error: %s\n", mysql_stmt_error(stmt));
+                cout << "Error:" << mysql_stmt_error(stmt) << endl;
                 failure("mysql_stmt_bind_result() failed");
             }
             int num_rows = 0;
@@ -456,11 +457,11 @@ void MySQL_uBenchmark::searchLease4Test() {
 
             if (num_rows) {
                 if (verbose_) {
-                    printf("."); // hit
+                    cout << "."; // hit
                 }
             } else {
                 if (verbose_) {
-                    printf("x"); // miss
+                    cout << "X"; // miss
                 }
             }
 
@@ -473,7 +474,7 @@ void MySQL_uBenchmark::searchLease4Test() {
         }
     }
 
-    printf("\n");
+    cout << endl;
 }
 
 void MySQL_uBenchmark::updateLease4Test() {
@@ -481,7 +482,7 @@ void MySQL_uBenchmark::updateLease4Test() {
         throw "Not connected to MySQL server.";
     }
 
-    printf("UPDATE:   ");
+    cout << "UPDATE:   ";
 
     uint32_t valid_lft = 1002; // just some dummy value
     char cltt[] = "now()";
@@ -541,7 +542,7 @@ void MySQL_uBenchmark::updateLease4Test() {
         }
 
         if (verbose_) {
-            printf(".");
+            cout << ".";
         }
     }
 
@@ -551,7 +552,7 @@ void MySQL_uBenchmark::updateLease4Test() {
         }
     }
 
-    printf("\n");
+    cout << endl;
 }
 
 void MySQL_uBenchmark::deleteLease4Test() {
@@ -559,7 +560,7 @@ void MySQL_uBenchmark::deleteLease4Test() {
         throw "Not connected to MySQL server.";
     }
 
-    printf("DELETE:   ");
+    cout << "DELETE:   ";
 
     uint32_t addr = 0;
 
@@ -612,7 +613,7 @@ void MySQL_uBenchmark::deleteLease4Test() {
         }
 
         if (verbose_) {
-            printf(".");
+            cout << ".";
         }
     }
 
@@ -622,7 +623,7 @@ void MySQL_uBenchmark::deleteLease4Test() {
         }
     }
 
-    printf("\n");
+    cout << endl;
 }
 
 void MySQL_uBenchmark::printInfo() {

tests/tools/dhcp-ubench/mysql_ubench.h

@@ -33,7 +33,7 @@ public:
     ///
     /// Synchronous operation means using InnDB, async is MyISAM.
     ///
-    /// @param hostname Name of the hostname to connect to
+    /// @param hostname Name of the host to connect to
     /// @param user usename used during MySQL connection
     /// @param pass password used during MySQL connection
     /// @param db name of the database to connect to

tests/tools/dhcp-ubench/sqlite_ubench.cc

@@ -58,6 +58,8 @@ void SQLite_uBenchmark::disconnect() {
     if (db_) {
         sqlite3_close(db_);
         db_ = NULL;
+    } else {
+        throw "Can't close SQLite connection: it was never open.";
     }
 }
 
@@ -81,7 +83,7 @@ void SQLite_uBenchmark::createLease4Test() {
     bool fqdn_fwd = true;           // Let's pretend to do AAAA update
     bool fqdn_rev = true;           // Let's pretend to do PTR update
 
-    printf("CREATE:   ");
+    cout << "CREATE:   ";
 
     for (uint8_t i = 0; i < hwaddr_len; i++) {
         hwaddr[i] = 65 + i;
@@ -126,7 +128,6 @@ void SQLite_uBenchmark::createLease4Test() {
                     addr, hwaddr, client_id, valid_lft, recycle_time,
                     cltt, pool_id, (fixed?"true":"false"),
                     hostname.c_str(), (fqdn_fwd?"true":"false"), (fqdn_rev?"true":"false"));
-            // printf("QUERY=[%s]\n", query);
 
             int result = sqlite3_exec(db_, query, NULL, 0, &errorMsg);
 
@@ -204,7 +205,7 @@ void SQLite_uBenchmark::createLease4Test() {
         }
 
         if (verbose_) {
-            printf(".");
+            cout << ".";
         }
     }
 
@@ -215,23 +216,30 @@ void SQLite_uBenchmark::createLease4Test() {
         }
     }
 
-    printf("\n");
+    cout << endl;
 }
 
-static int search_callback(void *counter, int /*argc*/, char** /*argv*/,
-                           char** /*azColName*/){
+static int search_callback(void *counter, int argc, char** argv,
+                           char** azColName){
 
     int* cnt = static_cast<int*>(counter);
     (*cnt)++;
 
-#if 0
-    int i;
-    for(i=0; i<argc; i++){
-        printf("%s=%s ", azColName[i], argv[i] ? argv[i] : "NULL");
+    char buf[512];
+
+    // retrieved lease can be accessed here
+    for(int i = 0; i < argc; i++){
+        // pretend we do something with returned lease
+        if (argv[i]) {
+            strncpy(buf, azColName[i], 512);
+            strncpy(buf, argv[i], 512);
+        }
+
+        // Uncomment this to print out all contents
+        // cout << azColName[i] << "=" << (argv[i] ? argv[i] : "NULL") << endl;
     }
-    printf("\n");
-#endif
-    return 0;
+
+    return (0);
 }
 
 void SQLite_uBenchmark::searchLease4Test() {
@@ -239,7 +247,7 @@ void SQLite_uBenchmark::searchLease4Test() {
         throw "SQLite connection is closed.";
     }
 
-    printf("RETRIEVE: ");
+    cout << "RETRIEVE: ";
 
     sqlite3_stmt *stmt = NULL;
     if (compiled_stmt_) {
@@ -331,7 +339,7 @@ void SQLite_uBenchmark::searchLease4Test() {
         }
 
         if (verbose_) {
-            printf("%s", (cnt?".":"X"));
+            cout << (cnt?".":"X");
         }
     }
 
@@ -342,7 +350,7 @@ void SQLite_uBenchmark::searchLease4Test() {
         }
     }
 
-    printf("\n");
+    cout << endl;
 }
 
 void SQLite_uBenchmark::updateLease4Test() {
@@ -350,7 +358,7 @@ void SQLite_uBenchmark::updateLease4Test() {
         throw "SQLite connection is closed.";
     }
 
-    printf("UPDATE:   ");
+    cout << "UPDATE:   ";
 
     sqlite3_stmt *stmt = NULL;
     if (compiled_stmt_) {
@@ -400,7 +408,7 @@ void SQLite_uBenchmark::updateLease4Test() {
         }
 
         if (verbose_) {
-            printf(".");
+            cout << ".";
         }
     }
 
@@ -411,7 +419,7 @@ void SQLite_uBenchmark::updateLease4Test() {
         }
     }
 
-    printf("\n");
+    cout << endl;
 }
 
 void SQLite_uBenchmark::deleteLease4Test() {
@@ -419,7 +427,7 @@ void SQLite_uBenchmark::deleteLease4Test() {
         throw "SQLite connection is closed.";
     }
 
-    printf("DELETE:   ");
+    cout << "DELETE:   ";
 
     sqlite3_stmt *stmt = NULL;
     if (compiled_stmt_) {
@@ -467,7 +475,7 @@ void SQLite_uBenchmark::deleteLease4Test() {
 
         }
         if (verbose_) {
-            printf(".");
+            cout << ".";
         }
     }
 
@@ -478,7 +486,7 @@ void SQLite_uBenchmark::deleteLease4Test() {
         }
     }
 
-    printf("\n");
+    cout << endl;
 }
 
 void SQLite_uBenchmark::printInfo() {