Merge lp:~michihenning/thumbnailer/config-access into lp:thumbnailer/devel

[Michi Henning]

Made client-side config settings available from the server.

Made client-side config settings available from the server. That's because we can't access gsettings from the client-side API when under confinement. Because the Settings class is no longer used on the client side, I moved that back into the internal namespace.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:327
http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-ci/621/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-vivid-amd64-ci/325
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-vivid-armhf-ci/326
        deb: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-vivid-armhf-ci/326/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-vivid-i386-ci/326
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-xenial-amd64-ci/75
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-xenial-armhf-ci/76
        deb: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-xenial-armhf-ci/76/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-xenial-i386-ci/73

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/thumbnailer-devel-ci/621/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:328
http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-ci/622/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-vivid-amd64-ci/326
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-vivid-armhf-ci/327
        deb: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-vivid-armhf-ci/327/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-vivid-i386-ci/327
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-xenial-amd64-ci/76
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-xenial-armhf-ci/77
        deb: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-xenial-armhf-ci/77/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-xenial-i386-ci/74

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/thumbnailer-devel-ci/622/rebuild

review: Approve (continuous-integration)

[Michi Henning (michihenning)]

Related branch for the apparmor profile update:

https://code.launchpad.net/~michihenning/apparmor-easyprof-ubuntu/new-thumbnailer-methods/+merge/281712

Note that this change can land regardless. If apparmor denies access, we use the defaults, so it'll magically work once apparmor catches up.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:331
http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-ci/634/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-vivid-amd64-ci/338/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-vivid-armhf-ci/339/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-vivid-i386-ci/339/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-xenial-amd64-ci/88/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-xenial-armhf-ci/89/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-xenial-i386-ci/86/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/thumbnailer-devel-ci/634/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:332
http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-ci/635/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-vivid-amd64-ci/339
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-vivid-armhf-ci/340
        deb: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-vivid-armhf-ci/340/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-vivid-i386-ci/340
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-xenial-amd64-ci/89
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-xenial-armhf-ci/90
        deb: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-xenial-armhf-ci/90/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-xenial-i386-ci/87

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/thumbnailer-devel-ci/635/rebuild

review: Approve (continuous-integration)

[James Henstridge (jamesh)]

I've left some inline comments. There are a few things to fix surrounding the use of the new D-Bus calls.

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:333
http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-ci/636/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-vivid-amd64-ci/340
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-vivid-armhf-ci/341
        deb: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-vivid-armhf-ci/341/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-vivid-i386-ci/341
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-xenial-amd64-ci/90
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-xenial-armhf-ci/91
        deb: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-xenial-armhf-ci/91/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-xenial-i386-ci/88

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/thumbnailer-devel-ci/636/rebuild

review: Approve (continuous-integration)

[James Henstridge (jamesh)]

Looks better. I've left one more small note inline.

[Michi Henning (michihenning)]

Thanks for that!

It's also broken in another way: the number of CPUs really is irrelevant here. I wasn't thinking straight. What we want is 10 if max-downloads is set to 0, and whatever max-downloads is set to, otherwise, regardless of the number of CPUs. (As is, we are too low for a 2-core or 4-core machine because we need max-downloads at 10, even when there are fewer than 8 cores.)

I'll fix it.

[Michi Henning (michihenning)]

OK, should be good now. Default backlog is 10 now, regardless of CPU cores. I also fixed the QDBusReply thing. The explicit definition is needed because template deduction can't be used for the type returned by the invocation stub.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:334
http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-ci/637/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-vivid-amd64-ci/341
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-vivid-armhf-ci/342
        deb: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-vivid-armhf-ci/342/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-vivid-i386-ci/342
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-xenial-amd64-ci/91
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-xenial-armhf-ci/92
        deb: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-xenial-armhf-ci/92/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/thumbnailer-devel-xenial-i386-ci/89

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/thumbnailer-devel-ci/637/rebuild

review: Approve (continuous-integration)

[lp:~michihenning/thumbnailer/config-access]

335. By Michi Henning on 2016-01-13

Simplified dbus invocation.

[James Henstridge (jamesh)]

Looks good now.

review: Approve

=== modified file 'data/com.canonical.Unity.Thumbnailer.gschema.xml'
--- data/com.canonical.Unity.Thumbnailer.gschema.xml	2016-01-07 01:21:03 +0000
+++ data/com.canonical.Unity.Thumbnailer.gschema.xml	2016-01-13 07:08:16 +0000
@@ -77,7 +77,7 @@
       <default>10</default>
       <summary>Maximum number of pending DBus requests before the thumbnailer starts queuing them.</summary>
       <description>
-        This parameter limits the number of pending DBus requests to the thumbnailer service. If there are more requests than this currently in progress, additional requests are queued and sent once the backlog drops below the limit.
+        This parameter limits the number of pending DBus requests to the thumbnailer service. If the number of concurrent requests exceeds the backlog, additional requests are queued and sent once the backlog drops below the limit.
      </description>
     </key>
 

=== modified file 'debian/changelog'
--- debian/changelog	2016-01-09 03:22:11 +0000
+++ debian/changelog	2016-01-13 07:08:16 +0000
@@ -10,6 +10,7 @@
   [ Michi Henning ]
   * Fixed incorrect display of cache policy by "thumbnailer-admin stats".
   * Limit GIF file size to 2 MB (LP: #1527315).
+  * Fix non-working config settings for client-side API (LP: #1528058).
 
   [ CI Train Bot ]
   * No-change rebuild.

=== renamed file 'include/settings.h' => 'include/internal/settings.h'
--- include/settings.h	2016-01-04 06:09:41 +0000
+++ include/internal/settings.h	2016-01-13 07:08:16 +0000
@@ -32,6 +32,9 @@
 namespace thumbnailer
 {
 
+namespace internal
+{
+
 class Settings
 {
 public:
@@ -66,6 +69,8 @@
     internal::gobj_ptr<GSettings> settings_;
 };
 
+}  // namespace internal
+
 }  // namespace thumbnailer
 
 }  // namespace unity

=== modified file 'man/thumbnailer-settings.5'
--- man/thumbnailer-settings.5	2016-01-07 01:21:03 +0000
+++ man/thumbnailer-settings.5	2016-01-13 07:08:16 +0000
@@ -59,7 +59,7 @@
 .TP
 .B max\-backlog \fR(int)\fP
 Controls the number of DBus requests that will be sent before queueing the requests internally.
-The default is 10 requests.
+The default is 10.
 .TP
 .B trace\-client \fR(bool)\fP
 If true, thumbnail and cancel requests are logged. Log messages are written to the calling application's log

=== modified file 'plugins/Ubuntu/Thumbnailer.0.1/plugin.cpp'
--- plugins/Ubuntu/Thumbnailer.0.1/plugin.cpp	2015-09-30 10:08:20 +0000
+++ plugins/Ubuntu/Thumbnailer.0.1/plugin.cpp	2016-01-13 07:08:16 +0000
@@ -18,7 +18,6 @@
 
 #include "plugin.h"
 
-#include <settings.h>
 #include "albumartgenerator.h"
 #include "artistartgenerator.h"
 #include "thumbnailgenerator.h"

=== modified file 'plugins/Ubuntu/Thumbnailer.0.1/thumbnailgenerator.cpp'
--- plugins/Ubuntu/Thumbnailer.0.1/thumbnailgenerator.cpp	2015-11-04 07:16:12 +0000
+++ plugins/Ubuntu/Thumbnailer.0.1/thumbnailgenerator.cpp	2016-01-13 07:08:16 +0000
@@ -22,7 +22,6 @@
 #include <QMimeDatabase>
 #include <QUrl>
 
-#include <settings.h>
 #include "thumbnailerimageresponse.h"
 
 namespace

=== modified file 'src/libthumbnailer-qt/CMakeLists.txt'
--- src/libthumbnailer-qt/CMakeLists.txt	2015-11-30 06:36:53 +0000
+++ src/libthumbnailer-qt/CMakeLists.txt	2016-01-13 07:08:16 +0000
@@ -9,6 +9,8 @@
   ${interface_files}
 )
 
+include_directories(${CMAKE_BINARY_DIR}/src)
+
 set_target_properties(${LIBTHUMBNAILER_QT} PROPERTIES
     VERSION "${LIBTHUMBNAILER_QT_SO_VERSION_MAJOR}.${LIBTHUMBNAILER_QT_SO_VERSION_MINOR}.${LIBTHUMBNAILER_QT_SO_VERSION_MICRO}"
     SOVERSION ${LIBTHUMBNAILER_QT_SO_VERSION}

=== modified file 'src/libthumbnailer-qt/libthumbnailer-qt.cpp'
--- src/libthumbnailer-qt/libthumbnailer-qt.cpp	2015-12-21 05:46:02 +0000
+++ src/libthumbnailer-qt/libthumbnailer-qt.cpp	2016-01-13 07:08:16 +0000
@@ -19,8 +19,8 @@
 
 #include <unity/thumbnailer/qt/thumbnailer-qt.h>
 
+#include <settings-defaults.h>
 #include <ratelimiter.h>
-#include <settings.h>
 #include <thumbnailerinterface.h>
 #include <service/dbus_names.h>
 
@@ -132,8 +132,8 @@
                                           QSize const& requested_size,
                                           std::function<QDBusPendingReply<QByteArray>()> const& job);
     std::unique_ptr<ThumbnailerInterface> iface_;
-    RateLimiter limiter_;
     bool trace_client_;
+    std::unique_ptr<RateLimiter> limiter_;
 };
 
 RequestImpl::RequestImpl(QString const& details,
@@ -316,10 +316,51 @@
 }
 
 ThumbnailerImpl::ThumbnailerImpl(QDBusConnection const& connection)
-    : limiter_(Settings().max_backlog())
-    , trace_client_(Settings().trace_client())
 {
     iface_.reset(new ThumbnailerInterface(service::BUS_NAME, service::THUMBNAILER_BUS_PATH, connection));
+
+    // We need to retrieve config parameters from the server because, when an app runs confined,
+    // it cannot read gsettings. We do this synchronously because we can't do anything else until
+    // after we get the settings anyway.
+
+    auto trace_client_call = iface_->TraceClient();
+    auto max_backlog_call = iface_->MaxBacklog();
+
+    {
+        trace_client_call.waitForFinished();
+        if (trace_client_call.isValid())
+        {
+            trace_client_ = trace_client_call.value();
+        }
+        // LCOV_EXCL_START
+        else
+        {
+            bool const dflt = TRACE_CLIENT_DEFAULT;
+            trace_client_ = dflt;
+            qCritical().nospace() << "could not retrieve trace-client setting: " << trace_client_call.error().message()
+                                  << " (using default value of " << dflt << ")";
+
+        }
+        // LCOV_EXCL_STOP
+    }
+
+    {
+        int constexpr dflt_backlog = MAX_BACKLOG_DEFAULT;
+        max_backlog_call.waitForFinished();
+        if (max_backlog_call.isValid())
+        {
+            int backlog = max_backlog_call.value();
+            limiter_.reset(new RateLimiter(backlog));
+        }
+        // LCOV_EXCL_START
+        else
+        {
+            limiter_.reset(new RateLimiter(dflt_backlog));
+            qCritical().nospace() << "could not retrieve max-backlog setting: " << max_backlog_call.error().message()
+                                  << " (using default value of " << dflt_backlog << ")";
+        }
+        // LCOV_EXCL_STOP
+    }
 }
 
 QSharedPointer<Request> ThumbnailerImpl::getAlbumArt(QString const& artist,
@@ -384,12 +425,12 @@
 
 RateLimiter& ThumbnailerImpl::limiter()
 {
-    return limiter_;
+    return *limiter_;
 }
 
 void ThumbnailerImpl::pump_limiter()
 {
-    return limiter_.done();
+    return limiter_->done();
 }
 
 }  // namespace internal

=== modified file 'src/service/dbusinterface.cpp'
--- src/service/dbusinterface.cpp	2016-01-10 07:00:47 +0000
+++ src/service/dbusinterface.cpp	2016-01-13 07:08:16 +0000
@@ -19,7 +19,6 @@
 #include "dbusinterface.h"
 
 #include <internal/file_io.h>
-#include <settings.h>
 
 #include <boost/algorithm/string.hpp>
 #include <boost/regex.hpp>
@@ -94,8 +93,9 @@
 
     extraction_limiter_ = make_shared<RateLimiter>(limit);
 
-    Settings s;
-    log_level_ = s.log_level();
+    log_level_ = settings_.log_level();
+    trace_client_ = settings_.trace_client();
+    max_backlog_ = settings_.max_backlog();
 }
 
 DBusInterface::~DBusInterface()
@@ -305,6 +305,16 @@
     }
 }
 
+bool DBusInterface::TraceClient()
+{
+    return trace_client_;
+}
+
+int DBusInterface::MaxBacklog()
+{
+    return max_backlog_;
+}
+
 }  // namespace service
 
 }  // namespace thumbnailer

=== modified file 'src/service/dbusinterface.h'
--- src/service/dbusinterface.h	2015-12-14 04:43:11 +0000
+++ src/service/dbusinterface.h	2016-01-13 07:08:16 +0000
@@ -22,7 +22,7 @@
 #include "handler.h"
 
 #include <ratelimiter.h>
-#include <settings.h>
+#include <internal/settings.h>
 
 #include <QDBusContext>
 #include <QThreadPool>
@@ -53,6 +53,11 @@
     QByteArray GetArtistArt(QString const& artist, QString const& album, QSize const& requestedSize);
     QByteArray GetThumbnail(QString const& filename, QSize const& requestedSize);
 
+    // These methods return the value of the corresponding gsettings key. We need to do this on the server
+    // side because the client-side API runs under confinement, which disallows access to gsettings.
+    bool TraceClient();
+    int MaxBacklog();
+
 private:
     void queueRequest(Handler* handler);
 
@@ -72,10 +77,12 @@
     std::shared_ptr<QThreadPool> create_thread_pool_;
     std::map<Handler*, std::unique_ptr<Handler>> requests_;
     std::map<std::string, std::vector<Handler*>> request_keys_;
-    unity::thumbnailer::Settings settings_;
+    unity::thumbnailer::internal::Settings settings_;
     std::shared_ptr<RateLimiter> download_limiter_;
     std::shared_ptr<RateLimiter> extraction_limiter_;
     int log_level_;
+    bool trace_client_;
+    int max_backlog_;
 };
 
 }  // namespace service

=== modified file 'src/service/dbusinterface.xml'
--- src/service/dbusinterface.xml	2015-12-03 02:01:47 +0000
+++ src/service/dbusinterface.xml	2016-01-13 07:08:16 +0000
@@ -20,5 +20,11 @@
       <arg direction="out" type="ay" name="thumbnail" />
       <annotation name="org.qtproject.QtDBus.QtTypeName.In1" value="QSize" />
     </method>
+    <method name="TraceClient">
+      <arg direction="out" type="b" name="traceClient" />
+    </method>
+    <method name="MaxBacklog">
+      <arg direction="out" type="i" name="maxBacklog" />
+    </method>
   </interface>
 </node>

=== modified file 'src/settings.cpp'
--- src/settings.cpp	2016-01-04 06:09:41 +0000
+++ src/settings.cpp	2016-01-13 07:08:16 +0000
@@ -16,7 +16,7 @@
  * Authored by: James Henstridge <[e-mail]>
  */
 
-#include <settings.h>
+#include <internal/settings.h>
 
 #include <internal/env_vars.h>
 #include "settings-defaults.h"
@@ -39,6 +39,9 @@
 namespace thumbnailer
 {
 
+namespace internal
+{
+
 Settings::Settings()
     : Settings("com.canonical.Unity.Thumbnailer")
 {
@@ -220,6 +223,8 @@
     return g_settings_get_boolean(settings_.get(), key);
 }
 
+}  // namespace internal
+
 }  // namespace thumbnailer
 
 }  // namespace unity

=== modified file 'src/thumbnailer.cpp'
--- src/thumbnailer.cpp	2016-01-08 09:22:07 +0000
+++ src/thumbnailer.cpp	2016-01-13 07:08:16 +0000
@@ -28,8 +28,8 @@
 #include <internal/make_directories.h>
 #include <internal/raii.h>
 #include <internal/safe_strerror.h>
+#include <internal/settings.h>
 #include <internal/ubuntuserverdownloader.h>
-#include <settings.h>
 
 #include <boost/filesystem.hpp>
 

=== modified file 'src/ubuntuserverdownloader.cpp'
--- src/ubuntuserverdownloader.cpp	2015-12-29 23:04:19 +0000
+++ src/ubuntuserverdownloader.cpp	2016-01-13 07:08:16 +0000
@@ -22,7 +22,7 @@
 
 #include <internal/artreply.h>
 #include <internal/env_vars.h>
-#include <settings.h>
+#include <internal/settings.h>
 
 #include <QNetworkReply>
 #include <QTimer>

=== modified file 'tests/settings/settings_test.cpp'
--- tests/settings/settings_test.cpp	2016-01-07 01:21:03 +0000
+++ tests/settings/settings_test.cpp	2016-01-13 07:08:16 +0000
@@ -18,7 +18,7 @@
 
 #include <internal/env_vars.h>
 #include <internal/gobj_memory.h>
-#include <settings.h>
+#include <internal/settings.h>
 #include <testsetup.h>
 #include <utils/env_var_guard.h>