Bug 34316

Summary: UpdateParameters: bad handling of DBus_Property and Has_Default in Unset argument
Product: Telepathy Reporter: Danielle Madeley <danielle>
Component: mission-controlAssignee: Will Thompson <will>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: normal    
Priority: medium CC: robert.oswald
Version: git masterKeywords: patch
Hardware: Other   
OS: All   
URL: http://git.collabora.co.uk/?p=user/wjt/telepathy-spec-wjt.git;a=shortlog;h=refs/heads/unset-DBus_Property-parameter
Whiteboard:
i915 platform: i915 features:

Description Danielle Madeley 2011-02-15 17:52:08 UTC
Originally from discussion on https://bugzilla.gnome.org/show_bug.cgi?id=642358

If you update a DBus_Property parameter:

method call sender=:1.187 -> dest=org.freedesktop.Telepathy.AccountManager serial=97 path=/org/freedesktop/Telepathy/Account/badger/badger/badger_2dtest_2d20; interface=org.freedesktop.Telepathy.Account; member=UpdateParameters
   array [
      dict entry(
         string "ofdT.Connection.Interface.Badger.ShowBadger"
         variant             boolean true
      )
   ]
   array [
   ]

method return sender=:1.183 -> dest=:1.187 reply_serial=97
   array [
   ]

MC calls Set() on the Connection correctly.

~

If you unset a parameter:

method call sender=:1.176 -> dest=org.freedesktop.Telepathy.AccountManager serial=110 path=/org/freedesktop/Telepathy/Account/badger/badger/badger_2dtest_2d20; interface=org.freedesktop.Telepathy.Account; member=UpdateParameters
   array [
   ]
   array [
      string "ofdT.Connection.Interface.Badger.ShowBadger"
   ]

method return sender=:1.183 -> dest=:1.176 reply_serial=110
   array [
      string "ofdT.Connection.Interface.Badger.ShowBadger" 
   ]

MC does NOT call Set() on the Connection.

~

In the latter case, I believe MC should be calling Set() with the default value, as specified by the CM instead of requiring a reconnect.
Comment 1 Will Thompson 2011-02-16 02:05:49 UTC
So this would add an implicit requirement that DBus_Property ⇒ Has_Default?
Comment 2 Danielle Madeley 2011-02-16 02:20:29 UTC
(In reply to comment #1)
> So this would add an implicit requirement that DBus_Property ⇒ Has_Default?

I don't think that has to be true.

Perhaps it only calls Set() if Has_Default. Otherwise it adds the property to Reconnect_Required.
Comment 3 Simon McVittie 2011-02-16 02:48:55 UTC
Already noted in the source:

        /* pessimistically assume that removing any parameter merits
         * reconnection (in a perfect implementation, if the
         * Has_Default flag was set we'd check whether the current
         * value is the default already) */

I believe this applies equally to parameters that are not a DBusProperty: if you have an account with { 'ignore-ssl-errors': False } in Parameters, the default is also False, and you unset 'ignore-ssl-errors', then we shouldn't ask for reconnnection.
Comment 4 Will Thompson 2011-02-16 02:54:13 UTC
Taking this to clarify the spec and fix the implementation.
Comment 5 Will Thompson 2011-02-18 09:33:10 UTC
Here's a (remarkably large) branch which cleans up the UpdateParameters() implementation and fixes this bug (as well as a couple of related bugs and a leak I spotted along the way).

% git diff --stat master src
 src/mcd-account-manager.c |    6 -
 src/mcd-account.c         |  358 ++++++++++++++++++++-------------------------
 src/mcd-account.h         |    7 +-
 3 files changed, 164 insertions(+), 207 deletions(-)
% git diff --stat master tests
 tests/twisted/account-manager/update-parameters.py |   97 +++++++++++++++++---
 tests/twisted/telepathy/managers/fakecm.manager    |    4 +-
 2 files changed, 87 insertions(+), 14 deletions(-)

I'll fix up the spec on Monday.
Comment 6 Danielle Madeley 2011-02-20 17:40:41 UTC
I like this. One niggle to prove I really read it:

+    if (!check_parameters (account, protocol, params, unset, dbus_properties,
+                           not_yet, &error))
+    {
+        g_hash_table_unref (dbus_properties);
+        g_ptr_array_unref (not_yet);
+        goto error;

+    g_hash_table_unref (dbus_properties);
+    g_ptr_array_unref (not_yet);
     tp_connection_manager_protocol_free (protocol);
     return;

It might be nice to have goto finally; and then error: then finally: so that all the memory is released in one location -- since my first question was whether or not the error path free'd @protocol.
Comment 7 Will Thompson 2011-02-21 09:53:18 UTC
(In reply to comment #6)
> I like this. One niggle to prove I really read it:
> 
> +    if (!check_parameters (account, protocol, params, unset, dbus_properties,
> +                           not_yet, &error))
> +    {
> +        g_hash_table_unref (dbus_properties);
> +        g_ptr_array_unref (not_yet);
> +        goto error;
> 
> +    g_hash_table_unref (dbus_properties);
> +    g_ptr_array_unref (not_yet);
>      tp_connection_manager_protocol_free (protocol);
>      return;
> 
> It might be nice to have goto finally; and then error: then finally: so that
> all the memory is released in one location -- since my first question was
> whether or not the error path free'd @protocol.

Quite right. Fixed slightly differently, at Nicolas’ suggestion: <http://git.collabora.co.uk/?p=telepathy-mission-control.git;a=commitdiff;h=718d297>

… and merged to master as <http://git.collabora.co.uk/?p=telepathy-mission-control.git;a=commit;h=a4ca0e6>. Sadly I introduced a regression. There's a fix at <http://git.collabora.co.uk/?p=user/wjt/telepathy-mission-control-wjt.git;a=commitdiff;h=a137d57>.
Comment 9 Will Thompson 2011-02-21 11:12:04 UTC
Nicolas reviewed the regression fix in MC. Just the spec clarification to go.
Comment 10 Danielle Madeley 2011-02-21 13:53:44 UTC
(In reply to comment #8)
> Here's a spec branch:
> <http://git.collabora.co.uk/?p=user/wjt/telepathy-spec-wjt.git;a=shortlog;h=refs/heads/unset-DBus_Property-parameter>

Review++ :)
Comment 11 Will Thompson 2011-02-22 08:35:18 UTC
Done!

The spec fix will be in 0.21.10; the MC fix will be in 5.7.4.
Comment 12 wope 2011-06-18 07:13:41 UTC
*** Bug 35743 has been marked as a duplicate of this bug. ***

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.