More SQL statement usage regularization. Back-touch some

chrome/browser/net/sqlite_origin_bound_cert_store.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/net/sqlite_origin_bound_cert_store.h"
 
 #include <list>
 
 #include "base/basictypes.h"
 #include "base/bind.h"
@@ skipping 158 matching lines @@
     db_.reset();
     return false;
   }
 
   db_->Preload();
 
   // Slurp all the certs into the out-vector.
   sql::Statement smt(db_->GetUniqueStatement(
       "SELECT origin, private_key, cert, cert_type, expiration_time, "
       "creation_time FROM origin_bound_certs"));
-  if (!smt) {
-    NOTREACHED() << "select statement prep failed";
+  if (!smt.is_valid()) {
     db_.reset();
     return false;
   }
 
   while (smt.Step()) {
     std::string private_key_from_db, cert_from_db;
     smt.ColumnBlobAsString(1, &private_key_from_db);
     smt.ColumnBlobAsString(2, &cert_from_db);
     scoped_ptr<net::DefaultOriginBoundCertStore::OriginBoundCert> cert(
         new net::DefaultOriginBoundCertStore::OriginBoundCert(
@@ skipping 22 matching lines @@
   }
 
   int cur_version = meta_table_.GetVersionNumber();
   if (cur_version == 1) {
     sql::Transaction transaction(db_.get());
     if (!transaction.Begin())
       return false;
     if (!db_->Execute("ALTER TABLE origin_bound_certs ADD COLUMN cert_type "
                       "INTEGER")) {
       LOG(WARNING) << "Unable to update origin bound cert database to "
                    << "version 2.";

[Scott Hess - ex-Googler, 2012/02/14 00:28:18]

This was the one I was mentioning.  Now it's right again!

       return false;
     }
     // All certs in version 1 database are rsa_sign, which has a value of 1.
     if (!db_->Execute("UPDATE origin_bound_certs SET cert_type = 1")) {
       LOG(WARNING) << "Unable to update origin bound cert database to "
                    << "version 2.";
       return false;
     }
     ++cur_version;
     meta_table_.SetVersionNumber(cur_version);
     meta_table_.SetCompatibleVersionNumber(
         std::min(cur_version, kCompatibleVersionNumber));
     transaction.Commit();
   }
 
   if (cur_version <= 3) {
     sql::Transaction transaction(db_.get());
     if (!transaction.Begin())
       return false;
 
     if (cur_version == 2) {
       if (!db_->Execute("ALTER TABLE origin_bound_certs ADD COLUMN "
                         "expiration_time INTEGER")) {
         LOG(WARNING) << "Unable to update origin bound cert database to "
-                     << "version 4.";
+                     << "version 3.";

[Scott Hess - ex-Googler, 2012/02/14 00:28:18]

But now this is back to wtc's earlier comment that it should be version 4
(because the if() which gets to here upgrades to version 4).

[Greg Billock, 2012/02/14 01:13:44]

My head hurts on this -- it looks like we're upgrading from 2->3, and then the
ALTER TABLE below from 3->4. I don't really understand the archaeology, though,
so I'm happy to put it to what it was.

On 2012/02/14 00:28:18, shess wrote:

[Scott Hess - ex-Googler, 2012/02/14 01:28:36]

My theory is that at some point someone added the 3-to-4 upgrade by modifying
the 2-to-3 upgrade, so now it's (2-or-3)-to-4.  But I think the relevant version
is the version things get set to on commit. (4).

On 2012/02/14 01:13:44, Greg Billock wrote:

         return false;
       }
     }
 
     if (!db_->Execute("ALTER TABLE origin_bound_certs ADD COLUMN "
                       "creation_time INTEGER")) {
       LOG(WARNING) << "Unable to update origin bound cert database to "
                    << "version 4.";
       return false;
     }
 
     sql::Statement smt(db_->GetUniqueStatement(
         "SELECT origin, cert FROM origin_bound_certs"));
     sql::Statement update_expires_smt(db_->GetUniqueStatement(
         "UPDATE origin_bound_certs SET expiration_time = ? WHERE origin = ?"));
     sql::Statement update_creation_smt(db_->GetUniqueStatement(
         "UPDATE origin_bound_certs SET creation_time = ? WHERE origin = ?"));
-    if (!smt || !update_expires_smt || !update_creation_smt) {
+    if (!smt.is_valid() ||
+        !update_expires_smt.is_valid() ||
+        !update_creation_smt.is_valid()) {
       LOG(WARNING) << "Unable to update origin bound cert database to "
                    << "version 4.";
       return false;
     }
+
+
     while (smt.Step()) {
       std::string origin = smt.ColumnString(0);
       std::string cert_from_db;
       smt.ColumnBlobAsString(1, &cert_from_db);
       // Parse the cert and extract the real value and then update the DB.
       scoped_refptr<net::X509Certificate> cert(
           net::X509Certificate::CreateFromBytes(
               cert_from_db.data(), cert_from_db.size()));
       if (cert) {
         if (cur_version == 2) {
           update_expires_smt.Reset();
           update_expires_smt.BindInt64(0,
                                        cert->valid_expiry().ToInternalValue());
           update_expires_smt.BindString(1, origin);
           if (!update_expires_smt.Run()) {
             LOG(WARNING) << "Unable to update origin bound cert database to "
-                         << "version 4.";
+                         << "version 3.";

[Scott Hess - ex-Googler, 2012/02/14 00:28:18]

And this one to version 4.

[Greg Billock, 2012/02/14 01:13:44]

Done.

             return false;
           }
         }
 
         update_creation_smt.Reset();
         update_creation_smt.BindInt64(0, cert->valid_start().ToInternalValue());
         update_creation_smt.BindString(1, origin);
         if (!update_creation_smt.Run()) {
           LOG(WARNING) << "Unable to update origin bound cert database to "
                        << "version 4.";
@@ skipping 77 matching lines @@
     num_pending_ = 0;
   }
 
   // Maybe an old timer fired or we are already Close()'ed.
   if (!db_.get() || ops.empty())
     return;
 
   sql::Statement add_smt(db_->GetCachedStatement(SQL_FROM_HERE,
       "INSERT INTO origin_bound_certs (origin, private_key, cert, cert_type, "
       "expiration_time) VALUES (?,?,?,?,?)"));
-  if (!add_smt) {
-    NOTREACHED();
+  if (!add_smt.is_valid())
     return;
-  }
 
   sql::Statement del_smt(db_->GetCachedStatement(SQL_FROM_HERE,
                              "DELETE FROM origin_bound_certs WHERE origin=?"));
-  if (!del_smt) {
-    NOTREACHED();
+  if (!del_smt.is_valid())
     return;
-  }
 
   sql::Transaction transaction(db_.get());
-  if (!transaction.Begin()) {
-    NOTREACHED();
+  if (!transaction.Begin())
     return;
-  }
+
   for (PendingOperationsList::iterator it = ops.begin();
        it != ops.end(); ++it) {
     // Free the certs as we commit them to the database.
     scoped_ptr<PendingOperation> po(*it);
     switch (po->op()) {
       case PendingOperation::CERT_ADD: {
         add_smt.Reset();
         add_smt.BindString(0, po->cert().origin());
         const std::string& private_key = po->cert().private_key();
         add_smt.BindBlob(1, private_key.data(), private_key.size());
@@ skipping 96 matching lines @@
   if (backend_.get())
     backend_->SetClearLocalStateOnExit(clear_local_state);
 }
 
 void SQLiteOriginBoundCertStore::Flush(const base::Closure& completion_task) {
   if (backend_.get())
     backend_->Flush(completion_task);
   else if (!completion_task.is_null())
     MessageLoop::current()->PostTask(FROM_HERE, completion_task);
 }