Merge lp:~mandel/ubuntu-download-manager/system-bus into lp:ubuntu-download-manager

[Manuel de la Peña]

Provide system dbus API to be used by apps such as the image updates.

Provide system dbus API to be used by apps such as the image updates.

[Manuel de la Peña (mandel)]

Please allow me to do the global approve, when this branch lands on trunk is very important due to deployment details.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:115
http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-ci/62/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-saucy-amd64-ci/62
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-saucy-armhf-ci/53

Click here to trigger a rebuild:
http://s-jenkins:8080/job/ubuntu-download-manager-ci/62/rebuild

review: Approve (continuous-integration)

[Mike McCracken (mikemc)]

irl_tests.py with system bus seems to work OK for me.
I was a little thrown by the parameters in progress_callback() being
backwards, though :)

really minor note - looks like the added calls to lastError() in
download_daemon.cpp don't add a space before they dump
"QDBUSError(whatever)", so maybe it'd be a good idea to pad the string
literal before them.

super nitpicky naming suggestion - initSessionBus() and
initSystemBus() sound like they are initializing the busses. Why not
name them after what they'll do, like openLogFile() and
openSyslogConnection() or something?

also, a note for the future branch where you add syslog - why not
separate generating the message (which looks the same for both cases)
from sending it to the logfile/syslog? logSystemMessage() and
logSessionMessage share a lot of code.

review: Approve

[Mike McCracken (mikemc)]

never mind the note about a space before lastError(). TIL qDebug does some padding of its own.

[Mike McCracken (mikemc)]

…and never mind the note about the progress_callback() parameters being flipped, I see the next branch fixes that

[Manuel de la Peña (mandel)]

> irl_tests.py with system bus seems to work OK for me.
> I was a little thrown by the parameters in progress_callback() being
> backwards, though :)
>
> really minor note - looks like the added calls to lastError() in
> download_daemon.cpp don't add a space before they dump
> "QDBUSError(whatever)", so maybe it'd be a good idea to pad the string
> literal before them.
>
> super nitpicky naming suggestion - initSessionBus() and
> initSystemBus() sound like they are initializing the busses. Why not
> name them after what they'll do, like openLogFile() and
> openSyslogConnection() or something?

Sure, lets do that is a goof idea and better now than never.

>
> also, a note for the future branch where you add syslog - why not
> separate generating the message (which looks the same for both cases)
> from sending it to the logfile/syslog? logSystemMessage() and
> logSessionMessage share a lot of code.

Lets do it know, same reason as above.

[Diego Sarmentero (diegosarmentero)]

+1

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:116
http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-ci/68/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-saucy-amd64-ci/68
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-saucy-armhf-ci/59

Click here to trigger a rebuild:
http://s-jenkins:8080/job/ubuntu-download-manager-ci/68/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-autolanding/40/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-saucy-amd64-autolanding/40/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-saucy-armhf-autolanding/38/console

review: Needs Fixing (continuous-integration)

[lp:~mandel/ubuntu-download-manager/system-bus]

117. By Manuel de la Peña on 2013-09-24

Merged with trunk.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:117
http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-ci/70/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-saucy-amd64-ci/70
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-download-manager-saucy-armhf-ci/61

Click here to trigger a rebuild:
http://s-jenkins:8080/job/ubuntu-download-manager-ci/70/rebuild

review: Approve (continuous-integration)

=== modified file 'debian/ubuntu-download-manager.install'
--- debian/ubuntu-download-manager.install	2013-07-21 20:16:06 +0000
+++ debian/ubuntu-download-manager.install	2013-09-24 09:53:05 +0000
@@ -1,3 +1,4 @@
 usr/bin/ubuntu-download-manager
 usr/share/dbus-1/services/ubuntu-download-manager.service
-
+usr/share/dbus-1/system-services/com.canonical.applications.Downloader.service
+etc/dbus-1/system.d/com.canonical.applications.Downloader.conf

=== modified file 'libubuntudownloadmanager/apparmor.cpp'
--- libubuntudownloadmanager/apparmor.cpp	2013-09-18 10:18:24 +0000
+++ libubuntudownloadmanager/apparmor.cpp	2013-09-24 09:53:05 +0000
@@ -19,6 +19,8 @@
 #include <errno.h>
 #include <nih/alloc.h>
 #include <libnih-dbus.h>
+#include <sys/types.h>
+#include <unistd.h>
 #include <QDBusConnection>
 #include <QDebug>
 #include <QRegExp>
@@ -35,13 +37,21 @@
     Q_DECLARE_PUBLIC(AppArmor)
 
  public:
-    explicit AppArmorPrivate(AppArmor* parent)
+    AppArmorPrivate(AppArmor* parent)
         : q_ptr(parent) {
         _dbus = new DBusProxy("org.freedesktop.DBus", "/",
             QDBusConnection::sessionBus());
         _uuidFactory = new UuidFactory();
     }
 
+    AppArmorPrivate(QSharedPointer<DBusConnection> connection,
+                    AppArmor* parent)
+        : q_ptr(parent) {
+        _dbus = new DBusProxy("org.freedesktop.DBus", "/",
+            connection->connection());
+        _uuidFactory = new UuidFactory();
+    }
+
     QString getUuidPath(QUuid id, QString path) {
         qDebug() << __PRETTY_FUNCTION__ << path;
         QString idString = path + "/" + id.toString().replace(
@@ -65,17 +75,24 @@
     }
 
     QString getLocalPath(const QString& appId) {
-        QStringList pathComponents;
-        if (!appId.isEmpty()) {
-            QStringList appIdInfo = appId.split("_");
-            if (appIdInfo.count() > 0)
-                pathComponents << appIdInfo[0];
-        }
+        // if the service is running as root we will always return /tmp
+        // as the local path root
+        if (getuid() == 0){
+            qDebug() << "Running as system bus using /tmp for downloads";
+            return QString("/tmp");
+        } else {
+            QStringList pathComponents;
+            if (!appId.isEmpty()) {
+                QStringList appIdInfo = appId.split("_");
+                if (appIdInfo.count() > 0)
+                    pathComponents << appIdInfo[0];
+            }
 
-        pathComponents << "Downloads";
-        QString localPath = XDGBasedir::saveDataPath(pathComponents);
-        qDebug() << "Local path is" << localPath;
-        return localPath;
+            pathComponents << "Downloads";
+            QString localPath = XDGBasedir::saveDataPath(pathComponents);
+            qDebug() << "Local path is" << localPath;
+            return localPath;
+        }  // not root
     }
 
     void getSecurePath(const QString& connName,
@@ -159,6 +176,12 @@
       d_ptr(new AppArmorPrivate(this)) {
 }
 
+AppArmor::AppArmor(QSharedPointer<DBusConnection> connection,
+                   QObject *parent)
+    : QObject(parent),
+      d_ptr(new AppArmorPrivate(connection, this)) {
+}
+
 void
 AppArmor::getDBusPath(QUuid& id, QString& dbusPath) {
     Q_D(AppArmor);

=== modified file 'libubuntudownloadmanager/apparmor.h'
--- libubuntudownloadmanager/apparmor.h	2013-09-18 10:18:24 +0000
+++ libubuntudownloadmanager/apparmor.h	2013-09-24 09:53:05 +0000
@@ -22,7 +22,9 @@
 #include <QObject>
 #include <QPair>
 #include <QString>
+#include <QSharedPointer>
 #include <QUuid>
+#include "./dbus_connection.h"
 
 class AppArmorPrivate;
 class AppArmor : public QObject {
@@ -31,6 +33,7 @@
 
  public:
     explicit AppArmor(QObject *parent = 0);
+    AppArmor(QSharedPointer<DBusConnection> connection, QObject *parent = 0);
 
     virtual void getDBusPath(QUuid& id, QString& dbusPath);
 

=== modified file 'libubuntudownloadmanager/dbus_connection.cpp'
--- libubuntudownloadmanager/dbus_connection.cpp	2013-07-23 15:25:55 +0000
+++ libubuntudownloadmanager/dbus_connection.cpp	2013-09-24 09:53:05 +0000
@@ -26,42 +26,37 @@
 class DBusConnectionPrivate {
     Q_DECLARE_PUBLIC(DBusConnection)
  public:
-    explicit DBusConnectionPrivate(DBusConnection* parent);
-
-    bool registerService(const QString& serviceName);
-    bool registerObject(const QString& path, QObject* object,
-        QDBusConnection::RegisterOptions options = QDBusConnection::ExportAdaptors);  // NOLINT(whitespace/line_length)
+    DBusConnectionPrivate(DBusConnection* parent)
+        : _conn(QDBusConnection::connectToBus(QDBusConnection::ActivationBus, "DBUS")),
+         q_ptr(parent) {
+    }
+
+    bool registerService(const QString& serviceName) {
+        return _conn.registerService(serviceName);
+    }
+
+    bool registerObject(const QString& path,
+                        QObject* object,
+                        QDBusConnection::RegisterOptions options =
+                            QDBusConnection::ExportAdaptors) {
+        return _conn.registerObject(path, object, options);
+    }
+
     void unregisterObject(const QString& path,
-        QDBusConnection::UnregisterMode mode = QDBusConnection::UnregisterNode);
+                          QDBusConnection::UnregisterMode mode =
+                              QDBusConnection::UnregisterNode) {
+        return _conn.unregisterObject(path, mode);
+    }
+
+    QDBusConnection connection() {
+        return _conn;
+    }
 
  private:
     QDBusConnection _conn;
     DBusConnection* q_ptr;
 };
 
-DBusConnectionPrivate::DBusConnectionPrivate(DBusConnection* parent)
-    : _conn(QDBusConnection::sessionBus()),
-      q_ptr(parent) {
-}
-
-bool
-DBusConnectionPrivate::registerService(const QString& serviceName) {
-    return _conn.registerService(serviceName);
-}
-
-bool
-DBusConnectionPrivate::registerObject(const QString& path,
-                                      QObject* object,
-                                      QDBusConnection::RegisterOptions options) {  // NOLINT(whitespace/line_length)
-    return _conn.registerObject(path, object, options);
-}
-
-void
-DBusConnectionPrivate::unregisterObject(const QString& path,
-    QDBusConnection::UnregisterMode mode) {
-    return _conn.unregisterObject(path, mode);
-}
-
 /*
  * PUBLIC IMPLEMENTATION
  */
@@ -92,3 +87,8 @@
     d->unregisterObject(path, mode);
 }
 
+QDBusConnection
+DBusConnection::connection() {
+    Q_D(DBusConnection);
+    return d->connection();
+}

=== modified file 'libubuntudownloadmanager/dbus_connection.h'
--- libubuntudownloadmanager/dbus_connection.h	2013-07-23 14:03:56 +0000
+++ libubuntudownloadmanager/dbus_connection.h	2013-09-24 09:53:05 +0000
@@ -34,6 +34,7 @@
         QDBusConnection::RegisterOptions options = QDBusConnection::ExportAdaptors);  // NOLINT(whitespace/line_length)
     virtual void unregisterObject(const QString& path,
         QDBusConnection::UnregisterMode mode = QDBusConnection::UnregisterNode);
+    virtual QDBusConnection connection();
 
  private:
     DBusConnectionPrivate* d_ptr;

=== modified file 'libubuntudownloadmanager/download_daemon.cpp'
--- libubuntudownloadmanager/download_daemon.cpp	2013-09-18 10:38:59 +0000
+++ libubuntudownloadmanager/download_daemon.cpp	2013-09-24 09:53:05 +0000
@@ -83,12 +83,14 @@
             ret = _conn->registerObject("/", _downInterface);
             qDebug() << ret;
             if (!ret) {
-                qDebug() << "Could not register interface";
+                qDebug() << "Could not register interface"
+                    << _conn->connection().lastError();
                 _app->exit(-1);
             }
             return;
         }
-        qDebug() << "Could not register service";
+        qDebug() << "Could not register service"
+            << _conn->connection().lastError();
         _app->exit(-1);
     }
 

=== modified file 'libubuntudownloadmanager/download_manager.cpp'
--- libubuntudownloadmanager/download_manager.cpp	2013-09-17 10:37:08 +0000
+++ libubuntudownloadmanager/download_manager.cpp	2013-09-24 09:53:05 +0000
@@ -37,7 +37,7 @@
         : _throttle(0),
           q_ptr(parent) {
         _conn = connection;
-        _apparmor = QSharedPointer<AppArmor>(new AppArmor());
+        _apparmor = QSharedPointer<AppArmor>(new AppArmor(connection));
         _networkInfo = QSharedPointer<SystemNetworkInfo>(
             new SystemNetworkInfo());
         QSharedPointer<RequestFactory> nam = QSharedPointer<RequestFactory>(

=== modified file 'libubuntudownloadmanager/logger.cpp'
--- libubuntudownloadmanager/logger.cpp	2013-07-26 15:24:30 +0000
+++ libubuntudownloadmanager/logger.cpp	2013-09-24 09:53:05 +0000
@@ -16,6 +16,8 @@
  * Boston, MA 02110-1301, USA.
  */
 
+#include <unistd.h>
+#include <sys/types.h>
 #include <QDateTime>
 #include <QDir>
 #include <QStandardPaths>
@@ -37,6 +39,22 @@
 }
 
 Logger::Logger(const QString filename) {
+    // decide if we are dealing with a system bus (that should use syslog)
+    // or a session bus
+    _isSystemBus = getuid() == 0;
+    if (_isSystemBus) {
+        openSyslogConnection();
+    } else {
+        openLogFile(filename);
+    }
+
+    qInstallMessageHandler(_realMessageHandler);
+
+    _initialized = true;
+}
+
+void
+Logger::openLogFile(const QString& filename) {
     if (filename == "") {
         _logFileName = getLogDir() + "/ubuntu-download-manager.log";
     } else {
@@ -48,10 +66,11 @@
         _logStream.setDevice(&_logFile);
         _logStream.flush();
     }
-
-    qInstallMessageHandler(_realMessageHandler);
-
-    _initialized = true;
+}
+
+void
+Logger::openSyslogConnection() {
+    // TODO(mandel): init syslog
 }
 
 void
@@ -106,6 +125,17 @@
 }
 
 void
+Logger::logSessionMessage(const QString &message) {
+    _logStream << message;
+    _logStream.flush();
+}
+
+void
+Logger::logSystemMessage(const QString &message) {
+    Q_UNUSED(message);
+}
+
+void
 Logger::logMessage(QtMsgType type,
                    const QMessageLogContext &context,
                    const QString &message) {
@@ -132,8 +162,11 @@
     }
     _stdErr.device()->close();
 
-    _logStream << logMessage;
-    _logStream.flush();
+    if (_isSystemBus) {
+        logSystemMessage(logMessage);
+    } else {
+        logSessionMessage(logMessage);
+    }
 
     if (type == QtFatalMsg)
         abort();

=== modified file 'libubuntudownloadmanager/logger.h'
--- libubuntudownloadmanager/logger.h	2013-07-25 21:15:08 +0000
+++ libubuntudownloadmanager/logger.h	2013-09-24 09:53:05 +0000
@@ -45,6 +45,16 @@
     static QString getLogDir();
 
  private:
+    void openLogFile(const QString& filename);
+
+    void openSyslogConnection();
+
+    void logSessionMessage(const QString &message);
+
+    void logSystemMessage(const QString &message);
+
+ private:
+    bool _isSystemBus = false;
     bool _initialized = false;
     QString _logFileName;
     QFile _logFile;

=== modified file 'ubuntu-download-manager-tests/irl_tests.py'
--- ubuntu-download-manager-tests/irl_tests.py	2013-09-17 09:37:52 +0000
+++ ubuntu-download-manager-tests/irl_tests.py	2013-09-24 09:53:05 +0000
@@ -58,7 +58,7 @@
 
 if __name__ == '__main__':
 
-    bus = dbus.SessionBus()
+    bus = dbus.SystemBus()
     loop = gobject.MainLoop()
     manager = bus.get_object('com.canonical.applications.Downloader',
             MANAGER_PATH)

=== added file 'ubuntu-download-manager/com.canonical.applications.Downloader.conf'
--- ubuntu-download-manager/com.canonical.applications.Downloader.conf	1970-01-01 00:00:00 +0000
+++ ubuntu-download-manager/com.canonical.applications.Downloader.conf	2013-09-24 09:53:05 +0000
@@ -0,0 +1,18 @@
+<?xml version="1.0" encoding="UTF-8"?>
+
+<!DOCTYPE busconfig PUBLIC
+ "-//freedesktop//DTD D-BUS Bus Configuration 1.0//EN"
+ "http://www.freedesktop.org/standards/dbus/1.0/busconfig.dtd">
+<busconfig>
+    <policy user="root">
+       <allow own="com.canonical.applications.Downloader"/>
+       <allow send_destination="com.canonical.applications.Downloader"/>
+    </policy>
+
+    <policy context="default">
+       <allow send_destination="com.canonical.applications.Downloader"/>
+       <allow send_destination="com.canonical.applications.Downloader"
+              send_interface="org.freedesktop.DBus.Introspectable"/>
+    </policy>
+
+</busconfig>

=== added file 'ubuntu-download-manager/com.canonical.applications.Downloader.service'
--- ubuntu-download-manager/com.canonical.applications.Downloader.service	1970-01-01 00:00:00 +0000
+++ ubuntu-download-manager/com.canonical.applications.Downloader.service	2013-09-24 09:53:05 +0000
@@ -0,0 +1,4 @@
+[D-BUS Service]
+Name=com.canonical.applications.Downloader
+User=root
+Exec=/usr/bin/ubuntu-download-manager

=== modified file 'ubuntu-download-manager/ubuntu-download-manager.pro'
--- ubuntu-download-manager/ubuntu-download-manager.pro	2013-08-22 20:49:57 +0000
+++ ubuntu-download-manager/ubuntu-download-manager.pro	2013-09-24 09:53:05 +0000
@@ -18,9 +18,17 @@
 
 SOURCES += main.cpp
 
-dbus_activation.path = /usr/share/dbus-1/services/
-dbus_activation.files = ubuntu-download-manager.service
-INSTALLS += dbus_activation
+dbus_session_activation.path = /usr/share/dbus-1/services/
+dbus_session_activation.files = ubuntu-download-manager.service
+INSTALLS += dbus_session_activation
+
+dbus_system_activation.path = /usr/share/dbus-1/system-services/
+dbus_system_activation.files = com.canonical.applications.Downloader.service
+INSTALLS += dbus_system_activation
+
+dbus_system_conf.path = /etc/dbus-1/system.d/
+dbus_system_conf.files = com.canonical.applications.Downloader.conf
+INSTALLS += dbus_system_conf
 
 target.path = /usr/bin/
 INSTALLS += target
@@ -31,4 +39,5 @@
 DEPENDPATH += $$PWD/../libubuntudownloadmanager
 
 OTHER_FILES += \
-    ubuntu-download-manager.service
+    ubuntu-download-manager.service \
+    com.canonical.applications.Downloader.conf