From 8c57830308a612b06b53f5fd31cfb765d8710d68 Mon Sep 17 00:00:00 2001 From: Tom Gundersen Date: Sun, 23 Mar 2014 15:33:24 +0100 Subject: [PATCH] sd-rtnl: message - don't reference associated rtnl object The object is not currently used, so just drop the refenence. If/when we end up using the object in the future, we must make sure to deal with possible mutual references between rtnl busses and their queued messages; as is done in sd-bus. --- src/libsystemd/sd-rtnl/rtnl-message.c | 9 +-- src/libsystemd/sd-rtnl/sd-rtnl.c | 91 +++++++-------------------- 2 files changed, 28 insertions(+), 72 deletions(-) diff --git a/src/libsystemd/sd-rtnl/rtnl-message.c b/src/libsystemd/sd-rtnl/rtnl-message.c index 84a8ffa59..690466e2f 100644 --- a/src/libsystemd/sd-rtnl/rtnl-message.c +++ b/src/libsystemd/sd-rtnl/rtnl-message.c @@ -46,6 +46,11 @@ int message_new(sd_rtnl *rtnl, sd_rtnl_message **ret, size_t initial_size) { assert_return(ret, -EINVAL); assert_return(initial_size >= sizeof(struct nlmsghdr), -EINVAL); + /* Note that 'rtnl' is curretly unused, if we start using it internally + we must take care to avoid problems due to mutual references between + busses and their queued messages. See sd-bus. + */ + m = new0(sd_rtnl_message, 1); if (!m) return -ENOMEM; @@ -61,9 +66,6 @@ int message_new(sd_rtnl *rtnl, sd_rtnl_message **ret, size_t initial_size) { m->hdr->nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK; m->sealed = false; - if (rtnl) - m->rtnl = sd_rtnl_ref(rtnl); - *ret = m; return 0; @@ -275,7 +277,6 @@ sd_rtnl_message *sd_rtnl_message_unref(sd_rtnl_message *m) { if (m && REFCNT_DEC(m->n_ref) <= 0) { unsigned i; - sd_rtnl_unref(m->rtnl); free(m->hdr); for (i = 0; i < m->n_containers; i++) diff --git a/src/libsystemd/sd-rtnl/sd-rtnl.c b/src/libsystemd/sd-rtnl/sd-rtnl.c index 695a2dacc..9f3a4f3de 100644 --- a/src/libsystemd/sd-rtnl/sd-rtnl.c +++ b/src/libsystemd/sd-rtnl/sd-rtnl.c @@ -104,6 +104,9 @@ int sd_rtnl_open(sd_rtnl **ret, uint32_t groups) { } sd_rtnl *sd_rtnl_ref(sd_rtnl *rtnl) { + assert_return(rtnl, NULL); + assert_return(!rtnl_pid_changed(rtnl), NULL); + if (rtnl) assert_se(REFCNT_INC(rtnl->n_ref) >= 2); @@ -111,87 +114,39 @@ sd_rtnl *sd_rtnl_ref(sd_rtnl *rtnl) { } sd_rtnl *sd_rtnl_unref(sd_rtnl *rtnl) { - unsigned long refs; - if (!rtnl) return NULL; - /* - * If our ref-cnt is exactly the number of internally queued messages - * plus the ref-cnt to be dropped, then we know there's no external - * reference to us. Hence, we look through all queued messages and if - * they also have no external references, we're about to drop the last - * ref. Flush the queues so the REFCNT_DEC() below will drop to 0. - * We must be careful not to introduce inter-message references or this - * logic will fall apart.. - */ - - refs = rtnl->rqueue_size + rtnl->wqueue_size + 1; + assert_return(!rtnl_pid_changed(rtnl), NULL); - if (REFCNT_GET(rtnl->n_ref) <= refs) { + if (REFCNT_DEC(rtnl->n_ref) <= 0) { struct match_callback *f; - bool q = true; unsigned i; - for (i = 0; i < rtnl->rqueue_size; i++) { - if (REFCNT_GET(rtnl->rqueue[i]->n_ref) > 1) { - q = false; - break; - } else if (rtnl->rqueue[i]->rtnl != rtnl) - --refs; - } - - if (q) { - for (i = 0; i < rtnl->wqueue_size; i++) { - if (REFCNT_GET(rtnl->wqueue[i]->n_ref) > 1) { - q = false; - break; - } else if (rtnl->wqueue[i]->rtnl != rtnl) - --refs; - } - } - - if (q && REFCNT_GET(rtnl->n_ref) == refs) { - /* Drop our own ref early to avoid recursion from: - * sd_rtnl_message_unref() - * sd_rtnl_unref() - * These must enter sd_rtnl_unref() with a ref-cnt - * smaller than us. */ - REFCNT_DEC(rtnl->n_ref); - - for (i = 0; i < rtnl->rqueue_size; i++) - sd_rtnl_message_unref(rtnl->rqueue[i]); - free(rtnl->rqueue); + for (i = 0; i < rtnl->rqueue_size; i++) + sd_rtnl_message_unref(rtnl->rqueue[i]); + free(rtnl->rqueue); - for (i = 0; i < rtnl->wqueue_size; i++) - sd_rtnl_message_unref(rtnl->wqueue[i]); - free(rtnl->wqueue); + for (i = 0; i < rtnl->wqueue_size; i++) + sd_rtnl_message_unref(rtnl->wqueue[i]); + free(rtnl->wqueue); - assert_se(REFCNT_GET(rtnl->n_ref) == 0); + hashmap_free_free(rtnl->reply_callbacks); + prioq_free(rtnl->reply_callbacks_prioq); - hashmap_free_free(rtnl->reply_callbacks); - prioq_free(rtnl->reply_callbacks_prioq); + sd_event_source_unref(rtnl->io_event_source); + sd_event_source_unref(rtnl->time_event_source); + sd_event_source_unref(rtnl->exit_event_source); + sd_event_unref(rtnl->event); - while ((f = rtnl->match_callbacks)) { - LIST_REMOVE(match_callbacks, rtnl->match_callbacks, f); - free(f); - } - - safe_close(rtnl->fd); - - sd_event_source_unref(rtnl->io_event_source); - sd_event_source_unref(rtnl->time_event_source); - sd_event_source_unref(rtnl->exit_event_source); - sd_event_unref(rtnl->event); - - free(rtnl); - - return NULL; + while ((f = rtnl->match_callbacks)) { + LIST_REMOVE(match_callbacks, rtnl->match_callbacks, f); + free(f); } - } - assert_se(REFCNT_GET(rtnl->n_ref) > 0); - REFCNT_DEC(rtnl->n_ref); + safe_close(rtnl->fd); + free(rtnl); + } return NULL; } -- 2.30.2