Remove process locking and refactor singal handling

1) refactor signal handling for tracking default
revision in its own signal. This allows to remove
an unnecessary indirection and helps to improve
revision-change-handling when the deployment list
has changed.

2) remove multi-process locking..

.. as it was based on incorrect assumptions on
how concurrency is handled by OSTree.

OSTree has API to lock the sysroot, but not the
repository itself. As we can not lock the repo
and writes to the repo are atomic, reading from
the repo in general is safe. Except when the data
has been read, there is no guarantee that the data
won't disapear under us (because some other process
has done repo pruning).

Concurrent writing to the repo is handled by OSTree.
When we deploy new sysroot version via "ostree deploy",
it locks the sysroot with the sysroot locking API, so
we don't need to do any additinal locking.

Change-Id: Ide593f9bfbe93827093a6693eeb10cc3b315e000
Reviewed-by: Gatis Paeglis <gatis.paeglis@qt.io>

7 files changed, 95 insertions, 138 deletions

diff --git a/examples/qml/basic/main.qml b/examples/qml/basic/main.qml
index a65b332..3168c54 100644
--- a/examples/qml/basic/main.qml
+++ b/examples/qml/basic/main.qml
@@ -252,7 +252,7 @@ Window {
         onErrorChanged: logError(error)
         onStatusChanged: log(status)
         onInitializationFinished: {
-            logWithCondition("Initialization", OtaClient.initialized)
+            logWithCondition("Initialization", success)
             if (!configureRepository(basicConfig, true))
                 updateConfigView(OtaClient.repositoryConfig())
             updateBootedMetadataLabel()
diff --git a/src/imports/pluginmain.cpp b/src/imports/pluginmain.cpp
index 3f154ac..a498b93 100644
--- a/src/imports/pluginmain.cpp
+++ b/src/imports/pluginmain.cpp
@@ -326,7 +326,8 @@ QT_BEGIN_NAMESPACE
     \readonly
 
     Holds a bool indicating whether a reboot is required. Reboot is required
-    after update() and rollback(), to boot into the new default system.
+    after update(), updateOffline() and rollback(), to boot into the new default
+    system.
 */
 
 /*!
@@ -337,10 +338,11 @@ QT_BEGIN_NAMESPACE
 */
 
 /*!
-    \qmlsignal OtaClient::initializationFinished()
+    \qmlsignal OtaClient::initializationFinished(bool success)
 
     This signal is emitted when the object has finished initialization. The
-    object is not ready for use until this signal is received.
+    object is not ready for use until this signal is received. The \a success
+    argument indicates whether the initialization was successful.
 */
 
 /*!
diff --git a/src/lib/qotaclient.cpp b/src/lib/qotaclient.cpp
index 4981ad0..f8d0d1a 100644
--- a/src/lib/qotaclient.cpp
+++ b/src/lib/qotaclient.cpp
@@ -113,36 +113,15 @@ void QOtaClientPrivate::refreshState()
     }
 }
 
-void QOtaClientPrivate::initializeFinished(const QString &defaultRev,
-                                           const QString &bootedRev, const QJsonDocument &bootedInfo)
+void QOtaClientPrivate::initializeFinished(bool success, const QString &bootedRev,
+                                           const QJsonDocument &bootedInfo)
 {
     Q_Q(QOtaClient);
-    m_defaultRev = defaultRev;
     m_bootedRev = bootedRev;
     updateInfoMembers(bootedInfo, &m_bootedInfo, &m_bootedVersion, &m_bootedDescription);
     refreshState();
-    m_initialized = true;
-    emit q->initializationFinished();
-}
-
-void QOtaClientPrivate::updateFinished(const QString &defaultRev, bool success)
-{
-    Q_Q(QOtaClient);
-    if (success) {
-        m_defaultRev = defaultRev;
-        refreshState();
-    }
-    emit q->updateFinished(success);
-}
-
-void QOtaClientPrivate::rollbackFinished(const QString &defaultRev, bool success)
-{
-    Q_Q(QOtaClient);
-    if (success) {
-        m_defaultRev = defaultRev;
-        refreshState();
-    }
-    emit q->rollbackFinished(success);
+    m_initialized = success;
+    emit q->initializationFinished(m_initialized);
 }
 
 void QOtaClientPrivate::statusStringChanged(const QString &status)
@@ -168,7 +147,7 @@ bool QOtaClientPrivate::verifyPathExist(const QString &path)
     return true;
 }
 
-void QOtaClientPrivate::rollbackChanged(const QString &rollbackRev, const QJsonDocument &rollbackInfo, int treeCount)
+void QOtaClientPrivate::rollbackInfoChanged(const QString &rollbackRev, const QJsonDocument &rollbackInfo, int treeCount)
 {
     Q_Q(QOtaClient);
     if (!m_rollbackAvailable && treeCount > 1) {
@@ -188,7 +167,16 @@ void QOtaClientPrivate::remoteInfoChanged(const QString &remoteRev, const QJsonD
 
     m_remoteRev = remoteRev;
     updateInfoMembers(remoteInfo, &m_remoteInfo, &m_remoteVersion, &m_remoteDescription);
+    refreshState();
     emit q->remoteInfoChanged();
+}
+
+void QOtaClientPrivate::defaultRevisionChanged(const QString &defaultRevision)
+{
+    if (m_defaultRev == defaultRevision)
+        return;
+
+    m_defaultRev = defaultRevision;
     refreshState();
 }
 
@@ -370,10 +358,11 @@ QString QOtaClientPrivate::revision(QueryTarget target) const
 */
 
 /*!
-    \fn void QOtaClient::initializationFinished()
+    \fn void QOtaClient::initializationFinished(bool success)
 
     This signal is emitted when the object has finished initialization. The
-    object is not ready for use until this signal is received.
+    object is not ready for use until this signal is received. The \a success
+    argument indicates whether the initialization was successful.
 */
 
 /*!
@@ -393,14 +382,15 @@ QOtaClient::QOtaClient(QObject *parent) :
         QOtaClientAsync *async = d->m_otaAsync.data();
         connect(async, &QOtaClientAsync::initializeFinished, d, &QOtaClientPrivate::initializeFinished);
         connect(async, &QOtaClientAsync::fetchRemoteInfoFinished, this, &QOtaClient::fetchRemoteInfoFinished);
-        connect(async, &QOtaClientAsync::updateFinished, d, &QOtaClientPrivate::updateFinished);
-        connect(async, &QOtaClientAsync::rollbackFinished, d, &QOtaClientPrivate::rollbackFinished);
+        connect(async, &QOtaClientAsync::updateFinished, this, &QOtaClient::updateFinished);
+        connect(async, &QOtaClientAsync::rollbackFinished, this, &QOtaClient::rollbackFinished);
         connect(async, &QOtaClientAsync::updateOfflineFinished, this, &QOtaClient::updateOfflineFinished);
         connect(async, &QOtaClientAsync::updateRemoteInfoOfflineFinished, this, &QOtaClient::updateRemoteInfoOfflineFinished);
         connect(async, &QOtaClientAsync::errorOccurred, d, &QOtaClientPrivate::errorOccurred);
         connect(async, &QOtaClientAsync::statusStringChanged, d, &QOtaClientPrivate::statusStringChanged);
-        connect(async, &QOtaClientAsync::rollbackChanged, d, &QOtaClientPrivate::rollbackChanged);
+        connect(async, &QOtaClientAsync::rollbackInfoChanged, d, &QOtaClientPrivate::rollbackInfoChanged);
         connect(async, &QOtaClientAsync::remoteInfoChanged, d, &QOtaClientPrivate::remoteInfoChanged);
+        connect(async, &QOtaClientAsync::defaultRevisionChanged, d, &QOtaClientPrivate::defaultRevisionChanged);
         d->m_otaAsync->initialize();
     }
 }
@@ -747,8 +737,8 @@ bool QOtaClient::rollbackAvailable() const
     \property QOtaClient::restartRequired
     \brief whether a reboot is required.
 
-    Reboot is required after update() and rollback(), to boot into the new
-    default system.
+    Reboot is required after update(), updateOffline() and rollback(),
+    to boot into the new default system.
 
 */
 bool QOtaClient::restartRequired() const
diff --git a/src/lib/qotaclient.h b/src/lib/qotaclient.h
index 1bea139..c5df719 100644
--- a/src/lib/qotaclient.h
+++ b/src/lib/qotaclient.h
@@ -108,7 +108,7 @@ Q_SIGNALS:
     void errorOccurred(const QString &error);
     void repositoryConfigChanged(QOtaRepositoryConfig *config);
 
-    void initializationFinished();
+    void initializationFinished(bool success);
     void fetchRemoteInfoFinished(bool success);
     void updateFinished(bool success);
     void rollbackFinished(bool success);
diff --git a/src/lib/qotaclient_p.h b/src/lib/qotaclient_p.h
index 0a30027..dd9b6b0 100644
--- a/src/lib/qotaclient_p.h
+++ b/src/lib/qotaclient_p.h
@@ -61,15 +61,13 @@ public:
     virtual ~QOtaClientPrivate();
 
     void refreshState();
-    void initializeFinished(const QString &defaultRev,
-                            const QString &bootedRev, const QJsonDocument &bootedInfo);
-    void updateFinished(const QString &defaultRev, bool success);
-    void rollbackFinished(const QString &defaultRev, bool success);
+    void initializeFinished(bool success, const QString &bootedRev, const QJsonDocument &bootedInfo);
     void statusStringChanged(const QString &status);
     void errorOccurred(const QString &error);
     bool verifyPathExist(const QString &path);
-    void rollbackChanged(const QString &rollbackRev, const QJsonDocument &rollbackInfo, int treeCount);
+    void rollbackInfoChanged(const QString &rollbackRev, const QJsonDocument &rollbackInfo, int treeCount);
     void remoteInfoChanged(const QString &remoteRev, const QJsonDocument &remoteInfo);
+    void defaultRevisionChanged(const QString &defaultRevision);
 
     QString version(QueryTarget target) const;
     QString description(QueryTarget target) const;
diff --git a/src/lib/qotaclientasync.cpp b/src/lib/qotaclientasync.cpp
index 9623bc7..c62fdd0 100644
--- a/src/lib/qotaclientasync.cpp
+++ b/src/lib/qotaclientasync.cpp
@@ -42,7 +42,7 @@ QT_BEGIN_NAMESPACE
 #define OSTREE_STATIC_DELTA_SUPERBLOCK_FORMAT "(a{sv}tayay" OSTREE_COMMIT_GVARIANT_STRING "aya" OSTREE_STATIC_DELTA_META_ENTRY_FORMAT "a" OSTREE_STATIC_DELTA_FALLBACK_FORMAT ")"
 
 QOtaClientAsync::QOtaClientAsync() :
-    m_sysroot(ostree_sysroot_new(0))
+    m_sysroot(ostree_sysroot_new_default())
 {
     // async mapper
     connect(this, &QOtaClientAsync::initialize, this, &QOtaClientAsync::_initialize);
@@ -55,7 +55,7 @@ QOtaClientAsync::QOtaClientAsync() :
 
 QOtaClientAsync::~QOtaClientAsync()
 {
-    g_object_unref (m_sysroot);
+    ostree_sysroot_unload (m_sysroot);
 }
 
 static void parseErrorString(QString *error)
@@ -144,61 +144,37 @@ QJsonDocument QOtaClientAsync::info(QOtaClientPrivate::QueryTarget target, bool
     return jsonInfo;
 }
 
-bool QOtaClientAsync::multiprocessLock(const QString &method)
-{
-    qCDebug(qota) << QTime::currentTime().toString() << method << "- waiting for lock...";
-    GError *error = nullptr;
-    ostree_sysroot_lock (m_sysroot, &error);
-    if (emitGError(error))
-        return false;
-    qCDebug(qota) << QTime::currentTime().toString() << "lock acquired";
-    return true;
-}
-
-void QOtaClientAsync::multiprocessUnlock()
-{
-    ostree_sysroot_unlock (m_sysroot);
-    qCDebug(qota) << QTime::currentTime().toString() << "lock released";
-}
-
-QString QOtaClientAsync::defaultRevision()
-{
-    g_autoptr(GPtrArray) deployments = ostree_sysroot_get_deployments (m_sysroot);
-    OstreeDeployment *firstDeployment = (OstreeDeployment*)deployments->pdata[0];
-    return QLatin1String(ostree_deployment_get_csum (firstDeployment));
-}
-
 void QOtaClientAsync::_initialize()
 {
-    if (!multiprocessLock(QStringLiteral("_initialize")))
+    GError *error = nullptr;
+    if (!ostree_sysroot_load (m_sysroot, 0, &error) ||
+        !ostree_sysroot_get_repo (m_sysroot, &m_repo, 0, &error)) {
+        emitGError(error);
+        emit initializeFinished(false);
         return;
+    }
 
-    // intentionally let the initialization to complete, even if some errors occur.
-    GError *error = nullptr;
-    ostree_sysroot_load (m_sysroot, 0, &error);
-    emitGError(error);
-    ostree_sysroot_get_repo (m_sysroot, &m_repo, 0, &error);
-    emitGError(error);
+    handleRevisionChanges();
 
-    OstreeDeployment *bootedDeployment = (OstreeDeployment*)ostree_sysroot_get_booted_deployment (m_sysroot);
-    QString bootedRev = QLatin1String(ostree_deployment_get_csum (bootedDeployment));
     bool ok = true;
-    QJsonDocument bootedInfo = info(QOtaClientPrivate::QueryTarget::Booted, &ok);
-    QString defaultRev = defaultRevision();
     // prepopulate with what we think is on the remote server (head of the local repo)
     QString remoteRev = ostree(QStringLiteral("ostree rev-parse linux/qt"), &ok);
-    QJsonDocument remoteInfo = info(QOtaClientPrivate::QueryTarget::Remote, &ok, remoteRev);
-
-    resetRollbackState();
+    QJsonDocument remoteInfo;
+    if (ok) remoteInfo = info(QOtaClientPrivate::QueryTarget::Remote, &ok, remoteRev);
+    if (!ok) {
+        emit initializeFinished(false);
+        return;
+    }
     emit remoteInfoChanged(remoteRev, remoteInfo);
-    emit initializeFinished(defaultRev, bootedRev, bootedInfo);
-    multiprocessUnlock();
+
+    OstreeDeployment *bootedDeployment = (OstreeDeployment*)ostree_sysroot_get_booted_deployment (m_sysroot);
+    QString bootedRev = QLatin1String(ostree_deployment_get_csum (bootedDeployment));
+    QJsonDocument bootedInfo = info(QOtaClientPrivate::QueryTarget::Booted, &ok);
+    emit initializeFinished(ok, bootedRev, bootedInfo);
 }
 
 void QOtaClientAsync::_fetchRemoteInfo()
 {
-    if (!multiprocessLock(QStringLiteral("_fetchRemoteInfo")))
-        return;
     QString remoteRev;
     QJsonDocument remoteInfo;
     bool ok = true;
@@ -208,7 +184,6 @@ void QOtaClientAsync::_fetchRemoteInfo()
     if (ok) remoteInfo = info(QOtaClientPrivate::QueryTarget::Remote, &ok, remoteRev);
     if (ok) emit remoteInfoChanged(remoteRev, remoteInfo);
     emit fetchRemoteInfoFinished(ok);
-    multiprocessUnlock();
 }
 
 bool QOtaClientAsync::deployCommit(const QString &commit)
@@ -235,26 +210,23 @@ bool QOtaClientAsync::deployCommit(const QString &commit)
 
 void QOtaClientAsync::_update(const QString &updateToRev)
 {
-    if (!multiprocessLock(QStringLiteral("_update")))
-        return;
     bool ok = true;
-    QString defaultRev;
     GError *error = nullptr;
     emit statusStringChanged(QStringLiteral("Checking for missing objects..."));
     ostree(QString(QStringLiteral("ostree pull qt-os:%1")).arg(updateToRev), &ok, true);
-    multiprocessUnlock();
-    if (!ok || deployCommit(updateToRev))
-        goto out;
-
-    ostree_sysroot_load (m_sysroot, 0, &error);
-    if (emitGError(error))
+    if (!ok || !deployCommit(updateToRev)) {
+        emit updateFinished(false);
         return;
+    }
 
-    resetRollbackState();
-    defaultRev = defaultRevision();
+    if (!ostree_sysroot_load (m_sysroot, 0, &error)) {
+        emitGError(error);
+        emit updateFinished(false);
+        return;
+    }
 
-out:
-    emit updateFinished(defaultRev, ok);
+    handleRevisionChanges();
+    emit updateFinished(ok);
 }
 
 int QOtaClientAsync::rollbackIndex()
@@ -270,25 +242,21 @@ int QOtaClientAsync::rollbackIndex()
     return 1;
 }
 
-void QOtaClientAsync::resetRollbackState()
+void QOtaClientAsync::handleRevisionChanges()
 {
-    int index = rollbackIndex();
-    if (index == -1)
-        return;
-
     g_autoptr(GPtrArray) deployments = ostree_sysroot_get_deployments (m_sysroot);
-    OstreeDeployment *rollbackDeployment = (OstreeDeployment*)deployments->pdata[index];
-    QString rollbackRev = QLatin1String(ostree_deployment_get_csum (rollbackDeployment));
-    bool ok = true;
-    QJsonDocument rollbackInfo = info(QOtaClientPrivate::QueryTarget::Rollback, &ok, rollbackRev);
-    emit rollbackChanged(rollbackRev, rollbackInfo, deployments->len);
-}
+    OstreeDeployment *firstDeployment = (OstreeDeployment*)deployments->pdata[0];
+    QString defaultRev(QLatin1String(ostree_deployment_get_csum (firstDeployment)));
+    emit defaultRevisionChanged(defaultRev);
 
-void QOtaClientAsync::emitRollbackFailed(const QString &error)
-{
-    emit errorOccurred(error);
-    emit rollbackFinished(QStringLiteral(""), false);
-    multiprocessUnlock();
+    int index = rollbackIndex();
+    if (index != -1) {
+        OstreeDeployment *rollbackDeployment = (OstreeDeployment*)deployments->pdata[index];
+        QString rollbackRev(QLatin1String(ostree_deployment_get_csum (rollbackDeployment)));
+        bool ok = true;
+        QJsonDocument rollbackInfo = info(QOtaClientPrivate::QueryTarget::Rollback, &ok, rollbackRev);
+        emit rollbackInfoChanged(rollbackRev, rollbackInfo, deployments->len);
+    }
 }
 
 bool QOtaClientAsync::emitGError(GError *error)
@@ -297,22 +265,23 @@ bool QOtaClientAsync::emitGError(GError *error)
         return false;
 
     emit errorOccurred(QString::fromLatin1((error->message)));
-    multiprocessUnlock();
+    g_error_free (error);
     return true;
 }
 
 void QOtaClientAsync::_rollback()
 {
-    if (!multiprocessLock(QStringLiteral("_rollback")))
-        return;
     GError *error = nullptr;
-    ostree_sysroot_load (m_sysroot, 0, &error);
-    if (emitGError(error))
+    if (!ostree_sysroot_load (m_sysroot, 0, &error)) {
+        emitGError(error);
+        emit rollbackFinished(false);
         return;
+    }
 
     int index = rollbackIndex();
     if (index == -1) {
-        emitRollbackFailed(QStringLiteral("At least 2 system versions required for rollback"));
+        emit errorOccurred(QStringLiteral("At least 2 system versions required for rollback"));
+        emit rollbackFinished(false);
         return;
     }
 
@@ -328,14 +297,12 @@ void QOtaClientAsync::_rollback()
     // atomically update bootloader configuration
     if (!ostree_sysroot_write_deployments (m_sysroot, newDeployments, 0, &error)) {
         emitGError(error);
-        emitRollbackFailed(QStringLiteral("Failed to update bootloader configuration"));
+        emit rollbackFinished(false);
         return;
     }
 
-    resetRollbackState();
-    QString defaultRev = defaultRevision();
-    emit rollbackFinished(defaultRev, true);
-    multiprocessUnlock();
+    handleRevisionChanges();
+    emit rollbackFinished(true);
 }
 
 bool QOtaClientAsync::extractPackage(const QString &packagePath, QString *updateToRev)
@@ -413,6 +380,9 @@ void QOtaClientAsync::_updateOffline(const QString &packagePath)
     if (!extractPackage(packagePath, &rev) || !deployCommit(rev))
         success = false;
 
+    if (success)
+        handleRevisionChanges();
+
     emit updateOfflineFinished(success);
 }
 
diff --git a/src/lib/qotaclientasync_p.h b/src/lib/qotaclientasync_p.h
index 6841eb5..bf35c0f 100644
--- a/src/lib/qotaclientasync_p.h
+++ b/src/lib/qotaclientasync_p.h
@@ -53,31 +53,28 @@ public:
 
 signals:
     void initialize();
-    void initializeFinished(const QString &defaultRev,
-                            const QString &bootedRev, const QJsonDocument &bootedInfo);
+    void initializeFinished(bool success, const QString &bootedRev = QString(),
+                            const QJsonDocument &bootedInfo = QJsonDocument());
     void fetchRemoteInfo();
     void fetchRemoteInfoFinished(bool success);
     void update(const QString &updateToRev);
-    void updateFinished(const QString &defaultRev, bool success);
+    void updateFinished(bool success);
     void rollback();
-    void rollbackFinished(const QString &defaultRev, bool success);
+    void rollbackFinished(bool success);
     void updateOffline(const QString &packagePath);
     void updateOfflineFinished(bool success);
     void updateRemoteInfoOffline(const QString &packagePath);
     void updateRemoteInfoOfflineFinished(bool success);
-    void rollbackChanged(const QString &rollbackRev, const QJsonDocument &rollbackInfo, int treeCount);
+    void rollbackInfoChanged(const QString &rollbackRev, const QJsonDocument &rollbackInfo, int treeCount);
     void errorOccurred(const QString &error);
     void statusStringChanged(const QString &status);
     void remoteInfoChanged(const QString &remoteRev, const QJsonDocument &remoteInfo);
+    void defaultRevisionChanged(const QString &defaultRevision);
 
 protected:
     QJsonDocument info(QOtaClientPrivate::QueryTarget target, bool *ok, const QString &rev = QString());
-    bool multiprocessLock(const QString &method);
-    void multiprocessUnlock();
-    QString defaultRevision();
-    void emitRollbackFailed(const QString &error);
     int rollbackIndex();
-    void resetRollbackState();
+    void handleRevisionChanges();
     bool emitGError(GError *error);
     bool deployCommit(const QString &commit);
     bool extractPackage(const QString &packagePath, QString *updateToRev);