bus: Separate RemoveMatch into prepare and commit stages

This means we don't send a spurious successful reply if a caller removes
a match rule that they never added.

Signed-off-by: Simon McVittie <smcv@collabora.com>

3 files changed, 60 insertions, 29 deletions

diff --git a/bus/driver.c b/bus/driver.c
index 3898cfd6..6f5451a1 100644
--- a/bus/driver.c
+++ b/bus/driver.c
@@ -1409,6 +1409,7 @@ bus_driver_handle_remove_match (DBusConnection *connection,
   const char *text;
   DBusString str;
   BusMatchmaker *matchmaker;
+  DBusList *link;
 
   _DBUS_ASSERT_ERROR_IS_CLEAR (error);
 
@@ -1429,17 +1430,21 @@ bus_driver_handle_remove_match (DBusConnection *connection,
   if (rule == NULL)
     goto failed;
 
-  /* Send the ack before we remove the rule, since the ack is undone
-   * on transaction cancel, but rule removal isn't.
-   */
-  if (!bus_driver_send_ack_reply (connection, transaction, message, error))
-    goto failed;
-
   matchmaker = bus_connection_get_matchmaker (connection);
 
-  if (!bus_matchmaker_remove_rule_by_value (matchmaker, rule, error))
+  /* Check whether the rule exists and prepare to remove it, but don't
+   * actually do it yet. */
+  link = bus_matchmaker_prepare_remove_rule_by_value (matchmaker, rule, error);
+  if (link == NULL)
+    goto failed;
+
+  /* We do this before actually removing the rule, because removing the
+   * rule cannot be undone if we run out of memory here. */
+  if (!bus_driver_send_ack_reply (connection, transaction, message, error))
     goto failed;
 
+  /* The rule exists, so now we can do things we can't undo. */
+  bus_matchmaker_commit_remove_rule_by_value (matchmaker, rule, link);
   bus_match_rule_unref (rule);
 
   return TRUE;
diff --git a/bus/signals.c b/bus/signals.c
index 08304c5b..2af5039b 100644
--- a/bus/signals.c
+++ b/bus/signals.c
@@ -1608,21 +1608,32 @@ bus_matchmaker_remove_rule (BusMatchmaker   *matchmaker,
   bus_match_rule_unref (rule);
 }
 
-/* Remove a single rule which is equal to the given rule by value */
-dbus_bool_t
-bus_matchmaker_remove_rule_by_value (BusMatchmaker   *matchmaker,
-                                     BusMatchRule    *value,
-                                     DBusError       *error)
+/*
+ * Prepare to remove the the most-recently-added rule which is equal to
+ * the given rule by value, but do not actually do it yet.
+ *
+ * Return a linked-list link which must be treated as opaque by the caller:
+ * the only valid thing to do with it is to pass it to
+ * bus_matchmaker_commit_remove_rule_by_value().
+ *
+ * The returned linked-list link becomes invalid when control returns to
+ * the main loop. If the caller decides not to remove the rule after all,
+ * there is currently no need to cancel explicitly.
+ */
+DBusList *
+bus_matchmaker_prepare_remove_rule_by_value (BusMatchmaker   *matchmaker,
+                                             BusMatchRule    *value,
+                                             DBusError       *error)
 {
   DBusList **rules;
   DBusList *link = NULL;
 
-  _dbus_verbose ("Removing rule by value with message_type %d, interface %s\n",
+  _dbus_verbose ("Finding rule by value with message_type %d, interface %s\n",
                  value->message_type,
                  value->interface != NULL ? value->interface : "<null>");
 
   rules = bus_matchmaker_get_rules (matchmaker, value->message_type,
-      value->interface, FALSE);
+                                    value->interface, FALSE);
 
   if (rules != NULL)
     {
@@ -1639,26 +1650,37 @@ bus_matchmaker_remove_rule_by_value (BusMatchmaker   *matchmaker,
           prev = _dbus_list_get_prev_link (rules, link);
 
           if (match_rule_equal (rule, value))
-            {
-              bus_matchmaker_remove_rule_link (rules, link);
-              break;
-            }
+            return link;
 
           link = prev;
         }
     }
 
-  if (link == NULL)
-    {
-      dbus_set_error (error, DBUS_ERROR_MATCH_RULE_NOT_FOUND,
-                      "The given match rule wasn't found and can't be removed");
-      return FALSE;
-    }
+  dbus_set_error (error, DBUS_ERROR_MATCH_RULE_NOT_FOUND,
+                  "The given match rule wasn't found and can't be removed");
+  return NULL;
+}
 
+/*
+ * Commit a previous call to bus_matchmaker_prepare_remove_rule_by_value(),
+ * which must have been done during the same main-loop iteration.
+ */
+void
+bus_matchmaker_commit_remove_rule_by_value (BusMatchmaker *matchmaker,
+                                            BusMatchRule  *value,
+                                            DBusList      *link)
+{
+  DBusList **rules;
+
+  _dbus_assert (match_rule_equal (link->data, value));
+  rules = bus_matchmaker_get_rules (matchmaker, value->message_type,
+                                    value->interface, FALSE);
+  /* Should only be called if a rule matching value was successfully
+   * added, which means rules must contain at least link */
+  _dbus_assert (rules != NULL);
+  bus_matchmaker_remove_rule_link (rules, link);
   bus_matchmaker_gc_rules (matchmaker, value->message_type, value->interface,
       rules);
-
-  return TRUE;
 }
 
 static void
diff --git a/bus/signals.h b/bus/signals.h
index 0edfb07e..027e921c 100644
--- a/bus/signals.h
+++ b/bus/signals.h
@@ -91,9 +91,6 @@ void           bus_matchmaker_unref (BusMatchmaker *matchmaker);
 
 dbus_bool_t bus_matchmaker_add_rule             (BusMatchmaker   *matchmaker,
                                                  BusMatchRule    *rule);
-dbus_bool_t bus_matchmaker_remove_rule_by_value (BusMatchmaker   *matchmaker,
-                                                 BusMatchRule    *value,
-                                                 DBusError       *error);
 void        bus_matchmaker_remove_rule          (BusMatchmaker   *matchmaker,
                                                  BusMatchRule    *rule);
 void        bus_matchmaker_disconnected         (BusMatchmaker   *matchmaker,
@@ -105,4 +102,11 @@ dbus_bool_t bus_matchmaker_get_recipients       (BusMatchmaker   *matchmaker,
                                                  DBusMessage     *message,
                                                  DBusList       **recipients_p);
 
+DBusList *bus_matchmaker_prepare_remove_rule_by_value (BusMatchmaker   *matchmaker,
+                                                       BusMatchRule    *value,
+                                                       DBusError       *error);
+void      bus_matchmaker_commit_remove_rule_by_value  (BusMatchmaker   *matchmaker,
+                                                       BusMatchRule    *value,
+                                                       DBusList        *link);
+
 #endif /* BUS_SIGNALS_H */