sd-ipv4ll: rework callbacks

Firstly, no longer distinguish between STOP and INIT states.

Secondly, do not trigger STOP events when calls to sd_ipv4ll_*() fail. The
caller is the one who would receive the event and will already know that the
call to sd_ipv4ll_*() has failed, so it is redundant.

STOP events will now only be triggered by calling sd_ipv4ll_stop() explicitly
or by some internal error in the library triggered by receiving a packet or
an expiring timeout (i.e., any error that would otherwise not be reported
back to the consumer of the library).

Lastly, follow CODING_STYLE and always return NULL on unref. Protect from
objects being destroyed in callbacks accordingly.

diff --git a/src/libsystemd-network/sd-ipv4ll.c b/src/libsystemd-network/sd-ipv4ll.c
index 2345e1aabbb1ba6035c7e1f9cd62748cd28e124e..05ce5c073a160b415132ef186ffc31eca6b3e717 100644 (file)
--- a/src/libsystemd-network/sd-ipv4ll.c
+++ b/src/libsystemd-network/sd-ipv4ll.c
@@ -50,6 +50,9 @@
 
 #define log_ipv4ll(ll, fmt, ...) log_internal(LOG_DEBUG, 0, __FILE__, __LINE__, __func__, "IPv4LL: " fmt, ##__VA_ARGS__)
 
+#define IPV4LL_DONT_DESTROY(ll) \
+        _cleanup_ipv4ll_unref_ _unused_ sd_ipv4ll *_dont_destroy_##ll = sd_ipv4ll_ref(ll)
+
 typedef enum IPv4LLState {
         IPV4LL_STATE_INIT,
         IPV4LL_STATE_WAITING_PROBE,
@@ -57,7 +60,6 @@ typedef enum IPv4LLState {
         IPV4LL_STATE_WAITING_ANNOUNCE,
         IPV4LL_STATE_ANNOUNCING,
         IPV4LL_STATE_RUNNING,
-        IPV4LL_STATE_STOPPED,
         _IPV4LL_STATE_MAX,
         _IPV4LL_STATE_INVALID = -1
 } IPv4LLState;
@@ -85,6 +87,63 @@ struct sd_ipv4ll {
         void* userdata;
 };
 
+sd_ipv4ll *sd_ipv4ll_ref(sd_ipv4ll *ll) {
+        if (!ll)
+                return NULL;
+
+        assert(ll->n_ref >= 1);
+        ll->n_ref++;
+
+        return ll;
+}
+
+sd_ipv4ll *sd_ipv4ll_unref(sd_ipv4ll *ll) {
+        if (!ll)
+                return NULL;
+
+        assert(ll->n_ref >= 1);
+        ll->n_ref--;
+
+        if (ll->n_ref > 0)
+                return NULL;
+
+        ll->receive_message = sd_event_source_unref(ll->receive_message);
+        ll->fd = safe_close(ll->fd);
+
+        ll->timer = sd_event_source_unref(ll->timer);
+
+        sd_ipv4ll_detach_event(ll);
+
+        free(ll->random_data);
+        free(ll->random_data_state);
+        free(ll);
+
+        return NULL;
+}
+
+DEFINE_TRIVIAL_CLEANUP_FUNC(sd_ipv4ll*, sd_ipv4ll_unref);
+#define _cleanup_ipv4ll_unref_ _cleanup_(sd_ipv4ll_unrefp)
+
+int sd_ipv4ll_new(sd_ipv4ll **ret) {
+        _cleanup_ipv4ll_unref_ sd_ipv4ll *ll = NULL;
+
+        assert_return(ret, -EINVAL);
+
+        ll = new0(sd_ipv4ll, 1);
+        if (!ll)
+                return -ENOMEM;
+
+        ll->n_ref = 1;
+        ll->state = IPV4LL_STATE_INIT;
+        ll->index = -1;
+        ll->fd = -1;
+
+        *ret = ll;
+        ll = NULL;
+
+        return 0;
+}
+
 static void ipv4ll_set_state(sd_ipv4ll *ll, IPv4LLState st, bool reset_counter) {
 
         assert(ll);
@@ -98,19 +157,16 @@ static void ipv4ll_set_state(sd_ipv4ll *ll, IPv4LLState st, bool reset_counter)
         }
 }
 
-static sd_ipv4ll *ipv4ll_client_notify(sd_ipv4ll *ll, int event) {
+static void ipv4ll_client_notify(sd_ipv4ll *ll, int event) {
         assert(ll);
 
-        if (ll->cb) {
-                ll = sd_ipv4ll_ref(ll);
+        if (ll->cb)
                 ll->cb(ll, event, ll->userdata);
-                ll = sd_ipv4ll_unref(ll);
-        }
-
-        return ll;
 }
 
-static sd_ipv4ll *ipv4ll_stop(sd_ipv4ll *ll, int event) {
+static void ipv4ll_stop(sd_ipv4ll *ll) {
+        IPV4LL_DONT_DESTROY(ll);
+
         assert(ll);
 
         ll->receive_message = sd_event_source_unref(ll->receive_message);
@@ -120,14 +176,20 @@ static sd_ipv4ll *ipv4ll_stop(sd_ipv4ll *ll, int event) {
 
         log_ipv4ll(ll, "STOPPED");
 
-        ll = ipv4ll_client_notify(ll, event);
+        ll->claimed_address = 0;
+        ipv4ll_set_state (ll, IPV4LL_STATE_INIT, true);
+}
 
-        if (ll) {
-                ll->claimed_address = 0;
-                ipv4ll_set_state (ll, IPV4LL_STATE_INIT, true);
-        }
+int sd_ipv4ll_stop(sd_ipv4ll *ll) {
+        IPV4LL_DONT_DESTROY(ll);
 
-        return ll;
+        assert_return(ll, -EINVAL);
+
+        ipv4ll_stop(ll);
+
+        ipv4ll_client_notify(ll, IPV4LL_EVENT_STOP);
+
+        return 0;
 }
 
 static int ipv4ll_pick_address(sd_ipv4ll *ll, be32_t *address) {
@@ -268,10 +330,7 @@ static int ipv4ll_on_timeout(sd_event_source *s, uint64_t usec, void *userdata)
                 if (ll->iteration == 0) {
                         log_ipv4ll(ll, "ANNOUNCE");
                         ll->claimed_address = ll->address;
-                        ll = ipv4ll_client_notify(ll, IPV4LL_EVENT_BIND);
-                        if (!ll || ll->state == IPV4LL_STATE_STOPPED)
-                                goto out;
-
+                        ipv4ll_client_notify(ll, IPV4LL_EVENT_BIND);
                         ll->conflict = 0;
                 }
 
@@ -282,21 +341,20 @@ static int ipv4ll_on_timeout(sd_event_source *s, uint64_t usec, void *userdata)
 
 out:
         if (r < 0 && ll)
-                ipv4ll_stop(ll, r);
+                sd_ipv4ll_stop(ll);
 
         return 1;
 }
 
 static int ipv4ll_on_conflict(sd_ipv4ll *ll) {
+        IPV4LL_DONT_DESTROY(ll);
         int r;
 
         assert(ll);
 
         log_ipv4ll(ll, "CONFLICT");
 
-        ll = ipv4ll_client_notify(ll, IPV4LL_EVENT_CONFLICT);
-        if (!ll || ll->state == IPV4LL_STATE_STOPPED)
-                return 0;
+        ipv4ll_client_notify(ll, IPV4LL_EVENT_CONFLICT);
 
         ll->claimed_address = 0;
 
@@ -321,7 +379,7 @@ static int ipv4ll_on_conflict(sd_ipv4ll *ll) {
                 log_ipv4ll(ll, "MAX_CONFLICTS");
                 ipv4ll_set_next_wakeup(ll, RATE_LIMIT_INTERVAL, PROBE_WAIT);
         } else
-              ipv4ll_set_next_wakeup(ll, 0, PROBE_WAIT);
+                ipv4ll_set_next_wakeup(ll, 0, PROBE_WAIT);
 
         return 0;
 }
@@ -379,7 +437,7 @@ static int ipv4ll_on_packet(sd_event_source *s, int fd,
 
 out:
         if (r < 0 && ll)
-                ipv4ll_stop(ll, r);
+                sd_ipv4ll_stop(ll);
 
         return 1;
 }
@@ -387,8 +445,7 @@ out:
 int sd_ipv4ll_set_index(sd_ipv4ll *ll, int interface_index) {
         assert_return(ll, -EINVAL);
         assert_return(interface_index > 0, -EINVAL);
-        assert_return(IN_SET(ll->state, IPV4LL_STATE_INIT,
-                             IPV4LL_STATE_STOPPED), -EBUSY);
+        assert_return(ll->state == IPV4LL_STATE_INIT, -EBUSY);
 
         ll->index = interface_index;
 
@@ -398,7 +455,7 @@ int sd_ipv4ll_set_index(sd_ipv4ll *ll, int interface_index) {
 int sd_ipv4ll_set_mac(sd_ipv4ll *ll, const struct ether_addr *addr) {
         assert_return(ll, -EINVAL);
         assert_return(addr, -EINVAL);
-        assert_return(IN_SET(ll->state, IPV4LL_STATE_INIT, IPV4LL_STATE_STOPPED), -EBUSY);
+        assert_return(ll->state == IPV4LL_STATE_INIT, -EBUSY);
 
         if (memcmp(&ll->mac_addr, addr, ETH_ALEN) == 0)
                 return 0;
@@ -426,10 +483,8 @@ int sd_ipv4ll_attach_event(sd_ipv4ll *ll, sd_event *event, int priority) {
                 ll->event = sd_event_ref(event);
         else {
                 r = sd_event_default(&ll->event);
-                if (r < 0) {
-                        ipv4ll_stop(ll, IPV4LL_EVENT_STOP);
+                if (r < 0)
                         return r;
-                }
         }
 
         ll->event_priority = priority;
@@ -457,7 +512,7 @@ int sd_ipv4ll_get_address(sd_ipv4ll *ll, struct in_addr *address){
         return 0;
 }
 
-int sd_ipv4ll_set_address_seed (sd_ipv4ll *ll, uint8_t seed[8]) {
+int sd_ipv4ll_set_address_seed(sd_ipv4ll *ll, uint8_t seed[8]) {
         unsigned int entropy;
         int r;
 
@@ -494,19 +549,18 @@ error:
 bool sd_ipv4ll_is_running(sd_ipv4ll *ll) {
         assert_return(ll, false);
 
-        return !IN_SET(ll->state, IPV4LL_STATE_INIT, IPV4LL_STATE_STOPPED);
+        return ll->state != IPV4LL_STATE_INIT;
 }
 
 #define HASH_KEY SD_ID128_MAKE(df,04,22,98,3f,ad,14,52,f9,87,2e,d1,9c,70,e2,f2)
 
-int sd_ipv4ll_start (sd_ipv4ll *ll) {
+int sd_ipv4ll_start(sd_ipv4ll *ll) {
         int r;
 
         assert_return(ll, -EINVAL);
         assert_return(ll->event, -EINVAL);
         assert_return(ll->index > 0, -EINVAL);
-        assert_return(IN_SET(ll->state, IPV4LL_STATE_INIT,
-                             IPV4LL_STATE_STOPPED), -EBUSY);
+        assert_return(ll->state == IPV4LL_STATE_INIT, -EBUSY);
 
         ll->state = IPV4LL_STATE_INIT;
 
@@ -539,7 +593,7 @@ int sd_ipv4ll_start (sd_ipv4ll *ll) {
         safe_close(ll->fd);
         ll->fd = r;
 
-        ipv4ll_set_state (ll, IPV4LL_STATE_INIT, true);
+        ipv4ll_set_state(ll, IPV4LL_STATE_INIT, true);
 
         r = sd_event_add_io(ll->event, &ll->receive_message, ll->fd,
                             EPOLLIN, ipv4ll_on_packet, ll);
@@ -559,74 +613,7 @@ int sd_ipv4ll_start (sd_ipv4ll *ll) {
                 goto out;
 out:
         if (r < 0)
-                ipv4ll_stop(ll, IPV4LL_EVENT_STOP);
-
-        return 0;
-}
-
-int sd_ipv4ll_stop(sd_ipv4ll *ll) {
-        ipv4ll_stop(ll, IPV4LL_EVENT_STOP);
-        if (ll)
-                ipv4ll_set_state(ll, IPV4LL_STATE_STOPPED, true);
-
-        return 0;
-}
-
-sd_ipv4ll *sd_ipv4ll_ref(sd_ipv4ll *ll) {
-
-        if (!ll)
-                return NULL;
-
-        assert(ll->n_ref >= 1);
-        ll->n_ref++;
-
-        return ll;
-}
-
-sd_ipv4ll *sd_ipv4ll_unref(sd_ipv4ll *ll) {
-
-        if (!ll)
-                return NULL;
-
-        assert(ll->n_ref >= 1);
-        ll->n_ref--;
-
-        if (ll->n_ref > 0)
-                return ll;
-
-        ll->receive_message = sd_event_source_unref(ll->receive_message);
-        ll->fd = safe_close(ll->fd);
-
-        ll->timer = sd_event_source_unref(ll->timer);
-
-        sd_ipv4ll_detach_event(ll);
-
-        free(ll->random_data);
-        free(ll->random_data_state);
-        free(ll);
-
-        return NULL;
-}
-
-DEFINE_TRIVIAL_CLEANUP_FUNC(sd_ipv4ll*, sd_ipv4ll_unref);
-#define _cleanup_ipv4ll_free_ _cleanup_(sd_ipv4ll_unrefp)
-
-int sd_ipv4ll_new(sd_ipv4ll **ret) {
-        _cleanup_ipv4ll_free_ sd_ipv4ll *ll = NULL;
-
-        assert_return(ret, -EINVAL);
-
-        ll = new0(sd_ipv4ll, 1);
-        if (!ll)
-                return -ENOMEM;
-
-        ll->n_ref = 1;
-        ll->state = IPV4LL_STATE_INIT;
-        ll->index = -1;
-        ll->fd = -1;
-
-        *ret = ll;
-        ll = NULL;
+                ipv4ll_stop(ll);
 
         return 0;
 }

diff --git a/src/libsystemd-network/test-ipv4ll.c b/src/libsystemd-network/test-ipv4ll.c
index e7447d322f10dd28978e317778f7f699062ba2d8..55ec2f3972208281db89bc52ab0b0cdcef26eff1 100644 (file)
--- a/src/libsystemd-network/test-ipv4ll.c
+++ b/src/libsystemd-network/test-ipv4ll.c
@@ -133,7 +133,7 @@ static void test_public_api_setters(sd_event *e) {
         assert_se(sd_ipv4ll_set_index(ll, 99) == 0);
 
         assert_se(sd_ipv4ll_ref(ll) == ll);
-        assert_se(sd_ipv4ll_unref(ll) == ll);
+        assert_se(sd_ipv4ll_unref(ll) == NULL);
 
         /* Cleanup */
         assert_se(sd_ipv4ll_unref(ll) == NULL);

diff --git a/src/network/networkd-ipv4ll.c b/src/network/networkd-ipv4ll.c
index 0a27a30278cb3bcaa3d6f0a5f652d20ca7e29cb4..43aaa749ff635552b427abaf289caf4f7b9ced13 100644 (file)
--- a/src/network/networkd-ipv4ll.c
+++ b/src/network/networkd-ipv4ll.c
@@ -195,10 +195,7 @@ static void ipv4ll_handler(sd_ipv4ll *ll, int event, void *userdata){
                         }
                         break;
                 default:
-                        if (event < 0)
-                                log_link_warning(link, "IPv4 link-local error: %s", strerror(-event));
-                        else
-                                log_link_warning(link, "IPv4 link-local unknown event: %d", event);
+                        log_link_warning(link, "IPv4 link-local unknown event: %d", event);
                         break;
         }
 }