Merge lp:~ted/libertine/qtdbus-connection into lp:libertine/trunk

[Ted Gould]

Ensure we don't leak references to the DBus session bus.

[Christopher Townsend (townsend)]

This was branched from lp:libertine instead of lp:libertine/trunk and we needed a fixed branch quickly, so I made https://code.launchpad.net/~townsend/libertine/qtdbus-connection/+merge/317244 to remedy that.

No need for this MP anymore.

review: Disapprove

Unmerged revisions

=== modified file 'liblibertine/libertined.cpp'
--- liblibertine/libertined.cpp	2017-01-24 16:03:25 +0000
+++ liblibertine/libertined.cpp	2017-02-14 17:15:01 +0000
@@ -30,6 +30,22 @@
 static const auto SERVICE_INTERFACE = "com.canonical.libertine.Service";
 static const auto PROGRESS_INTERFACE = "com.canonical.libertine.Service.Progress";
 
+/* Class to handle making sure the session bus isn't a singleton
+   which is how QtDBus does it by default. This is a bad thing for
+   a library to do, so we'll make sure we aren't causing global leaks */
+class SessionBus {
+public:
+  QDBusConnection bus;
+  SessionBus() :
+    /* NOTE: Must get the envvar each time or Qt caches that too */
+    bus{QDBusConnection::connectToBus(getenv("DBUS_SESSION_BUS_ADDRESS"), "libertined")}
+  {
+  }
+  ~SessionBus() {
+    QDBusConnection::disconnectFromBus("libertined");
+  }
+};
+
 static QVariantList
 dbusCall(QDBusConnection const& bus, QString const& iface, QString const& path,
          QString const& method, QVariantList const& args = QVariantList())
@@ -119,65 +135,65 @@
 QString
 container_info(char const* container_id, QString const& key)
 {
-  auto bus = QDBusConnection::sessionBus();
-  auto path = call(bus, "container_info", QVariantList{QVariant(container_id)});
+  SessionBus session;
+  auto path = call(session.bus, "container_info", QVariantList{QVariant(container_id)});
 
-  if (!waitForFinish(bus, path))
+  if (!waitForFinish(session.bus, path))
   {
     return QString();
   }
 
-  auto error = lastError(bus, path);
+  auto error = lastError(session.bus, path);
   if (!error.isEmpty())
   {
     qWarning() << "error:" << error;
     return QString();
   }
 
-  return QJsonDocument::fromJson(result(bus, path).toLatin1()).object().value(key).toString();
+  return QJsonDocument::fromJson(result(session.bus, path).toLatin1()).object().value(key).toString();
 }
 }
 
 QJsonArray
 libertined_list()
 {
-  auto bus = QDBusConnection::sessionBus();
-  auto path = call(bus, "list", QVariantList());
+  SessionBus session;
+  auto path = call(session.bus, "list", QVariantList());
 
-  if (!waitForFinish(bus, path))
+  if (!waitForFinish(session.bus, path))
   {
     return QJsonArray();
   }
 
-  auto error = lastError(bus, path);
+  auto error = lastError(session.bus, path);
   if (!error.isEmpty())
   {
     qWarning() << "error:" << error;
     return QJsonArray();
   }
 
-  return QJsonDocument::fromJson(result(bus, path).toLatin1()).array();
+  return QJsonDocument::fromJson(result(session.bus, path).toLatin1()).array();
 }
 
 QJsonArray
 libertined_list_app_ids(char const* container_id)
 {
-  auto bus = QDBusConnection::sessionBus();
-  auto path = call(bus, "list_app_ids", QVariantList{QVariant(container_id)});
+  SessionBus session;
+  auto path = call(session.bus, "list_app_ids", QVariantList{QVariant(container_id)});
 
-  if (!waitForFinish(bus, path))
+  if (!waitForFinish(session.bus, path))
   {
     return QJsonArray();
   }
 
-  auto error = lastError(bus, path);
+  auto error = lastError(session.bus, path);
   if (!error.isEmpty())
   {
     qWarning() << "error:" << error;
     return QJsonArray();
   }
 
-  return QJsonDocument::fromJson(result(bus, path).toLatin1()).array();
+  return QJsonDocument::fromJson(result(session.bus, path).toLatin1()).array();
 }
 
 QString

=== modified file 'python/libertine/ContainersConfig.py'
--- python/libertine/ContainersConfig.py	2017-01-25 18:48:36 +0000
+++ python/libertine/ContainersConfig.py	2017-02-14 17:15:01 +0000
@@ -102,21 +102,20 @@
         if not container:
             return
 
-        if (type(value) is str or
-            type(value) is bool):
-            container[key] = value
-        elif type(value) is dict:
-            if key not in container:
-                container[key] = [value]
-            else:
-                container[key].append(value)
-        elif type(value) is list:
-            if key not in container:
-                container[key] = value
-            else:
-                container[key] = container[key] + value
+        if type(value) is dict:
+            if key not in container:
+                newvalue = [value]
+            else:
+                newvalue = container[key].copy()
+                newvalue.append(value)
+        elif type(value) is list and key in container:
+            newvalue = container[key] + value
+        else:
+            newvalue = value
 
-        write_container_config_file(self.container_list)
+        if container.get(key, None) != newvalue:
+            container[key] = newvalue
+            write_container_config_file(self.container_list)
 
     def _set_array_object_value_by_key(self, container_id, array_key, object_key, matcher, key, value):
         container = self._get_container_entry(container_id)

=== modified file 'tools/libertine-container-manager'
--- tools/libertine-container-manager	2017-02-07 12:35:48 +0000
+++ tools/libertine-container-manager	2017-02-14 17:15:01 +0000
@@ -345,11 +345,7 @@
                 libertine.utils.get_logger().error("Configuring freeze is only valid on LXC and LXD container types.")
                 sys.exit(1)
 
-            current_freeze = self.containers_config.get_freeze_on_stop(container_id)
-            config_freeze = args.freeze == 'enable'
-
-            if current_freeze != config_freeze:
-               self.containers_config.update_freeze_on_stop(container_id, config_freeze)
+            self.containers_config.update_freeze_on_stop(container_id, args.freeze == 'enable')
 
         else:
             libertine.utils.get_logger().error("Configure called with no subcommand. See configure --help for usage.")