Re: [systemd-devel] FW: [PATCH] names: clear e->activator when activator has disconnected
> On 02/24/2014 12:47 PM, Daniel Mack wrote: > Yes, but there's one more pointer that can dangle :) > > As we're not on a fast path here, I think we can really walk the hash > of > all names in the registry of the bus, and clean up the ->activator > pointer. Which should also fix the bug you mentioned above. > > What about this one instead? > > > Thanks, > Daniel This does the job well also. Best Regards, Radoslaw Pajak ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] FW: [PATCH] names: clear e->activator when activator has disconnected
> On 02/24/2014 10:17 AM, Daniel Mack wrote: > > If e->activator wasn't released when activator connection has > disconnected, > > there was no possibility to open new activator connection for that > name > > in the case when there was another owner of that name. > > Interesting. Like in the last patch you sent, you seem to have > situations where the activator comes and goes just like a normal > connection. Which should be handled fine by kdbus, it's just different > from our use case. In our project it is possible that config files for activator connections may change so activator connections may disappear or appear again. > > @@ -255,6 +255,8 @@ void kdbus_name_remove_by_conn(struct > > kdbus_name_registry *reg, > > kdbus_name_queue_item_free(q); > > list_for_each_entry_safe(e, e_tmp, &names_list, conn_entry) > > kdbus_name_entry_release(e, ¬ify_list); > > + if (conn->flags & KDBUS_HELLO_ACTIVATOR) > > + conn->activator_of->activator = kdbus_conn_unref(conn); > > mutex_unlock(®->entries_lock); > > mutex_unlock(&conn->bus->lock); > > Hmm, we have the information if the connection that is disconnecting is > the activator of a name entry. So why don't just use that as condition > instead of introducing yet another level of indirection? > > IOW, does the following patch work for you as well? I might overlook > something, and I didn't build any test case yet, however. > > Daniel Your patch works if the activator connection is still the owner of the name, but it doesn't work if activator has lost the ownership. If regular connection takes over the name than kdbus_name_replace_owner calls kdbus_name_entry_remove_owner which removes e->conn_entry from the names list of the activator connection, which you wanted to use. If activator connection has closed than, there is stil e->activator pointer to this non existing connection which is bug by itself and new activator connection cannot acquire that name. Without our patch there is no link between the activator connection and name_entry. Best Regards, Radoslaw Pajak ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] FW: [PATCH] names: clear e->activator when activator has disconnected
If e->activator wasn't released when activator connection has disconnected,
there was no possibility to open new activator connection for that name
in the case when there was another owner of that name.
Signed-off-by: Radoslaw Pajak
---
connection.h |1 +
names.c |7 ++-
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/connection.h b/connection.h
index 858be94..34540c7 100644
--- a/connection.h
+++ b/connection.h
@@ -68,6 +68,7 @@ struct kdbus_conn {
struct list_head monitor_entry;
struct list_head names_list;
struct list_head names_queue_list;
+ struct kdbus_name_entry *activator_of;
struct list_head reply_list;
atomic_t reply_count;
size_t names;
diff --git a/names.c b/names.c
index 22ead9f..762871b 100644
--- a/names.c
+++ b/names.c
@@ -255,6 +255,8 @@ void kdbus_name_remove_by_conn(struct
kdbus_name_registry *reg,
kdbus_name_queue_item_free(q);
list_for_each_entry_safe(e, e_tmp, &names_list, conn_entry)
kdbus_name_entry_release(e, ¬ify_list);
+ if (conn->flags & KDBUS_HELLO_ACTIVATOR)
+ conn->activator_of->activator = kdbus_conn_unref(conn);
mutex_unlock(®->entries_lock);
mutex_unlock(&conn->bus->lock);
@@ -418,6 +420,7 @@ int kdbus_name_acquire(struct kdbus_name_registry *reg,
if (conn->flags & KDBUS_HELLO_ACTIVATOR &&
e->activator == NULL) {
e->activator = kdbus_conn_ref(conn);
+ conn->activator_of = e;
goto exit_unlock;
}
@@ -486,8 +489,10 @@ int kdbus_name_acquire(struct kdbus_name_registry *reg,
goto exit_unlock;
}
- if (conn->flags & KDBUS_HELLO_ACTIVATOR)
+ if (conn->flags & KDBUS_HELLO_ACTIVATOR) {
e->activator = kdbus_conn_ref(conn);
+ conn->activator_of = e;
+ }
e->flags = *flags;
INIT_LIST_HEAD(&e->queue_list);
--
1.7.9.5
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH] connection: update conn->flags before kdbus_notify_id_change uses them
Signed-off-by: Radoslaw Pajak
---
connection.c |6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/connection.c b/connection.c
index 95f75e1..9982da2 100644
--- a/connection.c
+++ b/connection.c
@@ -1884,6 +1884,9 @@ int kdbus_conn_new(struct kdbus_ep *ep,
BUILD_BUG_ON(sizeof(bus->id128) != sizeof(hello->id128));
memcpy(hello->id128, bus->id128, sizeof(hello->id128));
+ conn->flags = hello->conn_flags;
+ conn->attach_flags = hello->attach_flags;
+
/* notify about the new active connection */
ret = kdbus_notify_id_change(KDBUS_ITEM_ID_ADD, conn->id,
conn->flags,
¬ify_list);
@@ -1891,9 +1894,6 @@ int kdbus_conn_new(struct kdbus_ep *ep,
goto exit_unref_ep;
kdbus_conn_kmsg_list_send(conn->ep, ¬ify_list);
- conn->flags = hello->conn_flags;
- conn->attach_flags = hello->attach_flags;
-
if (activator_name) {
u64 flags = KDBUS_NAME_ACTIVATOR;
--
1.7.9.5
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH] connection: update conn->flags before kdbus_notify_id_change uses them
Signed-off-by: Radoslaw Pajak
---
connection.c |6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/connection.c b/connection.c
index 95f75e1..9982da2 100644
--- a/connection.c
+++ b/connection.c
@@ -1884,6 +1884,9 @@ int kdbus_conn_new(struct kdbus_ep *ep,
BUILD_BUG_ON(sizeof(bus->id128) != sizeof(hello->id128));
memcpy(hello->id128, bus->id128, sizeof(hello->id128));
+ conn->flags = hello->conn_flags;
+ conn->attach_flags = hello->attach_flags;
+
/* notify about the new active connection */
ret = kdbus_notify_id_change(KDBUS_ITEM_ID_ADD, conn->id, conn->flags,
¬ify_list);
@@ -1891,9 +1894,6 @@ int kdbus_conn_new(struct kdbus_ep *ep,
goto exit_unref_ep;
kdbus_conn_kmsg_list_send(conn->ep, ¬ify_list);
- conn->flags = hello->conn_flags;
- conn->attach_flags = hello->attach_flags;
-
if (activator_name) {
u64 flags = KDBUS_NAME_ACTIVATOR;
--
1.7.9.5
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH] match: kdbus_match_entry_free fixed
Signed-off-by: Radoslaw Pajak --- match.c |3 +++ 1 file changed, 3 insertions(+) diff --git a/match.c b/match.c index d619bef..4c51327 100644 --- a/match.c +++ b/match.c @@ -121,6 +121,9 @@ static void kdbus_match_entry_free(struct kdbus_match_entry *entry) list_for_each_entry_safe(r, tmp, &entry->rules_list, rules_entry) kdbus_match_rule_free(r); + + list_del(&entry->list_entry); + kfree(entry); } /** -- 1.7.9.5 ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH] match: kdbus_match_entry_free fixed
Signed-off-by: Radoslaw Pajak --- match.c |3 +++ 1 file changed, 3 insertions(+) diff --git a/match.c b/match.c index d619bef..4c51327 100644 --- a/match.c +++ b/match.c @@ -121,6 +121,9 @@ static void kdbus_match_entry_free(struct kdbus_match_entry *entry) list_for_each_entry_safe(r, tmp, &entry->rules_list, rules_entry) kdbus_match_rule_free(r); + + list_del(&entry->list_entry); + kfree(entry); } /** -- 1.7.9.5 ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
