Merge lp:~charlesk/keeper/keeper-tar-create-handle-bad-user-inputs-gracefully into lp:keeper

[Charles Kerr]

keeper-tar-create should fail gracefully if the user invokes without a busname or filenames.

keeper-tar-create should fail gracefully if the user invokes without a busname or filenames.

Add unit tests to confirm an error is returned in either of those two situations.

[lp:~charlesk/keeper/keeper-tar-create-handle-bad-user-inputs-gracefully]

61. By Charles Kerr on 2016-08-02

sync with trunk

[Xavi Garcia (xavi-garcia-mena)]

Looks like the branch should depend lp:~charlesk/keeper/service-dbusmock-should-notice-helper-failure....

Looks good to me, only one question about how it waits for stdin...

review: Needs Information

[lp:~charlesk/keeper/keeper-tar-create-handle-bad-user-inputs-gracefully]

62. By Charles Kerr on 2016-08-03

use QIODevice.waitForReadyready() instead of calling select() manually

63. By Charles Kerr on 2016-08-03

sync with trunk

64. By Charles Kerr on 2016-08-03

revert r62, we're not in a QApplication so waiting on Qt's event loop isn't as quick and easy as it seems

[Xavi Garcia (xavi-garcia-mena)]

LGTM

review: Approve

=== modified file 'src/tar/main.cpp'
--- src/tar/main.cpp	2016-07-13 02:20:24 +0000
+++ src/tar/main.cpp	2016-08-03 16:39:30 +0000
@@ -32,8 +32,10 @@
 
 #include <glib.h>
 
+#include <sys/select.h>
 #include <unistd.h>
 
+#include <cstdio> // fileno()
 #include <ctime>
 #include <iostream>
 #include <type_traits>
@@ -44,6 +46,20 @@
 QStringList
 get_filenames_from_file(FILE * fp, bool zero)
 {
+    // don't wait forever...
+    int fd = fileno(fp);
+    fd_set readfds;
+    FD_ZERO(&readfds);
+    FD_SET(fd, &readfds);
+    struct timeval tv {};
+    tv.tv_sec = 2;
+    select(1, &readfds, NULL, NULL, &tv);
+    if (!FD_ISSET(fd, &readfds)) {
+        qWarning() << "Couldn't read files from stdin";
+        return QStringList();
+    }
+
+    // read the file list
     QFile file;
     file.open(fp, QIODevice::ReadOnly);
     auto filenames_raw = file.readAll();

=== modified file 'src/tar/tar-creator.cpp'
--- src/tar/tar-creator.cpp	2016-07-22 23:36:45 +0000
+++ src/tar/tar-creator.cpp	2016-08-03 16:39:30 +0000
@@ -179,8 +179,13 @@
             ret = archive_write_header(archive, entry);
             if (ret==ARCHIVE_WARN)
                 qWarning() << archive_error_string(archive);
-            if (ret==ARCHIVE_FATAL)
-                qFatal("%s", archive_error_string(archive));
+            if (ret==ARCHIVE_FATAL) {
+                auto errstr = QString::fromUtf8("Error adding header for '%1': %2")
+                                .arg(filename)
+                                .arg(archive_error_string(archive));
+                qWarning() << qPrintable(errstr);
+                throw std::runtime_error(errstr.toStdString());
+            }
         } while (ret == ARCHIVE_RETRY);
 
         archive_entry_free(entry);

=== modified file 'tests/CMakeLists.txt'
--- tests/CMakeLists.txt	2016-08-02 09:14:49 +0000
+++ tests/CMakeLists.txt	2016-08-03 16:39:30 +0000
@@ -20,7 +20,6 @@
 )
 
 set(KEEPER_TAR_CREATE_BIN ${CMAKE_BINARY_DIR}/src/tar/keeper-tar-create)
-set(KEEPER_TAR_CREATE_INVOKE ${CMAKE_CURRENT_BINARY_DIR}/tar/keeper-tar-create-invoke.sh)
 set(KEPPER_HELPER_TEST_LOCATION ${CMAKE_BINARY_DIR}/tests/fakes/helpers-test.sh)
 
 add_definitions(
@@ -31,7 +30,6 @@
   -DKEEPER_CLIENT_BIN="${CMAKE_BINARY_DIR}/src/cli/keeper"
   -DTEST_SIMPLE_HELPER_SH="${KEPPER_HELPER_TEST_LOCATION}"
   -DKEEPER_TAR_CREATE_BIN="${KEEPER_TAR_CREATE_BIN}"
-  -DKEEPER_TAR_CREATE_INVOKE="${KEEPER_TAR_CREATE_INVOKE}"
 )
 
 set(SERVICE_TEMPLATE_FILE "${CMAKE_CURRENT_SOURCE_DIR}/com_canonical_keeper.py")

=== modified file 'tests/unit/CMakeLists.txt'
--- tests/unit/CMakeLists.txt	2016-07-31 17:20:13 +0000
+++ tests/unit/CMakeLists.txt	2016-08-03 16:39:30 +0000
@@ -8,11 +8,6 @@
   ${HELPERS_TEST_DEPS_INCLUDE_DIRS}
 )
 
-configure_file(
-  ${CMAKE_CURRENT_SOURCE_DIR}/tar/keeper-tar-create-invoke.sh.in
-  ${KEEPER_TAR_CREATE_INVOKE}
-)
-
 set(
   UNIT_TEST_LIBRARIES
   backup-helper

=== modified file 'tests/unit/tar/CMakeLists.txt'
--- tests/unit/tar/CMakeLists.txt	2016-07-31 17:20:13 +0000
+++ tests/unit/tar/CMakeLists.txt	2016-08-03 16:39:30 +0000
@@ -1,4 +1,40 @@
 #
+# create the wrapper scripts
+#
+
+set(
+  KTC_INVOKE
+  ${CMAKE_CURRENT_BINARY_DIR}/ktc-invoke.sh
+)
+configure_file(
+  ${CMAKE_CURRENT_SOURCE_DIR}/ktc-invoke.sh.in
+  ${KTC_INVOKE}
+)
+set(
+  KTC_INVOKE_NOBUS
+  ${CMAKE_CURRENT_BINARY_DIR}/ktc-invoke-nobus.sh
+)
+configure_file(
+  ${CMAKE_CURRENT_SOURCE_DIR}/ktc-invoke-nobus.sh.in
+  ${KTC_INVOKE_NOBUS}
+)
+set(
+  KTC_INVOKE_NOFILES
+  ${CMAKE_CURRENT_BINARY_DIR}/ktc-invoke-nofiles.sh
+)
+configure_file(
+  ${CMAKE_CURRENT_SOURCE_DIR}/ktc-invoke-nofiles.sh.in
+  ${KTC_INVOKE_NOFILES}
+)
+
+add_definitions(
+  -DKTC_INVOKE="${KTC_INVOKE}"
+  -DKTC_INVOKE_NOBUS="${KTC_INVOKE_NOBUS}"
+  -DKTC_INVOKE_NOFILES="${KTC_INVOKE_NOFILES}"
+)
+
+
+#
 # tar-creator-test
 #
 

=== modified file 'tests/unit/tar/keeper-tar-create-test.cpp'
--- tests/unit/tar/keeper-tar-create-test.cpp	2016-08-01 19:34:24 +0000
+++ tests/unit/tar/keeper-tar-create-test.cpp	2016-08-03 16:39:30 +0000
@@ -26,14 +26,34 @@
 #include <QTemporaryDir>
 
 
-using KeeperTarCreateFixture = KeeperDBusMockFixture;
+/***
+****
+***/
+
+class KeeperTarCreateFixture: public KeeperDBusMockFixture
+{
+    using parent = KeeperDBusMockFixture;
+
+    void SetUp() override
+    {
+        parent::SetUp();
+
+        qsrand(time(nullptr));
+    }
+
+    void TearDown() override
+    {
+    }
+};
+
+/***
+****
+***/
 
 TEST_F(KeeperTarCreateFixture, BackupRun)
 {
     static constexpr int n_runs {10};
 
-    qsrand(time(nullptr));
-
     for (int i=0; i<n_runs; ++i)
     {
         // build a directory full of random files
@@ -45,13 +65,13 @@
             { KEY_NAME, QDir(in.path()).dirName() },
             { KEY_TYPE, QStringLiteral("folder") },
             { KEY_SUBTYPE, in.path() },
-            { KEY_HELPER, QString::fromUtf8(KEEPER_TAR_CREATE_INVOKE) }
+            { KEY_HELPER, QString::fromUtf8(KTC_INVOKE) }
         });
 
         // start the backup
         QDBusReply<void> reply = user_iface_->call("StartBackup", QStringList{uuid});
         ASSERT_TRUE(reply.isValid()) << qPrintable(reply.error().message());
-        wait_for_tasks_to_finish();
+        ASSERT_TRUE(wait_for_tasks_to_finish());
 
         // ask keeper for the blob
         QDBusReply<QByteArray> blob = mock_iface_->call(QStringLiteral("GetBackupData"), uuid);
@@ -75,3 +95,65 @@
         EXPECT_TRUE(FileUtils::compareDirectories(in.path(), out.path()));
     }
 }
+
+/***
+****
+***/
+
+TEST_F(KeeperTarCreateFixture, BadArgNoBus)
+{
+    // build a directory full of random files
+    QTemporaryDir in;
+    FileUtils::fillTemporaryDirectory(in.path());
+
+    // tell keeper that's a backup choice
+    const auto uuid = add_backup_choice(QMap<QString,QVariant>{
+        { KEY_NAME, QDir(in.path()).dirName() },
+        { KEY_TYPE, QStringLiteral("folder") },
+        { KEY_SUBTYPE, in.path() },
+        { KEY_HELPER, QString::fromUtf8(KTC_INVOKE_NOBUS) }
+    });
+
+    // start the backup
+    QDBusReply<void> reply = user_iface_->call("StartBackup", QStringList{uuid});
+    ASSERT_TRUE(reply.isValid()) << qPrintable(reply.error().message());
+    EXPECT_TRUE(wait_for_tasks_to_finish());
+
+    // confirm that the backup ended in error
+    const auto state = user_iface_->state();
+    const auto& properties = state[uuid];
+    EXPECT_EQ(QString::fromUtf8("failed"), properties.value(KEY_ACTION))
+        << qPrintable(properties.value(KEY_ACTION).toString());
+    EXPECT_FALSE(properties.value(KEY_ERROR).toString().isEmpty());
+}
+
+/***
+****
+***/
+
+TEST_F(KeeperTarCreateFixture, BadArgNoFiles)
+{
+    // build a directory full of random files
+    QTemporaryDir in;
+    FileUtils::fillTemporaryDirectory(in.path());
+
+    // tell keeper that's a backup choice
+    const auto uuid = add_backup_choice(QMap<QString,QVariant>{
+        { KEY_NAME, QDir(in.path()).dirName() },
+        { KEY_TYPE, QStringLiteral("folder") },
+        { KEY_SUBTYPE, in.path() },
+        { KEY_HELPER, QString::fromUtf8(KTC_INVOKE_NOFILES) }
+    });
+
+    // start the backup
+    QDBusReply<void> reply = user_iface_->call("StartBackup", QStringList{uuid});
+    ASSERT_TRUE(reply.isValid()) << qPrintable(reply.error().message());
+    EXPECT_TRUE(wait_for_tasks_to_finish());
+
+    // confirm that the backup ended in error
+    const auto state = user_iface_->state();
+    const auto& properties = state[uuid];
+    EXPECT_EQ(QString::fromUtf8("failed"), properties.value(KEY_ACTION))
+        << qPrintable(properties.value(KEY_ACTION).toString());
+    EXPECT_FALSE(properties.value(KEY_ERROR).toString().isEmpty());
+}

=== added file 'tests/unit/tar/ktc-invoke-nobus.sh.in'
--- tests/unit/tar/ktc-invoke-nobus.sh.in	1970-01-01 00:00:00 +0000
+++ tests/unit/tar/ktc-invoke-nobus.sh.in	2016-08-03 16:39:30 +0000
@@ -0,0 +1,2 @@
+find ./ -type f
+find ./ -type f -print0 | @KEEPER_TAR_CREATE_BIN@ -0

=== added file 'tests/unit/tar/ktc-invoke-nofiles.sh.in'
--- tests/unit/tar/ktc-invoke-nofiles.sh.in	1970-01-01 00:00:00 +0000
+++ tests/unit/tar/ktc-invoke-nofiles.sh.in	2016-08-03 16:39:30 +0000
@@ -0,0 +1,1 @@
+@KEEPER_TAR_CREATE_BIN@ -0 -a /com/canonical/keeper/helper

=== renamed file 'tests/unit/tar/keeper-tar-create-invoke.sh.in' => 'tests/unit/tar/ktc-invoke.sh.in'
=== modified file 'tests/unit/tar/tar-creator-test.cpp'
--- tests/unit/tar/tar-creator-test.cpp	2016-07-22 13:25:07 +0000
+++ tests/unit/tar/tar-creator-test.cpp	2016-08-03 16:39:30 +0000
@@ -24,66 +24,92 @@
 #include <gtest/gtest.h>
 
 #include <QDebug>
+#include <QDir>
+#include <QFile>
+#include <QFileInfo>
+#include <QProcess>
 #include <QString>
 #include <QTemporaryDir>
 
 #include <algorithm>
+#include <array>
 #include <cstdio>
 
 class TarCreatorFixture: public ::testing::Test
 {
 protected:
 
+    void SetUp() override
+    {
+        qsrand(time(nullptr));
+    }
+
+    void TearDown() override
+    {
+    }
+
 };
 
-
-TEST_F(TarCreatorFixture, HelloWorld)
-{
-}
-
-TEST_F(TarCreatorFixture, CreateUncompressed)
-{
-    static constexpr int n_runs = 10;
-
-    qsrand(time(nullptr));
+/***
+****
+***/
+
+TEST_F(TarCreatorFixture, Create)
+{
+    static constexpr int n_runs {5};
 
     for (int i=0; i<n_runs; ++i)
     {
-        // build a directory full of random files
-        QTemporaryDir sandbox;
-        ASSERT_TRUE(FileUtils::fillTemporaryDirectory(sandbox.path()));
-        const auto files = FileUtils::getFilesRecursively(sandbox.path());
-
-        // create the tar creator
-        TarCreator tar_creator(files, false);
-
-        // simple sanity check on its size estimate
-        const auto filesize_sum = std::accumulate(files.begin(), files.end(), 0, [](ssize_t sum, QString const& filename){return sum + QFileInfo(filename).size();});
-
-#if 0
-ssize_t filesize_sum {};
-template< class InputIt, class T, class BinaryOperation >
-T accumulate( InputIt first, InputIt last, T init,
-              BinaryOperation op );
-        for (const auto& file : files)
-            filesize_sum += QFileInfo(file).size();
-#endif
-        const auto estimated_size = tar_creator.calculate_size();
-        ASSERT_GT(estimated_size, filesize_sum);
-
-        // does it match the actual size?
-        size_t actual_size {};
-        std::vector<char> buf;
-        while (tar_creator.step(buf))
-            actual_size += buf.size();
-        ASSERT_EQ(estimated_size, actual_size);
-
-        // FIXME: now extract and confirm the checksums are the same
+        for (const auto compression_enabled : std::array<bool,2>{false, true})
+        {
+            // build a directory full of random files
+            QTemporaryDir in;
+            QDir indir(in.path());
+            ASSERT_TRUE(FileUtils::fillTemporaryDirectory(in.path()));
+
+            // create the tar creator
+            EXPECT_TRUE(QDir::setCurrent(in.path()));
+            QStringList files;
+            for (file : FileUtils::getFilesRecursively(in.path()))
+                files += indir.relativeFilePath(file);
+            TarCreator tar_creator(files, compression_enabled);
+
+            // simple sanity check on its size estimate
+            const auto estimated_size = tar_creator.calculate_size();
+            const auto filesize_sum = std::accumulate(
+                files.begin(),
+                files.end(),
+                0,
+                [](ssize_t sum, QString const& filename){return sum + QFileInfo(filename).size();}
+            );
+            if (!compression_enabled)
+                ASSERT_GT(estimated_size, filesize_sum);
+
+            // does it match the actual size?
+            size_t actual_size {};
+            std::vector<char> contents, step;
+            while (tar_creator.step(step)) {
+                contents.insert(contents.end(), step.begin(), step.end());
+                actual_size += step.size();
+            }
+            ASSERT_EQ(estimated_size, actual_size);
+
+            // untar it
+            QTemporaryDir out;
+            QDir outdir(out.path());
+            QFile tarfile(outdir.filePath("tmp.tar"));
+            tarfile.open(QIODevice::WriteOnly);
+            tarfile.write(contents.data(), contents.size());
+            tarfile.close();
+            QProcess untar;
+            untar.setWorkingDirectory(outdir.path());
+            untar.start("tar", QStringList() << "xf" << tarfile.fileName());
+            EXPECT_TRUE(untar.waitForFinished()) << qPrintable(untar.errorString());
+
+            // compare it to the original
+            EXPECT_FALSE(FileUtils::compareDirectories(in.path(), out.path()));
+            EXPECT_TRUE(tarfile.remove());
+            EXPECT_TRUE(FileUtils::compareDirectories(in.path(), out.path()));
+        }
     }
 }
-
-// FIXME: calculate compressed size
-
-// FIXME: actually build the compressed tar and confirm the size eq calculated size
-
-// FIXME: what happens when we pass in a directory name instead of an ordinary file. We need to confirm the subtree gets walked correctly