Merge lp:~xavi-garcia-mena/keeper/sequential-execution-and-valgrind-fixes into lp:keeper

[Xavi Garcia]

Adds sequential execution of backup helpers.

Adds sequential execution of backup helpers.

Fixes the issue when deleting the "conn" variable in the keeper.cpp file detected with valgrind.

Modified the integration full test to run 2 backup helpers sequentially.

Modified backup_helper when detecting if it already finished.
Now it looks if it finishes when we receive the upstart signal. If we do that when detecting that the number of bytes expected and received from the backup helper match we may start the next backup helper too early, so for example, we were closing the storage-framework twice.

Added QFutureWatcher to wait for the storage framework socket to be closed before starting next task.

Added workaround to modify the number of bytes written right after calling write in the backup helper. The issue is that QlocalSocket::bytesWritten is called more times than expected. (We need to confirm this issue with Michi and James)

[Xavi Garcia (xavi-garcia-mena)]

I also modified the client for testing purposes.
It does a backup of all the XDG folders when passing --use-uuids as the first parameter.

This was really useful when testing keeper-service on the desktop with the upstart untrusted-helper job.

[Xavi Garcia (xavi-garcia-mena)]

Also... There are some files following camelCase. I followed the coding style in the file. My idea is to create a separated branch changing everything from camelCase to snake_case :)

[Charles Kerr (charlesk)]

review: Approve

=== modified file 'src/cli/main.cpp'
--- src/cli/main.cpp	2016-06-08 16:40:34 +0000
+++ src/cli/main.cpp	2016-08-02 15:14:16 +0000
@@ -22,6 +22,7 @@
 #include <util/logging.h>
 
 #include "keeper_interface.h"
+#include "keeper_user_interface.h"
 
 #include <QCoreApplication>
 #include <QDBusConnection>
@@ -36,7 +37,7 @@
     qInstallMessageHandler(util::loggingFunction);
 
     QCoreApplication app(argc, argv);
-//    DBusTypes::registerMetaTypes();
+    DBusTypes::registerMetaTypes();
 //    Variant::registerMetaTypes();
     std::srand(unsigned(std::time(nullptr)));
 
@@ -51,15 +52,56 @@
         qDebug() << QDBusConnection::sessionBus().baseService();
     }
 
-    QScopedPointer<DBusInterfaceKeeper> keeperInterface(new DBusInterfaceKeeper(DBusTypes::KEEPER_SERVICE,
-                                                            DBusTypes::KEEPER_SERVICE_PATH,
-                                                            QDBusConnection::sessionBus(), 0));
-
-    QDBusReply<void> userResp = keeperInterface->call(QLatin1String("start"));
-
-    if (!userResp.isValid())
-    {
-        qWarning() << "Error starting backup: " << userResp.error().message();
+    qDebug() << "Argc = " << argc;
+    if (argc == 2 && QString("--use-uuids") == argv[1])
+    {
+        QScopedPointer<DBusInterfaceKeeperUser> user_iface(new DBusInterfaceKeeperUser(
+                                                                DBusTypes::KEEPER_SERVICE,
+                                                                DBusTypes::KEEPER_USER_PATH,
+                                                                QDBusConnection::sessionBus()
+                                                            ) );
+        QDBusReply<QVariantDictMap> choices = user_iface->call("GetBackupChoices");
+        if (!choices.isValid())
+        {
+            qWarning() << "Error getting backup choices: " << choices.error().message();
+        }
+
+        QStringList uuids;
+        auto choices_values = choices.value();
+        for(auto iter = choices_values.begin(); iter != choices_values.end(); ++iter)
+        {
+            const auto& values = iter.value();
+            auto iter_values = values.find("type");
+            if (iter_values != values.end())
+            {
+                if (iter_values.value().toString() == "folder")
+                {
+                    // got it
+                    qDebug() << "Adding uuid " << iter.key() << " with type: " << "folder";
+                    uuids << iter.key();
+                }
+            }
+        }
+
+        QDBusReply<void> backup_reply = user_iface->call("StartBackup", uuids);
+
+        if (!backup_reply.isValid())
+        {
+            qWarning() << "Error starting backup: " << backup_reply.error().message();
+        }
+    }
+    else
+    {
+        QScopedPointer<DBusInterfaceKeeper> keeperInterface(new DBusInterfaceKeeper(DBusTypes::KEEPER_SERVICE,
+                                                                DBusTypes::KEEPER_SERVICE_PATH,
+                                                                QDBusConnection::sessionBus(), 0));
+
+        QDBusReply<void> userResp = keeperInterface->call(QLatin1String("start"));
+
+        if (!userResp.isValid())
+        {
+            qWarning() << "Error starting backup: " << userResp.error().message();
+        }
     }
 
 

=== modified file 'src/helper/backup-helper.cpp'
--- src/helper/backup-helper.cpp	2016-08-02 15:14:16 +0000
+++ src/helper/backup-helper.cpp	2016-08-02 15:14:16 +0000
@@ -122,6 +122,13 @@
                          std::bind(&BackupHelperPrivate::on_data_uploaded, this, std::placeholders::_1)
                          );
 
+        auto testHelper = qgetenv("KEEPER_TEST_HELPER");
+        if (!testHelper.isEmpty())
+        {
+            // In the testing environment we don't have upstart.
+            // TODO investigate if there's a better way to send the started signal in the tests
+            q_ptr->set_state(Helper::State::STARTED);
+        }
         reset_inactivity_timer();
     }
 
@@ -152,9 +159,11 @@
 
     void on_data_uploaded(qint64 n)
     {
-        n_uploaded_ += n;
+        // TODO review this after checking if there's a bug in storage framework.
+        // TODO The issue is that bytesWritten is called for every backup helper that was
+        // TODO executed before.
+//        n_uploaded_ += n;
         qDebug("n_read %zu n_uploaded %zu (newly uploaded %zu)", size_t(n_read_), size_t(n_uploaded_), size_t(n));
-        check_for_done();
         process_more();
     }
 
@@ -184,6 +193,7 @@
             if (n > 0) {
                 upload_buffer_.remove(0, int(n));
                 qDebug("upload_buffer_.size() is %zu after writing %zu to cloud", size_t(upload_buffer_.size()), size_t(n));
+                n_uploaded_ += n;
                 continue;
             }
             else {

=== modified file 'src/service/keeper.cpp'
--- src/service/keeper.cpp	2016-08-02 15:14:16 +0000
+++ src/service/keeper.cpp	2016-08-02 15:14:16 +0000
@@ -47,6 +47,7 @@
     QScopedPointer<StorageFrameworkClient> storage_;
     QVector<Metadata> cached_backup_choices_;
     QVector<Metadata> cached_restore_choices_;
+    QStringList remaining_tasks_;
 
     KeeperPrivate(Keeper* keeper,
                   const QSharedPointer<MetadataProvider>& backup_choices,
@@ -58,6 +59,7 @@
         , storage_(new StorageFrameworkClient())
         , cached_backup_choices_()
         , cached_restore_choices_()
+        , remaining_tasks_()
     {
         // listen for backup helper state changes
         QObject::connect(backup_helper_.data(), &Helper::state_changed,
@@ -90,6 +92,26 @@
         }
     }
 
+    void start_tasks(QStringList const& tasks)
+    {
+        remaining_tasks_ = tasks;
+        start_remaining_tasks();
+    }
+
+    void start_remaining_tasks()
+    {
+        if (remaining_tasks_.size())
+        {
+            auto task = remaining_tasks_.takeFirst();
+            start_task(task);
+        }
+    }
+
+    void clear_remaining_taks()
+    {
+        remaining_tasks_.clear();
+    }
+
 private:
 
     void on_backup_helper_state_changed(Helper::State state)
@@ -104,7 +126,13 @@
                 break;
 
             case Helper::State::CANCELLED:
+                qDebug() << "Backup helper cancelled... closing the socket.";
+                storage_->closeUploader();
+                break;
             case Helper::State::FAILED:
+                qDebug() << "Backup helper failed... closing the socket.";
+                storage_->closeUploader();
+                break;
             case Helper::State::COMPLETE:
                 qDebug() << "Backup helper finished... closing the socket.";
                 storage_->closeUploader();
@@ -155,6 +183,8 @@
     : QObject(parent)
     , d_ptr(new KeeperPrivate(this, backup_choices, restore_choices))
 {
+    Q_D(Keeper);
+    QObject::connect(d->storage_.data(), &StorageFrameworkClient::uploaderClosed, this, &Keeper::socketClosed);
 }
 
 Keeper::~Keeper() = default;
@@ -172,11 +202,7 @@
 {
     Q_D(Keeper);
 
-    // TODO maintain the list of remaining tasks
-    if (keys.size())
-    {
-        d->start_task(keys.at(0));
-    }
+    d->start_tasks(keys);
 }
 
 QDBusUnixFileDescriptor
@@ -187,8 +213,7 @@
     qDebug("Helper::StartBackup(n_bytes=%zu)", size_t(n_bytes));
 
     // the next time we get a socket from storage-framework, return it to our caller
-    auto conn = new QMetaObject::Connection();
-    auto on_socket_ready = [bus,msg,n_bytes,this,d,conn](std::shared_ptr<QLocalSocket> const &sf_socket)
+    auto on_socket_ready = [bus,msg,n_bytes,this,d](std::shared_ptr<QLocalSocket> const &sf_socket)
     {
         if (sf_socket)
         {
@@ -200,10 +225,9 @@
         auto reply = msg.createReply();
         reply << QVariant::fromValue(QDBusUnixFileDescriptor(d->backup_helper_->get_helper_socket()));
         bus.send(reply);
-        delete conn;
     };
     // cppcheck-suppress deallocuse
-    *conn = QObject::connect(d->storage_.data(), &StorageFrameworkClient::socketReady, on_socket_ready);
+    QObject::connect(d->storage_.data(), &StorageFrameworkClient::socketReady, on_socket_ready);
 
     // ask storage framework for a new socket
     d->storage_->getNewFileForBackup(n_bytes);
@@ -226,7 +250,19 @@
 
 void Keeper::socketClosed()
 {
+    Q_D(Keeper);
+
     qDebug() << "The storage framework socket was closed";
+    if (d->backup_helper_->state() == Helper::State::COMPLETE)
+    {
+        // the helper finished successfully, process more tasks
+        d->start_remaining_tasks();
+    }
+    else
+    {
+        // error or cancel case... clear all remaining tasks
+        d->clear_remaining_taks();
+    }
 }
 
 QVector<Metadata>

=== modified file 'src/storage-framework/storage_framework_client.cpp'
--- src/storage-framework/storage_framework_client.cpp	2016-08-02 15:14:16 +0000
+++ src/storage-framework/storage_framework_client.cpp	2016-08-02 15:14:16 +0000
@@ -36,9 +36,12 @@
     : QObject(parent)
     , runtime_(Runtime::create())
     , uploader_ready_watcher_(parent)
+    , uploader_closed_watcher_(parent)
     , uploader_()
 {
     QObject::connect(&uploader_ready_watcher_,&QFutureWatcher<std::shared_ptr<Uploader>>::finished, this, &StorageFrameworkClient::uploaderReady);
+    QObject::connect(&uploader_closed_watcher_,&QFutureWatcher<std::shared_ptr<unity::storage::qt::client::File>>::finished, this, &StorageFrameworkClient::onUploaderClosed);
+
 }
 
 
@@ -83,7 +86,7 @@
     try
     {
         uploader_->socket()->disconnectFromServer();
-        uploader_->finish_upload();
+        uploader_closed_watcher_.setFuture(uploader_->finish_upload());
     }
     catch (std::exception & e)
     {
@@ -97,3 +100,9 @@
 
     Q_EMIT (socketReady(uploader_->socket()));
 }
+
+void StorageFrameworkClient::onUploaderClosed()
+{
+    qDebug() << "Uploader was closed";
+    Q_EMIT(uploaderClosed());
+}

=== modified file 'src/storage-framework/storage_framework_client.h'
--- src/storage-framework/storage_framework_client.h	2016-08-02 15:14:16 +0000
+++ src/storage-framework/storage_framework_client.h	2016-08-02 15:14:16 +0000
@@ -42,14 +42,17 @@
 
 public Q_SLOTS:
     void uploaderReady();
+    void onUploaderClosed();
 
 Q_SIGNALS:
     void socketReady(std::shared_ptr<QLocalSocket> const& sf_socket);
+    void uploaderClosed();
 
 private:
     unity::storage::qt::client::Runtime::SPtr runtime_;
 
     // watchers
     QFutureWatcher<std::shared_ptr<unity::storage::qt::client::Uploader>> uploader_ready_watcher_;
+    QFutureWatcher<std::shared_ptr<unity::storage::qt::client::File>> uploader_closed_watcher_;
     std::shared_ptr<unity::storage::qt::client::Uploader> uploader_;
 };

=== modified file 'tests/integration/helpers/helpers-test.cc'
--- tests/integration/helpers/helpers-test.cc	2016-08-01 10:06:24 +0000
+++ tests/integration/helpers/helpers-test.cc	2016-08-02 15:14:16 +0000
@@ -313,6 +313,9 @@
             qDebug("test failed; leaving '%s'", data_home_dir.path().toUtf8().constData());
 
         ASSERT_EQ(nullptr, bus);
+
+        // let's leave things clean
+        EXPECT_TRUE(removeHelperMarkBeforeStarting());
     }
 
     bool startKeeperClient()
@@ -326,6 +329,29 @@
         return true;
     }
 
+    bool checkStorageFrameworkFiles(QStringList const & sourceDirs, bool compression=false)
+    {
+        QStringList dirs = sourceDirs;
+
+        while (dirs.size() > 0)
+        {
+            auto dir = dirs.takeLast();
+            QString lastFile = getLastStorageFrameworkFile();
+            if (lastFile.isEmpty())
+            {
+                qWarning() << "Did not found enough storage framework files";
+                return false;
+            }
+            if (!compareTarContent (lastFile, dir, compression))
+            {
+                return false;
+            }
+            // remove the last file, so next iteration the last one is different
+            QFile::remove(lastFile);
+        }
+        return true;
+    }
+
     bool checkLastStorageFrameworkFile (QString const & sourceDir, bool compression=false)
     {
         QString lastFile = getLastStorageFrameworkFile();
@@ -341,6 +367,8 @@
     {
         QTemporaryDir tempDir;
 
+        qDebug() << "Comparing tar content for dir: " << sourceDir << " with tar: " << tarPath;
+
         QFileInfo checkFile(tarPath);
         if (!checkFile.exists())
         {
@@ -411,26 +439,27 @@
             qWarning() << "ERROR: Storage framework directory: [" << storage_framework_dir_path << "] does not exist.";
             return QString();
         }
-        QFileInfo lastFile;
+
+        QStringList sortedFiles;
         QFileInfoList files = storage_framework_dir.entryInfoList();
         for (int i = 0; i < files.size(); ++i)
         {
             QFileInfo file = files[i];
             if (file.isFile())
             {
-                if (file.created() > lastFile.created())
-                {
-                    lastFile = file;
-                }
+                sortedFiles << files[i].absoluteFilePath();
             }
         }
-        if (!lastFile.isFile())
+
+        // we detect the last file by name.
+        // the file creation time had not enough precision
+        sortedFiles.sort();
+        if (sortedFiles.isEmpty())
         {
             qWarning() << "ERROR: last file in the storage-framework directory was not found";
             return QString();
         }
-
-        return lastFile.absoluteFilePath();
+        return sortedFiles.last();
     }
 
     bool checkStorageFrameworkContent(QString const & content)
@@ -465,21 +494,53 @@
 
 
 
-    bool waitUntilHelperFinishes(int maxTimeout = 15000)
+    bool waitUntilHelperFinishes(QString const & app_id, int maxTimeout = 15000, int times = 1)
     {
         // TODO create a new mock for upstart that controls the lifecycle of the
         // helper process so we can do this in a cleaner way.
         QFile helper_mark(SIMPLE_HELPER_MARK_FILE_PATH);
         QElapsedTimer timer;
         timer.start();
-        while (!timer.hasExpired(maxTimeout))
+        auto times_to_wait = times;
+        bool finished = false;
+        while (!timer.hasExpired(maxTimeout) && !finished)
         {
             if (helper_mark.exists())
             {
-                return true;
+                if (--times_to_wait)
+                {
+                    helper_mark.remove();
+                    timer.restart();
+                    qDebug() << "HELPER FINISHED, WAITING FOR " << times_to_wait << " MORE";
+                }
+                else
+                {
+                    qDebug() << "ALL HELPERS FINISHED";
+                    finished = true;
+                }
+                sendUpstartHelperTermination(app_id);
             }
         }
-        return false;
+        return finished;
+    }
+
+    void sendUpstartHelperTermination(QString const &app_id)
+    {
+        // send the upstart signal so keeper-service is aware of the helper termination
+        DbusTestDbusMockObject* objUpstart =
+            dbus_test_dbus_mock_get_object(mock, UPSTART_PATH, UPSTART_INTERFACE, NULL);
+
+        QString eventInfoStr = QString("('stopped', ['JOB=untrusted-helper', 'INSTANCE=backup-helper::%1'])").arg(app_id.toStdString().c_str());
+        dbus_test_dbus_mock_object_emit_signal(
+            mock, objUpstart, "EventEmitted", G_VARIANT_TYPE("(sas)"),
+            g_variant_new_parsed(
+                    eventInfoStr.toLocal8Bit().data()),
+            NULL);
+        g_usleep(100000);
+        while (g_main_pending())
+        {
+            g_main_iteration(TRUE);
+        }
     }
 
     QString getUUIDforXdgFolderPath(QString const &path, QVariantDictMap const & choices) const
@@ -809,37 +870,34 @@
     // fill something in the music dir
     ASSERT_TRUE(FileUtils::fillTemporaryDirectory(user_dir, qrand() % 1000));
 
-
     // search for the user folder uuid
     auto user_folder_uuid = getUUIDforXdgFolderPath(user_dir, choices.value());
     ASSERT_FALSE(user_folder_uuid.isEmpty());
     qDebug() << "User folder UUID is: " << user_folder_uuid;
 
+    QString user_option_2 = "XDG_VIDEOS_DIR";
+
+    auto user_dir_2 = qgetenv(user_option_2.toLatin1().data());
+    ASSERT_FALSE(user_dir_2.isEmpty());
+    qDebug() << "USER DIR 2: " << user_dir_2;
+
+    // fill something in the music dir
+    ASSERT_TRUE(FileUtils::fillTemporaryDirectory(user_dir_2, qrand() % 1000));
+
+    // search for the user folder uuid
+    auto user_folder_uuid_2 = getUUIDforXdgFolderPath(user_dir_2, choices.value());
+    ASSERT_FALSE(user_folder_uuid_2.isEmpty());
+    qDebug() << "User folder 2 UUID is: " << user_folder_uuid_2;
+
     // Now we know the music folder uuid, let's start the backup for it.
-    QDBusReply<void> backup_reply = user_iface->call("StartBackup", QStringList{user_folder_uuid});
+    QDBusReply<void> backup_reply = user_iface->call("StartBackup", QStringList{user_folder_uuid, user_folder_uuid_2});
     ASSERT_TRUE(backup_reply.isValid()) << qPrintable(QDBusConnection::sessionBus().lastError().message());
 
     // Wait until the helper finishes
-    EXPECT_TRUE(waitUntilHelperFinishes());
-
-    // send the upstart signal so keeper-service is aware of the helper termination
-    DbusTestDbusMockObject* objUpstart =
-        dbus_test_dbus_mock_get_object(mock, UPSTART_PATH, UPSTART_INTERFACE, NULL);
-
-    QString eventInfoStr = QString("('stopped', ['JOB=untrusted-helper', 'INSTANCE=backup-helper::%1'])").arg(DEKKO_APP_ID);
-    dbus_test_dbus_mock_object_emit_signal(
-        mock, objUpstart, "EventEmitted", G_VARIANT_TYPE("(sas)"),
-        g_variant_new_parsed(
-                eventInfoStr.toLocal8Bit().data()),
-        NULL);
-    g_usleep(100000);
-    while (g_main_pending())
-    {
-        g_main_iteration(TRUE);
-    }
+    EXPECT_TRUE(waitUntilHelperFinishes(DEKKO_APP_ID, 15000, 2));
 
     // check that the content of the file is the expected
-    EXPECT_TRUE(checkLastStorageFrameworkFile(user_dir));
+    EXPECT_TRUE(checkStorageFrameworkFiles(QStringList{user_dir, user_dir_2}));
     // let's leave things clean
     EXPECT_TRUE(removeHelperMarkBeforeStarting());