[3601] Addressed additional review comments

doc/guide/admin.xml
src/lib/dhcpsrv/dhcpsrv_messages.mes
src/lib/dhcpsrv/lease_file_loader.h
    minor clean up, typos

src/lib/dhcpsrv/tests/csv_lease_file4_unittest.cc
src/lib/dhcpsrv/tests/csv_lease_file6_unittest.cc
    removed unnecessary use of scoped_ptr

src/lib/util/csv_file.cc
    CSVRow::trim() - replaced use of std::vector<>::erase with resise

src/lib/util/versioned_csv_file.h
src/lib/util/versioned_csv_file.cc
    VersionedCSVFile::next() - reorganized to use input_schema_state_
    VersionedCSVFile::columnCountError() - new convenience method
    minor cleanups

doc/guide/admin.xml

@@ -159,7 +159,7 @@
         During startup the servers will check the schema version of the lease
         files against their own.  If there is a mismatch, the servers will
         automatically launch the LFC process to convert the files to the
-        server's schema vesion.  While this mechanism is primarily meant to
+        server's schema version.  While this mechanism is primarily meant to
         ease the process of upgrading to newer versions of Kea, it can also
         be used for downgrading should the need arise.  When upgrading, any
         values not present in the original lease files will be assigned

src/lib/dhcpsrv/dhcpsrv_messages.mes

@@ -359,7 +359,7 @@ timer used for lease file cleanup scheduling. This is highly unlikely
 and indicates programming error. The message include the reason for this
 error.
 
-% DHCPSRV_MEMFILE_NEEDS_DOWNGRADING lease file: %1 is beyond version %2, it needs to be downgraded to current schema version, %3.
+% DHCPSRV_MEMFILE_NEEDS_DOWNGRADING schema of lease file: %1 is later than version %2.
 A warning message issued when the schema of the lease file loaded by the server
 is newer than the memfile schema of the server.  The server converts the lease
 data from newer schemas to its schema as it is read, therefore the lease
@@ -367,7 +367,7 @@ information in use by the server will be correct. Note though, that any data
 data stored in newer schema fields will be dropped.  What remains is for the
 file itself to be rewritten using the current schema.
 
-% DHCPSRV_MEMFILE_NEEDS_UPGRADING lease file: %1 is schema version %2, it needs to be upgraded to current schema version, %3.
+% DHCPSRV_MEMFILE_NEEDS_UPGRADING schema of lease file: %1 is at version %2.
 A warning message issued when the schema of the lease file loaded by the server
 pre-dates the memfile schema of the server.  Note that the server converts the
 lease data from older schemas to the current schema as it is read, therefore

src/lib/dhcpsrv/lease_file_loader.h

@@ -161,8 +161,7 @@ public:
                       ?  DHCPSRV_MEMFILE_NEEDS_UPGRADING
                       : DHCPSRV_MEMFILE_NEEDS_DOWNGRADING))
                      .arg(lease_file.getFilename())
-                     .arg(lease_file.getInputSchemaState())
-                     .arg(lease_file.getSchemaVersion());
+                     .arg(lease_file.getInputSchemaState());
         }
 
         if (close_file_on_exit) {

src/lib/dhcpsrv/tests/csv_lease_file4_unittest.cc

@@ -18,8 +18,6 @@
 #include <dhcpsrv/csv_lease_file4.h>
 #include <dhcpsrv/lease.h>
 #include <dhcpsrv/tests/lease_file_io.h>
-#include <boost/scoped_ptr.hpp>
-#include <boost/shared_ptr.hpp>
 #include <gtest/gtest.h>
 #include <sstream>
 
@@ -125,22 +123,22 @@ TEST_F(CSVLeaseFile4Test, parse) {
     writeSampleFile();
 
     // Open the lease file.
-    boost::scoped_ptr<CSVLeaseFile4> lf(new CSVLeaseFile4(filename_));
-    ASSERT_NO_THROW(lf->open());
+    CSVLeaseFile4 lf(filename_);
+    ASSERT_NO_THROW(lf.open());
 
     // Verify the counters are cleared
     {
     SCOPED_TRACE("Check stats are empty");
-    checkStats(*lf, 0, 0, 0, 0, 0, 0);
+    checkStats(lf, 0, 0, 0, 0, 0, 0);
     }
 
     Lease4Ptr lease;
     // Reading first read should be successful.
     {
     SCOPED_TRACE("First lease valid");
-    EXPECT_TRUE(lf->next(lease));
+    EXPECT_TRUE(lf.next(lease));
     ASSERT_TRUE(lease);
-    checkStats(*lf, 1, 1, 0, 0, 0, 0);
+    checkStats(lf, 1, 1, 0, 0, 0, 0);
 
     // Verify that the lease attributes are correct.
     EXPECT_EQ("192.0.2.1", lease->addr_.toText());
@@ -159,17 +157,17 @@ TEST_F(CSVLeaseFile4Test, parse) {
     // Second lease is malformed - HW address is empty.
     {
     SCOPED_TRACE("Second lease malformed");
-    EXPECT_FALSE(lf->next(lease));
-    checkStats(*lf, 2, 1, 1, 0, 0, 0);
+    EXPECT_FALSE(lf.next(lease));
+    checkStats(lf, 2, 1, 1, 0, 0, 0);
     }
 
     // Even though parsing previous lease failed, reading the next lease should be
     // successful.
     {
     SCOPED_TRACE("Third lease valid");
-    EXPECT_TRUE(lf->next(lease));
+    EXPECT_TRUE(lf.next(lease));
     ASSERT_TRUE(lease);
-    checkStats(*lf, 3, 2, 1, 0, 0, 0);
+    checkStats(lf, 3, 2, 1, 0, 0, 0);
 
     // Verify that the third lease is correct.
     EXPECT_EQ("192.0.3.15", lease->addr_.toText());
@@ -190,28 +188,28 @@ TEST_F(CSVLeaseFile4Test, parse) {
     // lease pointer should be NULL.
     {
     SCOPED_TRACE("Fifth read empty");
-    EXPECT_TRUE(lf->next(lease));
+    EXPECT_TRUE(lf.next(lease));
     EXPECT_FALSE(lease);
-    checkStats(*lf, 4, 2, 1, 0, 0, 0);
+    checkStats(lf, 4, 2, 1, 0, 0, 0);
     }
 
     // We should be able to do it again.
     {
     SCOPED_TRACE("Sixth read empty");
-    EXPECT_TRUE(lf->next(lease));
+    EXPECT_TRUE(lf.next(lease));
     EXPECT_FALSE(lease);
-    checkStats(*lf, 5, 2, 1, 0, 0, 0);
+    checkStats(lf, 5, 2, 1, 0, 0, 0);
     }
 }
 
 // This test checks creation of the lease file and writing leases.
 TEST_F(CSVLeaseFile4Test, recreate) {
-    boost::scoped_ptr<CSVLeaseFile4> lf(new CSVLeaseFile4(filename_));
-    ASSERT_NO_THROW(lf->recreate());
+    CSVLeaseFile4 lf(filename_);
+    ASSERT_NO_THROW(lf.recreate());
     ASSERT_TRUE(io_.exists());
 
     // Verify the counters are cleared
-    checkStats(*lf, 0, 0, 0, 0, 0, 0);
+    checkStats(lf, 0, 0, 0, 0, 0, 0);
 
     // Create first lease, with NULL client id.
     Lease4Ptr lease(new Lease4(IOAddress("192.0.3.2"),
@@ -222,8 +220,8 @@ TEST_F(CSVLeaseFile4Test, recreate) {
     lease->state_ = Lease::STATE_EXPIRED_RECLAIMED;
     {
     SCOPED_TRACE("First write");
-    ASSERT_NO_THROW(lf->append(*lease));
-    checkStats(*lf, 0, 0, 0, 1, 1, 0);
+    ASSERT_NO_THROW(lf.append(*lease));
+    checkStats(lf, 0, 0, 0, 1, 1, 0);
     }
 
     // Create second lease, with non-NULL client id.
@@ -233,12 +231,12 @@ TEST_F(CSVLeaseFile4Test, recreate) {
                            100, 60, 90, 0, 7));
     {
     SCOPED_TRACE("Second write");
-    ASSERT_NO_THROW(lf->append(*lease));
-    checkStats(*lf, 0, 0, 0, 2, 2, 0);
+    ASSERT_NO_THROW(lf.append(*lease));
+    checkStats(lf, 0, 0, 0, 2, 2, 0);
     }
 
     // Close the lease file.
-    lf->close();
+    lf.close();
     // Check that the contents of the csv file are correct.
     EXPECT_EQ("address,hwaddr,client_id,valid_lifetime,expire,subnet_id,"
               "fqdn_fwd,fqdn_rev,hostname,state\n"
@@ -268,15 +266,15 @@ TEST_F(CSVLeaseFile4Test, mixedSchemaload) {
                    );
 
     // Open the lease file.
-    boost::scoped_ptr<CSVLeaseFile4> lf(new CSVLeaseFile4(filename_));
-    ASSERT_NO_THROW(lf->open());
+    CSVLeaseFile4 lf(filename_);
+    ASSERT_NO_THROW(lf.open());
 
     Lease4Ptr lease;
 
     // Reading first read should be successful.
     {
     SCOPED_TRACE("First lease valid");
-    EXPECT_TRUE(lf->next(lease));
+    EXPECT_TRUE(lf.next(lease));
     ASSERT_TRUE(lease);
 
     // Verify that the lease attributes are correct.
@@ -296,7 +294,7 @@ TEST_F(CSVLeaseFile4Test, mixedSchemaload) {
 
     {
     SCOPED_TRACE("Second lease valid");
-    EXPECT_TRUE(lf->next(lease));
+    EXPECT_TRUE(lf.next(lease));
     ASSERT_TRUE(lease);
 
     // Verify that the lease attributes are correct.
@@ -315,7 +313,7 @@ TEST_F(CSVLeaseFile4Test, mixedSchemaload) {
 
     {
     SCOPED_TRACE("Third lease valid");
-    EXPECT_TRUE(lf->next(lease));
+    EXPECT_TRUE(lf.next(lease));
     ASSERT_TRUE(lease);
 
     // Verify that the third lease is correct.
@@ -342,8 +340,8 @@ TEST_F(CSVLeaseFile4Test, tooFewHeaderColumns) {
                   "fqdn_fwd,fqdn_rev\n");
 
     // Open the lease file.
-    boost::scoped_ptr<CSVLeaseFile4> lf(new CSVLeaseFile4(filename_));
-    ASSERT_THROW(lf->open(), CSVFileError);
+    CSVLeaseFile4 lf(filename_);
+    ASSERT_THROW(lf.open(), CSVFileError);
 }
 
 // Verifies that a lease file with an unrecognized column header
@@ -354,8 +352,8 @@ TEST_F(CSVLeaseFile4Test, invalidHeaderColumn) {
                   "fqdn_fwd,fqdn_rev,hostname,state\n");
 
     // Open the lease file.
-    boost::scoped_ptr<CSVLeaseFile4> lf(new CSVLeaseFile4(filename_));
-    ASSERT_THROW(lf->open(), CSVFileError);
+    CSVLeaseFile4 lf(filename_);
+    ASSERT_THROW(lf.open(), CSVFileError);
 }
 
 // Verifies that a lease file with more header columns than defined
@@ -369,16 +367,16 @@ TEST_F(CSVLeaseFile4Test, downGrade) {
                   "three.example.com,2,BOGUS\n");
 
     // Lease file should open and report as needing downgrade.
-    boost::scoped_ptr<CSVLeaseFile4> lf(new CSVLeaseFile4(filename_));
-    ASSERT_NO_THROW(lf->open());
-    EXPECT_TRUE(lf->needsConversion());
+    CSVLeaseFile4 lf(filename_);
+    ASSERT_NO_THROW(lf.open());
+    EXPECT_TRUE(lf.needsConversion());
     EXPECT_EQ(util::VersionedCSVFile::NEEDS_DOWNGRADE,
-              lf->getInputSchemaState());
+              lf.getInputSchemaState());
     Lease4Ptr lease;
 
     {
     SCOPED_TRACE("First lease valid");
-    EXPECT_TRUE(lf->next(lease));
+    EXPECT_TRUE(lf.next(lease));
     ASSERT_TRUE(lease);
 
     // Verify that the third lease is correct.

src/lib/dhcpsrv/tests/csv_lease_file6_unittest.cc

@@ -18,8 +18,6 @@
 #include <dhcpsrv/csv_lease_file6.h>
 #include <dhcpsrv/lease.h>
 #include <dhcpsrv/tests/lease_file_io.h>
-#include <boost/scoped_ptr.hpp>
-#include <boost/shared_ptr.hpp>
 #include <gtest/gtest.h>
 #include <sstream>
 
@@ -126,22 +124,22 @@ TEST_F(CSVLeaseFile6Test, parse) {
     writeSampleFile();
 
     // Open the lease file.
-    boost::scoped_ptr<CSVLeaseFile6> lf(new CSVLeaseFile6(filename_));
-    ASSERT_NO_THROW(lf->open());
+    CSVLeaseFile6 lf(filename_);
+    ASSERT_NO_THROW(lf.open());
 
     // Verify the counters are cleared
     {
     SCOPED_TRACE("Check stats are empty");
-    checkStats(*lf, 0, 0, 0, 0, 0, 0);
+    checkStats(lf, 0, 0, 0, 0, 0, 0);
     }
 
     Lease6Ptr lease;
     // Reading first read should be successful.
     {
     SCOPED_TRACE("First lease valid");
-    EXPECT_TRUE(lf->next(lease));
+    EXPECT_TRUE(lf.next(lease));
     ASSERT_TRUE(lease);
-    checkStats(*lf, 1, 1, 0, 0, 0, 0);
+    checkStats(lf, 1, 1, 0, 0, 0, 0);
 
     // Verify that the lease attributes are correct.
     EXPECT_EQ("2001:db8:1::1", lease->addr_.toText());
@@ -162,17 +160,17 @@ TEST_F(CSVLeaseFile6Test, parse) {
     // Second lease is malformed - DUID is empty.
     {
     SCOPED_TRACE("Second lease malformed");
-    EXPECT_FALSE(lf->next(lease));
-    checkStats(*lf, 2, 1, 1, 0, 0, 0);
+    EXPECT_FALSE(lf.next(lease));
+    checkStats(lf, 2, 1, 1, 0, 0, 0);
     }
 
     // Even, parsing previous lease failed, reading the next lease should be
     // successful.
     {
     SCOPED_TRACE("Third lease valid");
-    EXPECT_TRUE(lf->next(lease));
+    EXPECT_TRUE(lf.next(lease));
     ASSERT_TRUE(lease);
-    checkStats(*lf, 3, 2, 1, 0, 0, 0);
+    checkStats(lf, 3, 2, 1, 0, 0, 0);
 
     // Verify that the third lease is correct.
     EXPECT_EQ("2001:db8:2::10", lease->addr_.toText());
@@ -193,9 +191,9 @@ TEST_F(CSVLeaseFile6Test, parse) {
     // Reading the fourth lease should be successful.
     {
     SCOPED_TRACE("Fourth lease valid");
-    EXPECT_TRUE(lf->next(lease));
+    EXPECT_TRUE(lf.next(lease));
     ASSERT_TRUE(lease);
-    checkStats(*lf, 4, 3, 1, 0, 0, 0);
+    checkStats(lf, 4, 3, 1, 0, 0, 0);
 
     // Verify that the lease is correct.
     EXPECT_EQ("3000:1::", lease->addr_.toText());
@@ -217,30 +215,30 @@ TEST_F(CSVLeaseFile6Test, parse) {
     // lease pointer should be NULL.
     {
     SCOPED_TRACE("Fifth read empty");
-    EXPECT_TRUE(lf->next(lease));
+    EXPECT_TRUE(lf.next(lease));
     EXPECT_FALSE(lease);
-    checkStats(*lf, 5, 3, 1, 0, 0, 0);
+    checkStats(lf, 5, 3, 1, 0, 0, 0);
     }
 
     // We should be able to do it again.
     {
     SCOPED_TRACE("Sixth read empty");
-    EXPECT_TRUE(lf->next(lease));
+    EXPECT_TRUE(lf.next(lease));
     EXPECT_FALSE(lease);
-    checkStats(*lf, 6, 3, 1, 0, 0, 0);
+    checkStats(lf, 6, 3, 1, 0, 0, 0);
     }
 }
 
 // This test checks creation of the lease file and writing leases.
 TEST_F(CSVLeaseFile6Test, recreate) {
-    boost::scoped_ptr<CSVLeaseFile6> lf(new CSVLeaseFile6(filename_));
-    ASSERT_NO_THROW(lf->recreate());
+    CSVLeaseFile6 lf(filename_);
+    ASSERT_NO_THROW(lf.recreate());
     ASSERT_TRUE(io_.exists());
 
     // Verify the counters are cleared
     {
     SCOPED_TRACE("Check stats are empty");
-    checkStats(*lf, 0, 0, 0, 0, 0, 0);
+    checkStats(lf, 0, 0, 0, 0, 0, 0);
     }
 
     Lease6Ptr lease(new Lease6(Lease::TYPE_NA, IOAddress("2001:db8:1::1"),
@@ -250,8 +248,8 @@ TEST_F(CSVLeaseFile6Test, recreate) {
     lease->cltt_ = 0;
     {
     SCOPED_TRACE("First write");
-    ASSERT_NO_THROW(lf->append(*lease));
-    checkStats(*lf, 0, 0, 0, 1, 1, 0);
+    ASSERT_NO_THROW(lf.append(*lease));
+    checkStats(lf, 0, 0, 0, 1, 1, 0);
     }
 
     lease.reset(new Lease6(Lease::TYPE_NA, IOAddress("2001:db8:2::10"),
@@ -261,8 +259,8 @@ TEST_F(CSVLeaseFile6Test, recreate) {
     lease->cltt_ = 0;
     {
     SCOPED_TRACE("Second write");
-    ASSERT_NO_THROW(lf->append(*lease));
-    checkStats(*lf, 0, 0, 0, 2, 2, 0);
+    ASSERT_NO_THROW(lf.append(*lease));
+    checkStats(lf, 0, 0, 0, 2, 2, 0);
     }
 
     lease.reset(new Lease6(Lease::TYPE_PD, IOAddress("3000:1:1::"),
@@ -272,8 +270,8 @@ TEST_F(CSVLeaseFile6Test, recreate) {
     lease->cltt_ = 0;
     {
     SCOPED_TRACE("Third write");
-    ASSERT_NO_THROW(lf->append(*lease));
-    checkStats(*lf, 0, 0, 0, 3, 3, 0);
+    ASSERT_NO_THROW(lf.append(*lease));
+    checkStats(lf, 0, 0, 0, 3, 3, 0);
     }
 
     EXPECT_EQ("address,duid,valid_lifetime,expire,subnet_id,pref_lifetime,"
@@ -309,13 +307,13 @@ TEST_F(CSVLeaseFile6Test, mixedSchemaLoad) {
               "200,200,8,100,0,7,0,1,1,three.example.com,0a:0b:0c:0d:0e,1\n");
 
     // Open the lease file.
-    boost::scoped_ptr<CSVLeaseFile6> lf(new CSVLeaseFile6(filename_));
-    ASSERT_NO_THROW(lf->open());
+    CSVLeaseFile6 lf(filename_);
+    ASSERT_NO_THROW(lf.open());
 
     Lease6Ptr lease;
     {
     SCOPED_TRACE("First lease valid");
-    EXPECT_TRUE(lf->next(lease));
+    EXPECT_TRUE(lf.next(lease));
     ASSERT_TRUE(lease);
 
     // Verify that the lease attributes are correct.
@@ -340,7 +338,7 @@ TEST_F(CSVLeaseFile6Test, mixedSchemaLoad) {
 
     {
     SCOPED_TRACE("Second lease valid");
-    EXPECT_TRUE(lf->next(lease));
+    EXPECT_TRUE(lf.next(lease));
     ASSERT_TRUE(lease);
 
     // Verify that the lease attributes are correct.
@@ -365,7 +363,7 @@ TEST_F(CSVLeaseFile6Test, mixedSchemaLoad) {
 
     {
     SCOPED_TRACE("Third lease valid");
-    EXPECT_TRUE(lf->next(lease));
+    EXPECT_TRUE(lf.next(lease));
     ASSERT_TRUE(lease);
 
     // Verify that the lease attributes are correct.
@@ -396,8 +394,8 @@ TEST_F(CSVLeaseFile6Test, tooFewHeaderColumns) {
               "lease_type,iaid,prefix_len,fqdn_fwd,fqdn_rev\n");
 
     // Open should fail.
-    boost::scoped_ptr<CSVLeaseFile6> lf(new CSVLeaseFile6(filename_));
-    ASSERT_THROW(lf->open(), CSVFileError);
+    CSVLeaseFile6 lf(filename_);
+    ASSERT_THROW(lf.open(), CSVFileError);
 }
 
 // Verifies that a lease file with an unrecognized column header
@@ -408,8 +406,8 @@ TEST_F(CSVLeaseFile6Test, invalidHeaderColumn) {
               "hwaddr,state\n");
 
     // Open should fail.
-    boost::scoped_ptr<CSVLeaseFile6> lf(new CSVLeaseFile6(filename_));
-    ASSERT_THROW(lf->open(), CSVFileError);
+    CSVLeaseFile6 lf(filename_);
+    ASSERT_THROW(lf.open(), CSVFileError);
 }
 
 // Verifies that a lease file with more header columns than defined
@@ -428,17 +426,17 @@ TEST_F(CSVLeaseFile6Test, downGrade) {
               "BOGUS\n");
 
     // Open should succeed in the event someone is downgrading.
-    boost::scoped_ptr<CSVLeaseFile6> lf(new CSVLeaseFile6(filename_));
-    ASSERT_NO_THROW(lf->open());
-    EXPECT_TRUE(lf->needsConversion());
+    CSVLeaseFile6 lf(filename_);
+    ASSERT_NO_THROW(lf.open());
+    EXPECT_TRUE(lf.needsConversion());
     EXPECT_EQ(util::VersionedCSVFile::NEEDS_DOWNGRADE,
-              lf->getInputSchemaState());
+              lf.getInputSchemaState());
 
 
     Lease6Ptr lease;
     {
     SCOPED_TRACE("First lease valid");
-    EXPECT_TRUE(lf->next(lease));
+    EXPECT_TRUE(lf.next(lease));
     ASSERT_TRUE(lease);
 
     // Verify that the lease attributes are correct.

src/lib/util/csv_file.cc

@@ -68,7 +68,7 @@ CSVRow::writeAt(const size_t at, const char* value) {
 void
 CSVRow::trim(const size_t count) {
     checkIndex(count);
-    values_.erase(values_.end() - count, values_.end());
+    values_.resize(values_.size() - count);
 }
 
 std::ostream& operator<<(std::ostream& os, const CSVRow& row) {

src/lib/util/versioned_csv_file.cc

@@ -82,8 +82,7 @@ VersionedCSVFile::recreate() {
 
     CSVFile::recreate();
     // For new files they always match.
-    valid_column_count_ = getColumnCount();
-    input_header_count_ = getColumnCount();
+    input_header_count_ = valid_column_count_ = getColumnCount();
 }
 
 VersionedCSVFile::InputSchemaState
@@ -127,6 +126,7 @@ VersionedCSVFile::getVersionedColumn(const size_t index) const {
 
 bool
 VersionedCSVFile::next(CSVRow& row) {
+    setReadMsg("success");
     // Use base class to physical read the row, but skip its row
     // validation
     CSVFile::next(row, true);
@@ -134,44 +134,59 @@ VersionedCSVFile::next(CSVRow& row) {
         return(true);
     }
 
-    // If we're upgrading, valid_column_count_ will be less than
-    // defined column count.  If not they're the equal.  Either way
-    // each data row must have valid_column_count_ values or its
-    // an invalid row.
-    if (row.getValuesCount() < getValidColumnCount()) {
-        std::ostringstream s;
-        s << " The row '" << row << "' has too few valid columns "
-          << getValidColumnCount() << "' of the CSV file '"
-          << getFilename() << "'";
-        setReadMsg(s.str());
-        return (false);
+    bool row_valid = true;
+    switch(getInputSchemaState()) {
+        case CURRENT:
+            // All rows must match than the current schema
+            if (row.getValuesCount() != getColumnCount()) {
+                columnCountError(row, "must match current schema");
+                row_valid = false;
+            }
+            break;
+
+        case NEEDS_UPGRADE:
+            // Rows must be at least as long as header but not longer
+            // than the current schema
+            if (row.getValuesCount() < getValidColumnCount()) {
+                columnCountError(row, "too few columns to upgrade");
+                row_valid = false;
+            } else if (row.getValuesCount() > getColumnCount()) {
+                columnCountError(row, "too many columns to upgrade");
+                row_valid = false;
+            } else {
+                // Add any missing values
+                for (size_t index = row.getValuesCount();
+                     index < getColumnCount(); ++index) {
+                    row.append(columns_[index]->default_value_);
+                }
+            }
+            break;
+
+        case NEEDS_DOWNGRADE:
+            // Rows may be as long as header but not shorter than
+            // the the current schema
+            if (row.getValuesCount() < getColumnCount()) {
+                columnCountError(row, "too few columns to downgrade");
+            } else if (row.getValuesCount() > getInputHeaderCount()) {
+                columnCountError(row, "too many columns to downgrade");
+            } else {
+                // Toss any the extra columns
+                row.trim(row.getValuesCount() - getColumnCount());
+            }
+            break;
     }
 
-    // If we have more values than columns defined, we need to
-    // check if we should "downgrade" the row. We will if the
-    // number of values we have matches the number of columns in
-    // input header.  If now we'll toss the row.
-    if (row.getValuesCount() > getColumnCount()) {
-        if (row.getValuesCount() != getInputHeaderCount()) {
-            std::ostringstream s;
-            s << " The row '" << row << "' has too many columns "
-              << getValidColumnCount() << "' of the CSV file '"
-              << getFilename() << "'";
-              setReadMsg(s.str());
-            return (false);
-        }
-
-        // We're downgrading a row, so toss the extra columns
-        row.trim(row.getValuesCount() - getColumnCount());
-    } else {
-        // If we're upgrading, we need to add in any missing values
-        for (size_t index = row.getValuesCount(); index < getColumnCount();
-            ++index) {
-            row.append(columns_[index]->default_value_);
-        }
-    }
+    return (row_valid);
+}
 
-    return (true);
+void
+VersionedCSVFile::columnCountError(const CSVRow& row,
+                                  const std::string& reason) {
+    std::ostringstream s;
+    s <<  "Invalid number of columns: "
+      << row.getValuesCount()  << " in row: '" << row
+      << "', file: '" << getFilename() << "' : " << reason;
+      setReadMsg(s.str());
 }
 
 bool

src/lib/util/versioned_csv_file.h

@@ -89,7 +89,7 @@ typedef boost::shared_ptr<VersionedColumn> VersionedColumnPtr;
 /// is presumed to be an earlier schema version and will be upgraded as it is
 /// read.  There is an ability to mark a specific column as being the minimum
 /// column which must be present, see @ref VersionedCSVFile::
-/// setMinimumValidColumns().  If the header does contain match up to this
+/// setMinimumValidColumns().  If the header columns do not match up to this
 /// minimum column, the file is presumed to be too old to upgrade and the
 /// open will fail.  A valid, upgradable file will have an input schema
 /// state of VersionedCSVFile::NEEDS_UPGRADE.
@@ -109,10 +109,11 @@ typedef boost::shared_ptr<VersionedColumn> VersionedColumnPtr;
 /// VersionedCSVFile::NEEDS_DOWNGRADE.
 ///
 /// After successfully opening a file,  rows are read one at a time via
-/// @ref VersionedCSVFile::next().  Each data row is expected to have at least
-/// the same number of columns as were found in the header. Any row which as
-/// fewer values is discarded as invalid.  Similarly, any row which is found
-/// to have more values than were found in the header is discarded as invalid.
+/// @ref VersionedCSVFile::next() and handled according to the input schema
+/// state.   Each data row is expected to have at least the same number of
+/// columns as were found in the header. Any row which as fewer values is
+/// discarded as invalid.  Similarly, any row which is found to have more
+/// values than were found in the header is discarded as invalid.
 ///
 /// When upgrading a row, the values for each missing column is filled in
 /// with the default value specified by that column's descriptor.  When
@@ -172,6 +173,9 @@ public:
     /// for the file to be considered valid.
     size_t getMinimumValidColumns() const;
 
+    /// @brief Returns the number of columns found in the input header
+    size_t getInputHeaderCount() const;
+
     /// @brief Returns the number of valid columns found in the header
     /// For newly created files this will always match the number of defined
     /// columns (i.e. getColumnCount()).  For existing files, this will be
@@ -181,8 +185,6 @@ public:
     /// been opened.
     size_t getValidColumnCount() const;
 
-    size_t getInputHeaderCount() const;
-
     /// @brief Opens existing file or creates a new one.
     ///
     /// This function will try to open existing file if this file has size
@@ -286,6 +288,15 @@ protected:
     /// @return true if header matches the columns; false otherwise.
     virtual bool validateHeader(const CSVRow& header);
 
+    /// @brief Convenience method for adding an error message
+    ///
+    /// Constructs an error message indicating that the number of columns
+    /// in a given row are wrong and why, then adds it readMsg.
+    ///
+    /// @param row The row in error
+    /// @param reason An explanation as to why the row column count is wrong
+    void columnCountError(const CSVRow& row, const std::string& reason);
+
 private:
     /// @brief Holds the collection of column descriptors
     std::vector<VersionedColumnPtr> columns_;