[master] Merge branch 'trac2541'

src/lib/datasrc/Makefile.am

@@ -31,7 +31,7 @@ libb10_datasrc_la_SOURCES += zonetable.h zonetable.cc
 libb10_datasrc_la_SOURCES += zone.h zone_finder.cc zone_finder_context.cc
 libb10_datasrc_la_SOURCES += result.h
 libb10_datasrc_la_SOURCES += logger.h logger.cc
-libb10_datasrc_la_SOURCES += client.h iterator.h
+libb10_datasrc_la_SOURCES += client.h client.cc iterator.h
 libb10_datasrc_la_SOURCES += database.h database.cc
 libb10_datasrc_la_SOURCES += factory.h factory.cc
 libb10_datasrc_la_SOURCES += client_list.h client_list.cc

src/lib/datasrc/client.cc

@@ -0,0 +1,48 @@
+// Copyright (C) 2012  Internet Systems Consortium, Inc. ("ISC")
+//
+// Permission to use, copy, modify, and/or distribute this software for any
+// purpose with or without fee is hereby granted, provided that the above
+// copyright notice and this permission notice appear in all copies.
+//
+// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
+// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
+// AND FITNESS.  IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
+// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
+// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
+// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
+// PERFORMANCE OF THIS SOFTWARE.
+
+#include <datasrc/client.h>
+
+#include <exceptions/exceptions.h>
+
+/// This file defines a few default implementations for methods.
+///
+/// While some of the detail of the API are worked out, we define
+/// default implementations to ease development for some of the
+/// more tentative methods (those that are not (yet) pure virtual)
+/// They should all throw NotImplemented
+
+namespace isc {
+namespace datasrc {
+
+ZoneIteratorPtr
+DataSourceClient::getIterator(const isc::dns::Name&, bool) const {
+    isc_throw(isc::NotImplemented,
+              "Data source doesn't support iteration");
+}
+
+unsigned int
+DataSourceClient::getZoneCount() const {
+    isc_throw(isc::NotImplemented,
+              "Data source doesn't support getZoneCount");
+}
+
+bool
+DataSourceClient::createZone(const dns::Name&) {
+    isc_throw(isc::NotImplemented,
+              "Data source doesn't support addZone");
+}
+
+} // end namespace datasrc
+} // end namespace isc

src/lib/datasrc/client.h

@@ -20,8 +20,6 @@
 #include <boost/noncopyable.hpp>
 #include <boost/shared_ptr.hpp>
 
-#include <exceptions/exceptions.h>
-
 #include <datasrc/zone.h>
 
 /// \file
@@ -225,15 +223,7 @@ public:
     ///                     adjusted to the lowest one found.
     /// \return Pointer to the iterator.
     virtual ZoneIteratorPtr getIterator(const isc::dns::Name& name,
-                                        bool separate_rrs = false) const {
-        // This is here to both document the parameter in doxygen (therefore it
-        // needs a name) and avoid unused parameter warning.
-        static_cast<void>(name);
-        static_cast<void>(separate_rrs);
-
-        isc_throw(isc::NotImplemented,
-                  "Data source doesn't support iteration");
-    }
+                                        bool separate_rrs = false) const;
 
     /// Return an updater to make updates to a specific zone.
     ///
@@ -368,16 +358,36 @@ public:
     /// This is an optional convenience method, currently only implemented
     /// by the InMemory datasource. By default, it throws NotImplemented
     ///
+    /// \note This is a tentative API, and this method is likely to change
+    /// or be removed in the near future. For that reason, it currently
+    /// provides a default implementation that throws NotImplemented.
+    ///
     /// \exception NotImplemented Thrown if this method is not supported
     ///            by the datasource
     ///
-    /// \note This is a tentative API, and this method may likely to be
-    ///       removed in the near future.
     /// \return The number of zones known to this datasource
-    virtual unsigned int getZoneCount() const {
-        isc_throw(isc::NotImplemented,
-                  "Data source doesn't support getZoneCount");
-    }
+    virtual unsigned int getZoneCount() const;
+
+    /// \brief Create a zone in the database
+    ///
+    /// Creates a new (empty) zone in the data source backend, which
+    /// can subsequently be filled with data (through getUpdater()).
+    ///
+    /// \note This is a tentative API, and this method is likely to change
+    /// or be removed in the near future. For that reason, it currently
+    /// provides a default implementation that throws NotImplemented.
+    ///
+    /// Apart from the two exceptions mentioned below, in theory this
+    /// call can throw anything, depending on the implementation of
+    /// the datasource backend.
+    ///
+    /// \throw NotImplemented If the datasource backend does not support
+    ///                       direct zone creation.
+    /// \throw DataSourceError If something goes wrong in the data source
+    ///                        while creating the zone.
+    /// \param name The (fully qualified) name of the zone to create
+    /// \return True if the zone was added, false if it already existed
+    virtual bool createZone(const dns::Name& name);
 };
 }
 }

src/lib/datasrc/database.cc

@@ -43,6 +43,42 @@ using boost::scoped_ptr;
 
 namespace isc {
 namespace datasrc {
+// RAII-style transaction holder; roll back the transaction unless explicitly
+// committed
+namespace {
+class TransactionHolder {
+public:
+    TransactionHolder(DatabaseAccessor& accessor) : accessor_(accessor),
+                                                    committed_(false)
+    {
+        accessor_.startTransaction();
+    }
+    ~TransactionHolder() {
+        if (!committed_) {
+            try {
+                accessor_.rollback();
+            } catch (const DataSourceError& e) {
+                // We generally expect that rollback always succeeds, and
+                // it should in fact succeed in a way we execute it.  But
+                // as the public API allows rollback() to fail and
+                // throw, we should expect it.  Obviously we cannot re-throw
+                // it.  The best we can do is to log it as a critical error.
+                logger.error(DATASRC_DATABASE_TRANSACTION_ROLLBACKFAIL).
+                            arg(accessor_.getDBName()).
+                            arg(e.what());
+            }
+        }
+    }
+    void commit() {
+        accessor_.commit();
+        committed_ = true;
+    }
+private:
+    DatabaseAccessor& accessor_;
+    bool committed_;
+};
+} // end unnamed namespace
+
 
 DatabaseClient::DatabaseClient(RRClass rrclass,
                                boost::shared_ptr<DatabaseAccessor>
@@ -80,6 +116,18 @@ DatabaseClient::findZone(const Name& name) const {
     return (FindResult(result::NOTFOUND, ZoneFinderPtr()));
 }
 
+bool
+DatabaseClient::createZone(const Name& name) {
+    TransactionHolder transaction(*accessor_);
+    std::pair<bool, int> zone(accessor_->getZone(name.toText()));
+    if (zone.first) {
+        return (false);
+    }
+    accessor_->addZone(name.toText());
+    transaction.commit();
+    return (true);
+}
+
 DatabaseClient::Finder::Finder(boost::shared_ptr<DatabaseAccessor> accessor,
                                int zone_id, const isc::dns::Name& origin) :
     accessor_(accessor),
@@ -1349,11 +1397,8 @@ public:
                 logger.info(DATASRC_DATABASE_UPDATER_ROLLBACK)
                     .arg(zone_name_).arg(zone_class_).arg(db_name_);
             } catch (const DataSourceError& e) {
-                // We generally expect that rollback always succeeds, and
-                // it should in fact succeed in a way we execute it.  But
-                // as the public API allows rollback() to fail and
-                // throw, we should expect it.  Obviously we cannot re-throw
-                // it.  The best we can do is to log it as a critical error.
+                // See The destructor ~TransactionHolder() for the
+                // reason to catch this.
                 logger.error(DATASRC_DATABASE_UPDATER_ROLLBACKFAIL)
                     .arg(zone_name_).arg(zone_class_).arg(db_name_)
                     .arg(e.what());

src/lib/datasrc/database.h

@@ -177,6 +177,24 @@ public:
     ///     an opaque handle.
     virtual std::pair<bool, int> getZone(const std::string& name) const = 0;
 
+    /// \brief Add a new zone to the database
+    ///
+    /// This method creates a new (and empty) zone in the database.
+    ///
+    /// Like for addRecordToZone, implementations are not required to
+    /// check for the existence of the given zone name, it is the
+    /// responsibility of the caller to do so.
+    ///
+    /// Callers must also start a transaction before calling this method,
+    /// implementations should throw DataSourceError if this has not been
+    /// done. Callers should also expect DataSourceErrors for other potential
+    /// problems.
+    ///
+    /// \param name The (fully qualified) domain name of the zone to add.
+    /// \return The internal zone id of the zone (whether is existed already
+    ///         or was created by this call).
+    virtual int addZone(const std::string& name) = 0;
+
     /// \brief This holds the internal context of ZoneIterator for databases
     ///
     /// While the ZoneIterator implementation from DatabaseClient does all the
@@ -1373,6 +1391,16 @@ public:
     ///     should use it as a ZoneFinder only.
     virtual FindResult findZone(const isc::dns::Name& name) const;
 
+    /// \brief Create a zone in the database
+    ///
+    /// This method implements \c DataSourceClient::createZone()
+    ///
+    /// It starts a transaction, checks if the zone exists, and if it
+    /// does not, creates it, commits, and returns true. If the zone
+    /// does exist already, it does nothing (except abort the transaction)
+    /// and returns false.
+    virtual bool createZone(const isc::dns::Name& name);
+
     /// \brief Get the zone iterator
     ///
     /// The iterator allows going through the whole zone content. If the

src/lib/datasrc/datasrc_messages.mes

@@ -216,6 +216,16 @@ to find any invalid data and fix it.
 No match (not even a wildcard) was found in the named data source for the given
 name/type/class in the data source.
 
+% DATASRC_DATABASE_TRANSACTION_ROLLBACKFAIL failed to roll back transaction on %1: %2
+A transaction on the database was rolled back without committing the
+changes to the database, but the rollback itself unexpectedly fails.
+The higher level implementation does not expect it to fail, so this means
+either a serious operational error in the underlying data source (such as a
+system failure of a database) or software bug in the underlying data source
+implementation.  In either case if this message is logged the administrator
+should carefully examine the underlying data source to see what exactly
+happens and whether the data is still valid.
+
 % DATASRC_DATABASE_UPDATER_COMMIT updates committed for '%1/%2' on %3
 Debug information.  A set of updates to a zone has been successfully
 committed to the corresponding database backend.  The zone name,

src/lib/datasrc/sqlite3_accessor.cc

@@ -78,7 +78,8 @@ enum StatementID {
     ADD_NSEC3_RECORD = 19,
     DEL_ZONE_NSEC3_RECORDS = 20,
     DEL_NSEC3_RECORD = 21,
-    NUM_STATEMENTS = 22
+    ADD_ZONE = 22,
+    NUM_STATEMENTS = 23
 };
 
 const char* const text_statements[NUM_STATEMENTS] = {
@@ -161,7 +162,10 @@ const char* const text_statements[NUM_STATEMENTS] = {
     "DELETE FROM nsec3 WHERE zone_id=?1",
     // DEL_NSEC3_RECORD: delete specified NSEC3-related records
     "DELETE FROM nsec3 WHERE zone_id=?1 AND hash=?2 "
-    "AND rdtype=?3 AND rdata=?4"
+    "AND rdtype=?3 AND rdata=?4",
+
+    // ADD_ZONE: add a zone to the zones table
+    "INSERT INTO zones (name, rdclass) VALUES (?1, ?2)" // ADD_ZONE
 };
 
 struct SQLite3Parameters {
@@ -612,6 +616,33 @@ SQLite3Accessor::getZone(const std::string& name) const {
     return (std::pair<bool, int>(false, 0));
 }
 
+int
+SQLite3Accessor::addZone(const std::string& name) {
+    // Transaction should have been started by the caller
+    if (!dbparameters_->in_transaction) {
+        isc_throw(DataSourceError, "performing addZone on SQLite3 "
+                  "data source without transaction");
+    }
+
+    StatementProcessor proc(*dbparameters_, ADD_ZONE, "add zone");
+    // note: using TRANSIENT here, STATIC would be slightly more
+    // efficient, but TRANSIENT is safer and performance is not
+    // as important in this specific code path.
+    proc.bindText(1, name.c_str(), SQLITE_TRANSIENT);
+    proc.bindText(2, class_.c_str(), SQLITE_TRANSIENT);
+    proc.exec();
+
+    // There are tricks to getting this in one go, but it is safer
+    // to do a new lookup (sqlite3_last_insert_rowid is unsafe
+    // regarding threads and triggers). This requires two
+    // statements, and is unpredictable in the case a zone is added
+    // twice, but this method assumes the caller does not do that
+    // anyway
+    std::pair<bool, int> getzone_result = getZone(name);
+    assert(getzone_result.first);
+    return (getzone_result.second);
+}
+
 namespace {
 
 // Conversion to plain char

src/lib/datasrc/sqlite3_accessor.h

@@ -34,13 +34,11 @@ class RRClass;
 
 namespace datasrc {
 
-/**
- * \brief Low-level database error
- *
- * This exception is thrown when the SQLite library complains about something.
- * It might mean corrupt database file, invalid request or that something is
- * rotten in the library.
- */
+/// \brief Low-level database error
+///
+/// This exception is thrown when the SQLite library complains about something.
+/// It might mean corrupt database file, invalid request or that something is
+/// rotten in the library.
 class SQLite3Error : public DataSourceError {
 public:
     SQLite3Error(const char* file, size_t line, const char* what) :
@@ -53,24 +51,20 @@ public:
         isc::Exception(file, line, what) {}
 };
 
-/**
- * \brief Too Much Data
- *
- * Thrown if a query expecting a certain number of rows back returned too
- * many rows.
- */
+/// \brief Too Much Data
+///
+/// Thrown if a query expecting a certain number of rows back returned too
+/// many rows.
 class TooMuchData : public DataSourceError {
 public:
     TooMuchData(const char* file, size_t line, const char* what) :
         DataSourceError(file, line, what) {}
 };
 
-/**
- * \brief Too Little Data
- *
- * Thrown if a query expecting a certain number of rows back returned too
- * few rows (including none).
- */
+/// \brief Too Little Data
+///
+/// Thrown if a query expecting a certain number of rows back returned too
+/// few rows (including none).
 class TooLittleData : public DataSourceError {
 public:
     TooLittleData(const char* file, size_t line, const char* what) :
@@ -79,70 +73,83 @@ public:
 
 struct SQLite3Parameters;
 
-/**
- * \brief Concrete implementation of DatabaseAccessor for SQLite3 databases
- *
- * This opens one database file with our schema and serves data from there.
- * According to the design, it doesn't interpret the data in any way, it just
- * provides unified access to the DB.
- */
+/// \brief Concrete implementation of DatabaseAccessor for SQLite3 databases
+///
+/// This opens one database file with our schema and serves data from there.
+/// According to the design, it doesn't interpret the data in any way, it just
+/// provides unified access to the DB.
 class SQLite3Accessor : public DatabaseAccessor,
     public boost::enable_shared_from_this<SQLite3Accessor> {
 public:
-    /**
-     * \brief Constructor
-     *
-     * This opens the database and becomes ready to serve data from there.
-     *
-     * \exception SQLite3Error will be thrown if the given database file
-     * doesn't work (it is broken, doesn't exist and can't be created, etc).
-     *
-     * \param filename The database file to be used.
-     * \param rrclass Textual representation of RR class ("IN", "CH", etc),
-     *     specifying which class of data it should serve (while the database
-     *     file can contain multiple classes of data, a single accessor can
-     *     work with only one class).
-     */
+    /// \brief Constructor
+    ///
+    /// This opens the database and becomes ready to serve data from there.
+    ///
+    /// \exception SQLite3Error will be thrown if the given database file
+    /// doesn't work (it is broken, doesn't exist and can't be created, etc).
+    ///
+    /// \param filename The database file to be used.
+    /// \param rrclass Textual representation of RR class ("IN", "CH", etc),
+    ///    specifying which class of data it should serve (while the database
+    ///    file can contain multiple classes of data, a single accessor can
+    ///    work with only one class).
     SQLite3Accessor(const std::string& filename, const std::string& rrclass);
 
-    /**
-     * \brief Destructor
-     *
-     * Closes the database.
-     */
-    ~SQLite3Accessor();
+    /// \brief Destructor
+    ///
+    /// Closes the database.
+    virtual ~SQLite3Accessor();
 
     /// This implementation internally opens a new sqlite3 database for the
     /// same file name specified in the constructor of the original accessor.
     virtual boost::shared_ptr<DatabaseAccessor> clone();
 
-    /**
-     * \brief Look up a zone
-     *
-     * This implements the getZone from DatabaseAccessor and looks up a zone
-     * in the data. It looks for a zone with the exact given origin and class
-     * passed to the constructor.
-     *
-     * \exception SQLite3Error if something about the database is broken.
-     *
-     * \param name The (fully qualified) domain name of zone to look up
-     * \return The pair contains if the lookup was successful in the first
-     *     element and the zone id in the second if it was.
-     */
+    /// \brief Look up a zone
+    ///
+    /// This implements the getZone from DatabaseAccessor and looks up a zone
+    /// in the data. It looks for a zone with the exact given origin and class
+    /// passed to the constructor.
+    ///
+    /// \exception SQLite3Error if something about the database is broken.
+    ///
+    /// \param name The (fully qualified) domain name of zone to look up
+    /// \return The pair contains if the lookup was successful in the first
+    ///    element and the zone id in the second if it was.
     virtual std::pair<bool, int> getZone(const std::string& name) const;
 
-    /** \brief Look up all resource records for a name
-     *
-     * This implements the getRecords() method from DatabaseAccessor
-     *
-     * \exception SQLite3Error if there is an sqlite3 error when performing
-     *                         the query
-     *
-     * \param name the name to look up
-     * \param id the zone id, as returned by getZone()
-     * \param subdomains Match subdomains instead of the name.
-     * \return Iterator that contains all records with the given name
-     */
+    /// \brief Add a zone
+    ///
+    /// This implements the addZone from DatabaseAccessor and adds a zone
+    /// into the zones table. If the zone exists already, it is still added,
+    /// so the caller should make sure this does not happen (by making
+    /// sure the zone does not exist). In the case of duplicate addition,
+    /// it is undefined which zone id is returned.
+    ///
+    /// The class of the newly created zone is the class passed at construction
+    /// time of the accessor.
+    ///
+    /// This method requires a transaction has been started (with
+    /// \c beginTransaction) by the caller.
+    ///
+    /// \exception DataSourceError if no transaction is active, or if there
+    ///                           is an SQLite3 error when performing the
+    ///                           queries.
+    ///
+    /// \param name The origin name of the zone to add
+    /// \return the id of the zone that has been added
+    virtual int addZone(const std::string& name);
+
+    /// \brief Look up all resource records for a name
+    ///
+    /// This implements the getRecords() method from DatabaseAccessor
+    ///
+    /// \exception SQLite3Error if there is an sqlite3 error when performing
+    ///                        the query
+    ///
+    /// \param name the name to look up
+    /// \param id the zone id, as returned by getZone()
+    /// \param subdomains Match subdomains instead of the name.
+    /// \return Iterator that contains all records with the given name
     virtual IteratorContextPtr getRecords(const std::string& name,
                                           int id,
                                           bool subdomains = false) const;
@@ -155,35 +162,33 @@ public:
     virtual IteratorContextPtr getNSEC3Records(const std::string& hash,
                                                int id) const;
 
-    /** \brief Look up all resource records for a zone
-     *
-     * This implements the getRecords() method from DatabaseAccessor
-     *
-     * \exception SQLite3Error if there is an sqlite3 error when performing
-     *                         the query
-     *
-     * \param id the zone id, as returned by getZone()
-     * \return Iterator that contains all records in the given zone
-     */
+    /// \brief Look up all resource records for a zone
+    ///
+    /// This implements the getRecords() method from DatabaseAccessor
+    ///
+    /// \exception SQLite3Error if there is an sqlite3 error when performing
+    ///                        the query
+    ///
+    /// \param id the zone id, as returned by getZone()
+    /// \return Iterator that contains all records in the given zone
     virtual IteratorContextPtr getAllRecords(int id) const;
 
-    /** \brief Creates an iterator context for a set of differences.
-     *
-     * Implements the getDiffs() method from DatabaseAccessor
-     *
-     * \exception NoSuchSerial if either of the versions do not exist in
-     *            the difference table.
-     * \exception SQLite3Error if there is an sqlite3 error when performing
-     *            the query
-     *
-     * \param id The ID of the zone, returned from getZone().
-     * \param start The SOA serial number of the version of the zone from
-     *        which the difference sequence should start.
-     * \param end The SOA serial number of the version of the zone at which
-     *        the difference sequence should end.
-     *
-     * \return Iterator containing difference records.
-     */
+    /// \brief Creates an iterator context for a set of differences.
+    ///
+    /// Implements the getDiffs() method from DatabaseAccessor
+    ///
+    /// \exception NoSuchSerial if either of the versions do not exist in
+    ///           the difference table.
+    /// \exception SQLite3Error if there is an sqlite3 error when performing
+    ///           the query
+    ///
+    /// \param id The ID of the zone, returned from getZone().
+    /// \param start The SOA serial number of the version of the zone from
+    ///       which the difference sequence should start.
+    /// \param end The SOA serial number of the version of the zone at which
+    ///       the difference sequence should end.
+    ///
+    /// \return Iterator containing difference records.
     virtual IteratorContextPtr
     getDiffs(int id, uint32_t start, uint32_t end) const;
 

src/lib/datasrc/tests/client_unittest.cc

@@ -60,4 +60,8 @@ TEST_F(ClientTest, defaultGetZoneCount) {
     EXPECT_THROW(client_.getZoneCount(), isc::NotImplemented);
 }
 
+TEST_F(ClientTest, defaultCreateZone) {
+    EXPECT_THROW(client_.createZone(Name("example.com.")), isc::NotImplemented);
+}
+
 }

src/lib/datasrc/tests/database_unittest.cc

@@ -274,6 +274,11 @@ public:
         }
     }
 
+    virtual int addZone(const std::string&) {
+        isc_throw(isc::NotImplemented,
+                  "This database datasource can't add zones");
+    }
+
     virtual boost::shared_ptr<DatabaseAccessor> clone() {
         // This accessor is stateless, so we can simply return a new instance.
         return (boost::shared_ptr<DatabaseAccessor>(new NopAccessor));
@@ -329,6 +334,7 @@ public:
         isc_throw(isc::NotImplemented,
                   "This test database knows nothing about NSEC3 nor order");
     }
+
 private:
     const std::string database_name_;
 
@@ -1780,7 +1786,7 @@ doFindAllTestResult(ZoneFinder& finder, const isc::dns::Name& name,
               expected_name, result->rrset->getName());
 }
 
-// When asking for an RRset where RRs somehow have different TTLs, it should 
+// When asking for an RRset where RRs somehow have different TTLs, it should
 // convert to the lowest one.
 TEST_F(MockDatabaseClientTest, ttldiff) {
     ZoneIteratorPtr it(this->client_->getIterator(Name("example.org")));
@@ -4092,4 +4098,57 @@ TYPED_TEST(DatabaseClientTest, findNSEC3) {
     performNSEC3Test(*finder, true);
 }
 
+TYPED_TEST(DatabaseClientTest, createZone) {
+    const Name new_name("example.com");
+    const DataSourceClient::FindResult
+        zone(this->client_->findZone(new_name));
+    ASSERT_EQ(result::NOTFOUND, zone.code);
+
+    // The mock implementation does not do createZone,
+    // in which case it should throw NotImplemented (from
+    // the base class)
+    if (this->is_mock_) {
+        ASSERT_THROW(this->client_->createZone(new_name), isc::NotImplemented);
+    } else {
+        // But in the real case, it should work and return true
+        ASSERT_TRUE(this->client_->createZone(new_name));
+        const DataSourceClient::FindResult
+            zone2(this->client_->findZone(new_name));
+        ASSERT_EQ(result::SUCCESS, zone2.code);
+        // And the second call should return false since
+        // it already exists
+        ASSERT_FALSE(this->client_->createZone(new_name));
+    }
+}
+
+TYPED_TEST(DatabaseClientTest, createZoneRollbackOnLocked) {
+    // skip test for mock
+    if (this->is_mock_) {
+        return;
+    }
+
+    const Name new_name("example.com");
+    isc::datasrc::ZoneUpdaterPtr updater =
+        this->client_->getUpdater(this->zname_, true);
+    ASSERT_THROW(this->client_->createZone(new_name), DataSourceError);
+    // createZone started a transaction as well, but since it failed,
+    // it should have been rolled back. Roll back the other one as
+    // well, and the next attempt should succeed
+    updater.reset();
+    ASSERT_TRUE(this->client_->createZone(new_name));
+}
+
+TYPED_TEST(DatabaseClientTest, createZoneRollbackOnExists) {
+    // skip test for mock
+    if (this->is_mock_) {
+        return;
+    }
+
+    const Name new_name("example.com");
+    ASSERT_FALSE(this->client_->createZone(this->zname_));
+    // createZone started a transaction, but since it failed,
+    // it should have been rolled back, and the next attempt should succeed
+    ASSERT_TRUE(this->client_->createZone(new_name));
+}
+
 }

src/lib/datasrc/tests/sqlite3_accessor_unittest.cc

@@ -667,6 +667,64 @@ TEST_F(SQLite3Create, creationtest) {
     ASSERT_TRUE(isReadable(SQLITE_NEW_DBFILE));
 }
 
+// Test addZone works. This is done on the 'createtest' fixture so we
+// can easily be sure it does not exist yet.
+TEST_F(SQLite3Create, addZone) {
+    // Need shared_ptr for the getAllRecords at the end of the test
+    boost::shared_ptr<SQLite3Accessor> accessor(
+        new SQLite3Accessor(SQLITE_NEW_DBFILE, "IN"));
+
+    const std::string zone_name("example.com");
+    EXPECT_FALSE(accessor->getZone(zone_name).first);
+
+    // Calling addZone without transaction should fail
+    EXPECT_THROW(accessor->addZone(zone_name), DataSourceError);
+
+    // Add the zone. Returns the new zone id
+    accessor->startTransaction();
+    const int new_zone_id = accessor->addZone(zone_name);
+    accessor->commit();
+
+    // Check that it exists now, but has no records at this point
+    const std::pair<bool, int> zone_info(accessor->getZone(zone_name));
+    ASSERT_TRUE(zone_info.first);
+    EXPECT_EQ(new_zone_id, zone_info.second);
+
+    DatabaseAccessor::IteratorContextPtr context =
+        accessor->getAllRecords(zone_info.second);
+    string data[DatabaseAccessor::COLUMN_COUNT];
+    EXPECT_NE(DatabaseAccessor::IteratorContextPtr(), context);
+    EXPECT_FALSE(context->getNext(data));
+}
+
+// Test addZone does not get confused with different classes
+TEST_F(SQLite3Create, addZoneDifferentClass) {
+    const std::string zone_name("example.com.");
+
+    // Add IN zone
+    boost::shared_ptr<SQLite3Accessor> accessor(
+        new SQLite3Accessor(SQLITE_NEW_DBFILE, "IN"));
+    accessor->startTransaction();
+    const int new_zone_id_IN = accessor->addZone(zone_name);
+    accessor->commit();
+
+    // Add CH zone
+    accessor.reset(new SQLite3Accessor(SQLITE_NEW_DBFILE, "CH"));
+    accessor->startTransaction();
+    const int new_zone_id_CH = accessor->addZone(zone_name);
+    accessor->commit();
+
+    // id's should differ
+    ASSERT_NE(new_zone_id_IN, new_zone_id_CH);
+
+    // Reopen the database for both classes, and make sure
+    // we get the right zone id on searches
+    accessor.reset(new SQLite3Accessor(SQLITE_NEW_DBFILE, "IN"));
+    EXPECT_EQ(new_zone_id_IN, accessor->getZone(zone_name).second);
+    accessor.reset(new SQLite3Accessor(SQLITE_NEW_DBFILE, "CH"));
+    EXPECT_EQ(new_zone_id_CH, accessor->getZone(zone_name).second);
+}
+
 TEST_F(SQLite3Create, emptytest) {
     ASSERT_FALSE(isReadable(SQLITE_NEW_DBFILE));
 
@@ -1538,4 +1596,23 @@ TEST_F(SQLite3Update, addDiffWithUpdate) {
 
     checkDiffs(expected_stored, accessor->getDiffs(zone_id, 1234, 1300));
 }
+
+TEST_F(SQLite3Update, addZoneWhileLocked) {
+    const std::string existing_zone = "example.com.";
+    const std::string new_zone = "example2.com.";
+
+    // Start 'replacing' an existing zone, it should lock the db
+    zone_id = accessor->startUpdateZone(existing_zone, true).second;
+
+    // addZone should throw an exception that it is locked
+    another_accessor->startTransaction();
+    EXPECT_THROW(another_accessor->addZone(new_zone), DataSourceError);
+    // Commit should do nothing, but not fail
+    another_accessor->commit();
+
+    accessor->rollback();
+    // New zone should not exist
+    EXPECT_FALSE(accessor->getZone(new_zone).first);
+}
+
 } // end anonymous namespace