Merge branch 'mt-gs' into calamares

Fix up potential thread-safety issues with GS, expand testing.

src/libcalamares/CMakeLists.txt

@@ -1,6 +1,7 @@
 # === This file is part of Calamares - <https://github.com/calamares> ===
 #
 #   SPDX-FileCopyrightText: 2020 Adriaan de Groot <groot@kde.org>
+#   SPDX-License-Identifier: GPL-3.0-or-later
 #
 #   Calamares is free software: you can redistribute it and/or modify
 #   it under the terms of the GNU General Public License as published by
@@ -15,12 +16,12 @@
 #   You should have received a copy of the GNU General Public License
 #   along with Calamares. If not, see <http://www.gnu.org/licenses/>.
 #
-#   SPDX-License-Identifier: GPL-3.0-or-later
-#   License-Filename: LICENSE
-#
+
 #
 # libcalamares is the non-GUI part of Calamares, which includes handling
-# translations, configurations, logging, utilities, global storage, and (non-GUI) jobs.
+# translations, configurations, logging, utilities, global storage, and
+# (non-GUI) jobs.
+#
 
 add_definitions( -DDLLEXPORT_PRO )
 include_directories( ${CMAKE_CURRENT_BINARY_DIR} ${CMAKE_CURRENT_SOURCE_DIR} )
@@ -217,18 +218,24 @@ endforeach()
 #
 calamares_add_test(
     libcalamarestest
+    SOURCES
+        Tests.cpp
+)
+
+calamares_add_test(
+    libcalamaresutilstest
     SOURCES
         utils/Tests.cpp
 )
 
 calamares_add_test(
-    libcalamarestestpaths
+    libcalamaresutilspathstest
     SOURCES
         utils/TestPaths.cpp
 )
 
 calamares_add_test(
-    geoiptest
+    libcalamaresgeoiptest
     SOURCES
         geoip/GeoIPTests.cpp
         ${geoip_src}

src/libcalamares/GlobalStorage.cpp

@@ -2,6 +2,7 @@
  *
  *   SPDX-FileCopyrightText: 2014-2015 Teo Mrnjavac <teo@kde.org>
  *   SPDX-FileCopyrightText: 2017-2018 Adriaan de Groot <groot@kde.org>
+ *   SPDX-License-Identifier: GPL-3.0-or-later
  *
  *   Calamares is free software: you can redistribute it and/or modify
  *   it under the terms of the GNU General Public License as published by
@@ -16,13 +17,9 @@
  *   You should have received a copy of the GNU General Public License
  *   along with Calamares. If not, see <http://www.gnu.org/licenses/>.
  *
- *   SPDX-License-Identifier: GPL-3.0-or-later
- *   License-Filename: LICENSE
- *
  */
 
 #include "GlobalStorage.h"
-#include "JobQueue.h"
 
 #include "utils/Logger.h"
 #include "utils/Units.h"
@@ -30,12 +27,35 @@
 
 #include <QFile>
 #include <QJsonDocument>
+#include <QMutexLocker>
 
 using CalamaresUtils::operator""_MiB;
 
 namespace Calamares
 {
 
+class GlobalStorage::ReadLock : public QMutexLocker
+{
+public:
+    ReadLock( const GlobalStorage* gs )
+        : QMutexLocker( &gs->m_mutex )
+    {
+    }
+};
+
+class GlobalStorage::WriteLock : public QMutexLocker
+{
+public:
+    WriteLock( GlobalStorage* gs )
+        : QMutexLocker( &gs->m_mutex )
+        , m_gs( gs )
+    {
+    }
+    ~WriteLock() { m_gs->changed(); }
+
+    GlobalStorage* m_gs;
+};
+
 GlobalStorage::GlobalStorage( QObject* parent )
     : QObject( parent )
 {
@@ -45,6 +65,7 @@ GlobalStorage::GlobalStorage( QObject* parent )
 bool
 GlobalStorage::contains( const QString& key ) const
 {
+    ReadLock l( this );
     return m.contains( key );
 }
 
@@ -52,6 +73,7 @@ GlobalStorage::contains( const QString& key ) const
 int
 GlobalStorage::count() const
 {
+    ReadLock l( this );
     return m.count();
 }
 
@@ -59,14 +81,15 @@ GlobalStorage::count() const
 void
 GlobalStorage::insert( const QString& key, const QVariant& value )
 {
+    WriteLock l( this );
     m.insert( key, value );
-    emit changed();
 }
 
 
 QStringList
 GlobalStorage::keys() const
 {
+    ReadLock l( this );
     return m.keys();
 }
 
@@ -74,8 +97,8 @@ GlobalStorage::keys() const
 int
 GlobalStorage::remove( const QString& key )
 {
+    WriteLock l( this );
     int nItems = m.remove( key );
-    emit changed();
     return nItems;
 }
 
@@ -83,21 +106,25 @@ GlobalStorage::remove( const QString& key )
 QVariant
 GlobalStorage::value( const QString& key ) const
 {
+    ReadLock l( this );
     return m.value( key );
 }
 
 void
 GlobalStorage::debugDump() const
 {
+    ReadLock l( this );
+    cDebug() << "GlobalStorage" << Logger::Pointer( this ) << m.count() << "items";
     for ( auto it = m.cbegin(); it != m.cend(); ++it )
     {
-        cDebug() << it.key() << '\t' << it.value();
+        cDebug() << Logger::SubEntry << it.key() << '\t' << it.value();
     }
 }
 
 bool
-GlobalStorage::save( const QString& filename )
+GlobalStorage::saveJson( const QString& filename ) const
 {
+    ReadLock l( this );
     QFile f( filename );
     if ( !f.open( QFile::WriteOnly ) )
     {
@@ -110,7 +137,7 @@ GlobalStorage::save( const QString& filename )
 }
 
 bool
-GlobalStorage::load( const QString& filename )
+GlobalStorage::loadJson( const QString& filename )
 {
     QFile f( filename );
     if ( !f.open( QFile::ReadOnly ) )
@@ -130,10 +157,14 @@ GlobalStorage::load( const QString& filename )
     }
     else
     {
+        WriteLock l( this );
+        // Do **not** use method insert() here, because it would
+        //   recursively lock the mutex, leading to deadlock. Also,
+        //   that would emit changed() for each key.
         auto map = d.toVariant().toMap();
         for ( auto i = map.constBegin(); i != map.constEnd(); ++i )
         {
-            insert( i.key(), *i );
+            m.insert( i.key(), *i );
         }
         return true;
     }
@@ -141,8 +172,9 @@ GlobalStorage::load( const QString& filename )
 }
 
 bool
-GlobalStorage::saveYaml( const QString& filename )
+GlobalStorage::saveYaml( const QString& filename ) const
 {
+    ReadLock l( this );
     return CalamaresUtils::saveYaml( filename, m );
 }
 
@@ -150,12 +182,20 @@ bool
 GlobalStorage::loadYaml( const QString& filename )
 {
     bool ok = false;
-    auto gs = CalamaresUtils::loadYaml( filename, &ok );
+    auto map = CalamaresUtils::loadYaml( filename, &ok );
     if ( ok )
     {
-        m = gs;
+        WriteLock l( this );
+        // Do **not** use method insert() here, because it would
+        //   recursively lock the mutex, leading to deadlock. Also,
+        //   that would emit changed() for each key.
+        for ( auto i = map.constBegin(); i != map.constEnd(); ++i )
+        {
+            m.insert( i.key(), *i );
+        }
+        return true;
     }
-    return ok;
+    return false;
 }
 
 

src/libcalamares/GlobalStorage.h

@@ -2,6 +2,7 @@
  *
  *   SPDX-FileCopyrightText: 2014-2015 Teo Mrnjavac <teo@kde.org>
  *   SPDX-FileCopyrightText: 2017-2018 Adriaan de Groot <groot@kde.org>
+ *   SPDX-License-Identifier: GPL-3.0-or-later
  *
  *   Calamares is free software: you can redistribute it and/or modify
  *   it under the terms of the GNU General Public License as published by
@@ -16,16 +17,12 @@
  *   You should have received a copy of the GNU General Public License
  *   along with Calamares. If not, see <http://www.gnu.org/licenses/>.
  *
- *   SPDX-License-Identifier: GPL-3.0-or-later
- *   License-Filename: LICENSE
- *
  */
 
 #ifndef CALAMARES_GLOBALSTORAGE_H
 #define CALAMARES_GLOBALSTORAGE_H
 
-#include "CalamaresConfig.h"
-
+#include <QMutex>
 #include <QObject>
 #include <QString>
 #include <QVariantMap>
@@ -33,67 +30,146 @@
 namespace Calamares
 {
 
-class DebugWindow;
-
+/** @brief Storage for data that passes between Calamares modules.
+ *
+ * The Global Storage is global to the Calamars JobQueue and
+ * everything that depends on that: all of its modules use the
+ * same instance of the JobQueue, and so of the Global Storage.
+ *
+ * GS is used to pass data between modules; there is only convention
+ * about what keys are used, and individual modules should document
+ * what they put in to GS or what they expect to find in it.
+ *
+ * GS behaves as a basic key-value store, with a QVariantMap behind
+ * it. Any QVariant can be put into the storage, and the signal
+ * changed() is emitted when any data is modified.
+ *
+ * In general, see QVariantMap (possibly after calling data()) for details.
+ *
+ * This class is thread-safe -- most accesses go through JobQueue, which
+ * handles threading itself, but because modules load in parallel and can
+ * have asynchronous tasks like GeoIP lookups, the storage itself also
+ * has locking. All methods are thread-safe, use data() to make a snapshot
+ * copy for use outside of the thread-safe API.
+ */
 class GlobalStorage : public QObject
 {
     Q_OBJECT
 public:
+    /** @brief Create a GS object
+     *
+     * **Generally** there is only one GS object (hence, "global") which
+     * is owned by the JobQueue instance (which is a singleton). However,
+     * it is possible to create more GS objects.
+     */
     explicit GlobalStorage( QObject* parent = nullptr );
 
-    //NOTE: thread safety is guaranteed by JobQueue, which executes jobs one by one.
-    //      If at any time jobs become concurrent, this class must be made thread-safe.
+    /** @brief Insert a key and value into the store
+     *
+     * The @p value is added to the store with key @p key. If @p key
+     * already exists in the store, its existing value is overwritten.
+     * The changed() signal is emitted regardless.
+     */
     void insert( const QString& key, const QVariant& value );
+    /** @brief Removes a key and its value
+     *
+     * The @p key is removed from the store. If the @p key does not
+     * exist, nothing happens. changed() is emitted regardless.
+     *
+     * @return the number of keys remaining
+     */
     int remove( const QString& key );
 
-    /// @brief dump keys and values to the debug log
+    /** @brief dump keys and values to the debug log
+     *
+     * All the keys and their values are written to the debug log.
+     * See save() for caveats: this can leak sensitive information.
+     */
     void debugDump() const;
 
     /** @brief write as JSON to the given filename
+     *
+     * The file named @p filename is overwritten with a JSON representation
+     * of the entire global storage (this may be structured, for instance
+     * if maps or lists have been inserted).
      *
      * No tidying, sanitization, or censoring is done -- for instance,
      * the user module sets a slightly-obscured password in global storage,
      * and this JSON file will contain that password in-the-only-slightly-
      * obscured form.
      */
-    bool save( const QString& filename );
+    bool saveJson( const QString& filename ) const;
 
     /** @brief Adds the keys from the given JSON file
      *
      * No tidying, sanitization, or censoring is done.
      * The JSON file is read and each key is added as a value to
-     * the global storage.
+     * the global storage. The storage is not cleared first: existing
+     * keys will remain; keys that also occur in the JSON file are overwritten.
      */
-    bool load( const QString& filename );
+    bool loadJson( const QString& filename );
 
     /** @brief write as YAML to the given filename
      *
      * See also save(), above.
      */
-    bool saveYaml( const QString& filename );
+    bool saveYaml( const QString& filename ) const;
 
-    /// @brief reads settings from the given filename
+    /** @brief reads settings from the given filename
+     *
+     * See also load(), above.
+     */
     bool loadYaml( const QString& filename );
 
-    /** @brief Get internal mapping as a constant object
+    /** @brief Make a complete copy of the data
      *
-     * Note that the VariantMap underneath may change, because
-     * it's not constant in itself. Connect to the changed()
-     * signal for notifications.
+     * Provides a snapshot of the data at a given time.
      */
-    const QVariantMap& data() const { return m; }
+    QVariantMap data() const { return m; }
 
 public Q_SLOTS:
+    /** @brief Does the store contain the given key?
+     *
+     * This can distinguish an explicitly-inserted QVariant() from
+     * a no-value-exists QVariant. See value() for details.
+     */
     bool contains( const QString& key ) const;
+    /** @brief The number of keys in the store
+     *
+     * This should be unsigned, but the underlying QMap uses signed as well.
+     * Equal to keys().length(), in theory.
+     */
     int count() const;
+    /** @brief The keys in the store
+     *
+     * This makes a copy of all the keys currently in the store, which
+     * could be used for iterating over all the values in the store.
+     */
     QStringList keys() const;
+    /** @brief Gets a value from the store
+     *
+     * If a value has been previously inserted, returns that value.
+     * If @p key does not exist in the store, returns a QVariant()
+     * (an invalid QVariant, which boolean-converts to false). Since
+     * QVariant() van also be inserted explicitly, use contains()
+     * to check for the presence of a key if you need that.
+     */
     QVariant value( const QString& key ) const;
 
 signals:
+    /** @brief Emitted any time the store changes
+     *
+     * Also emitted sometimes when the store does not change, e.g.
+     * when removing a non-existent key or inserting a value that
+     * is already present.
+     */
     void changed();
 
 private:
+    class ReadLock;
+    class WriteLock;
     QVariantMap m;
+    mutable QMutex m_mutex;
 };
 
 }  // namespace Calamares

src/libcalamares/Tests.cpp

@@ -0,0 +1,147 @@
+/* === This file is part of Calamares - <https://github.com/calamares> ===
+ *
+ *   SPDX-FileCopyrightText: 2018-2020 Adriaan de Groot <groot@kde.org>
+ *   SPDX-License-Identifier: GPL-3.0-or-later
+ *
+ *
+ *   Calamares is free software: you can redistribute it and/or modify
+ *   it under the terms of the GNU General Public License as published by
+ *   the Free Software Foundation, either version 3 of the License, or
+ *   (at your option) any later version.
+ *
+ *   Calamares is distributed in the hope that it will be useful,
+ *   but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ *   GNU General Public License for more details.
+ *
+ *   You should have received a copy of the GNU General Public License
+ *   along with Calamares. If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#include "GlobalStorage.h"
+
+#include "utils/Logger.h"
+
+#include <QObject>
+#include <QSignalSpy>
+#include <QtTest/QtTest>
+
+class TestLibCalamares : public QObject
+{
+    Q_OBJECT
+public:
+    TestLibCalamares() {}
+    virtual ~TestLibCalamares() {}
+
+private Q_SLOTS:
+    void testGSModify();
+    void testGSLoadSave();
+    void testGSLoadSave2();
+};
+
+void
+TestLibCalamares::testGSModify()
+{
+    Calamares::GlobalStorage gs;
+    QSignalSpy spy( &gs, &Calamares::GlobalStorage::changed );
+
+    const QString key( "derp" );
+
+    QCOMPARE( gs.count(), 0 );
+    QVERIFY( !gs.contains( key ) );
+
+    const int value = 17;
+    gs.insert( key, value );
+    QCOMPARE( gs.count(), 1 );
+    QVERIFY( gs.contains( key ) );
+    QCOMPARE( gs.value( key ).type(), QVariant::Int );
+    QCOMPARE( gs.value( key ).toString(), QString( "17" ) );  // It isn't a string, but does convert
+    QCOMPARE( gs.value( key ).toInt(), value );
+
+    gs.remove( key );
+    QCOMPARE( gs.count(), 0 );
+    QVERIFY( !gs.contains( key ) );
+
+    QCOMPARE( spy.count(), 2 );  // one insert, one remove
+}
+
+void
+TestLibCalamares::testGSLoadSave()
+{
+    Calamares::GlobalStorage gs;
+    const QString jsonfilename( "gs.test.json" );
+    const QString yamlfilename( "gs.test.yaml" );
+
+    gs.insert( "derp", 17 );
+    gs.insert( "cow", "moo" );
+    gs.insert( "dwarfs", QStringList { "dopey", "sneezy" } );
+
+    QCOMPARE( gs.count(), 3 );
+
+    QVERIFY( gs.saveJson( jsonfilename ) );
+    Calamares::GlobalStorage gs2;
+    QCOMPARE( gs2.count(), 0 );
+    QVERIFY( gs2.loadJson( jsonfilename ) );
+    QCOMPARE( gs2.count(), 3 );
+    QCOMPARE( gs2.data(), gs.data() );
+
+    QVERIFY( gs.saveYaml( yamlfilename ) );
+    Calamares::GlobalStorage gs3;
+    QCOMPARE( gs3.count(), 0 );
+    QVERIFY( gs3.loadYaml( jsonfilename ) );
+    QCOMPARE( gs3.count(), 3 );
+    QCOMPARE( gs3.data(), gs.data() );
+
+    // Failures in loading
+    QVERIFY( !gs3.loadYaml( jsonfilename ) );
+    QVERIFY( !gs3.loadJson( yamlfilename ) );
+
+    Calamares::GlobalStorage gs4;
+    gs4.insert( "derp", 32 );
+    gs4.insert( "dorp", "Varsseveld" );
+    QCOMPARE( gs4.count(), 2 );
+    QVERIFY( gs4.contains( "dorp" ) );
+    QCOMPARE( gs4.value( "derp" ).toInt(), 32 );
+    QVERIFY( gs4.loadJson( jsonfilename ) );
+    // 3 keys from the file, but one overwrite
+    QCOMPARE( gs4.count(), 4 );
+    QVERIFY( gs4.contains( "dorp" ) );
+    QCOMPARE( gs4.value( "derp" ).toInt(), 17 );  // This one was overwritten
+}
+
+void
+TestLibCalamares::testGSLoadSave2()
+{
+    Logger::setupLogLevel( Logger::LOGDEBUG );
+
+    const QString filename( "../src/libcalamares/testdata/yaml-list.conf" );
+    if ( !QFile::exists( filename ) )
+    {
+        return;
+    }
+
+    Calamares::GlobalStorage gs1;
+    const QString key( "dwarfs" );
+
+    QVERIFY( gs1.loadYaml( filename ) );
+    QCOMPARE( gs1.count(), 3 );  // From examining the file
+    QVERIFY( gs1.contains( key ) );
+    cDebug() << gs1.value( key ).type() << gs1.value( key );
+    QCOMPARE( gs1.value( key ).type(), QVariant::List );
+
+    const QString yamlfilename( "gs.test.yaml" );
+    QVERIFY( gs1.saveYaml( yamlfilename ) );
+
+    Calamares::GlobalStorage gs2;
+    QVERIFY( gs2.loadYaml( yamlfilename ) );
+    QVERIFY( gs2.contains( key ) );
+    QCOMPARE( gs2.value( key ).type(), QVariant::List );
+}
+
+
+QTEST_GUILESS_MAIN( TestLibCalamares )
+
+#include "utils/moc-warnings.h"
+
+#include "Tests.moc"

src/libcalamares/testdata/yaml-list.conf

@@ -0,0 +1,8 @@
+# YAML dump
+---
+"cow": "moo"
+"derp": 17
+"dwarfs":
+    - "sleepy"
+    - "sneezy"
+    - "doc"

src/libcalamares/utils/Tests.cpp

@@ -148,6 +148,7 @@ LibCalamaresTests::recursiveCompareMap( const QVariantMap& a, const QVariantMap&
 void
 LibCalamaresTests::testLoadSaveYamlExtended()
 {
+    Logger::setupLogLevel( Logger::LOGDEBUG );
     bool loaded_ok;
     for ( const auto& confname : findConf( QDir( "../src" ) ) )
     {

src/modules/preservefiles/PreserveFiles.cpp

@@ -138,7 +138,7 @@ PreserveFiles::exec()
         }
         if ( it.type == ItemType::Config )
         {
-            if ( Calamares::JobQueue::instance()->globalStorage()->save( dest ) )
+            if ( Calamares::JobQueue::instance()->globalStorage()->saveJson( dest ) )
             {
                 cWarning() << "Could not write config for" << dest;
             }