Merge pull request #30895 from yuwata/network-drop-object-even-on-fail

network: remove Address object even when we failed to remove relevant address

src/network/networkd-address.c

@@ -1111,18 +1111,35 @@ static int address_set_netlink_message(const Address *address, sd_netlink_messag
         return 0;
 }
 
-static int address_remove_handler(sd_netlink *rtnl, sd_netlink_message *m, Link *link) {
+static int address_remove_handler(sd_netlink *rtnl, sd_netlink_message *m, RemoveRequest *rreq) {
         int r;
 
         assert(m);
-        assert(link);
+        assert(rreq);
 
-        if (IN_SET(link->state, LINK_STATE_FAILED, LINK_STATE_LINGER))
+        Link *link = ASSERT_PTR(rreq->link);
+        Address *address = ASSERT_PTR(rreq->userdata);
+
+        if (link->state == LINK_STATE_LINGER)
                 return 0;
 
         r = sd_netlink_message_get_errno(m);
-        if (r < 0 && r != -EADDRNOTAVAIL)
-                log_link_message_warning_errno(link, m, r, "Could not drop address");
+        if (r < 0) {
+                log_link_message_full_errno(link, m,
+                                            (r == -EADDRNOTAVAIL || !address->link) ? LOG_DEBUG : LOG_WARNING,
+                                            r, "Could not drop address");
+
+                if (address->link) {
+                        /* If the address cannot be removed, then assume the address is already removed. */
+                        log_address_debug(address, "Forgetting", link);
+
+                        Request *req;
+                        if (address_get_request(link, address, &req) >= 0)
+                                address_enter_removed(req->userdata);
+
+                        (void) address_drop(address);
+                }
+        }
 
         return 1;
 }
@@ -1149,13 +1166,9 @@ int address_remove(Address *address, Link *link) {
         if (r < 0)
                 return log_link_warning_errno(link, r, "Could not set netlink attributes: %m");
 
-        r = netlink_call_async(link->manager->rtnl, NULL, m,
-                               address_remove_handler,
-                               link_netlink_destroy_callback, link);
+        r = link_remove_request_add(link, address, address, link->manager->rtnl, m, address_remove_handler);
         if (r < 0)
-                return log_link_warning_errno(link, r, "Could not send rtnetlink message: %m");
-
-        link_ref(link);
+                return log_link_warning_errno(link, r, "Could not queue rtnetlink message: %m");
 
         address_enter_removing(address);
 

src/network/networkd-manager.c

@@ -424,6 +424,7 @@ static int manager_connect_rtnl(Manager *m, int fd) {
 static int manager_post_handler(sd_event_source *s, void *userdata) {
         Manager *manager = ASSERT_PTR(userdata);
 
+        (void) manager_process_remove_requests(manager);
         (void) manager_process_requests(manager);
         (void) manager_clean_all(manager);
         return 0;
@@ -594,6 +595,7 @@ Manager* manager_free(Manager *m) {
                 (void) link_stop_engines(link, true);
 
         m->request_queue = ordered_set_free(m->request_queue);
+        m->remove_request_queue = ordered_set_free(m->remove_request_queue);
 
         m->dirty_links = set_free_with_destructor(m->dirty_links, link_unref);
         m->new_wlan_ifindices = set_free(m->new_wlan_ifindices);

src/network/networkd-manager.h

@@ -102,6 +102,7 @@ struct Manager {
 
         bool request_queued;
         OrderedSet *request_queue;
+        OrderedSet *remove_request_queue;
 
         Hashmap *tuntap_fds_by_name;
 };

src/network/networkd-neighbor.c

@@ -399,19 +399,36 @@ int link_request_static_neighbors(Link *link) {
         return 0;
 }
 
-static int neighbor_remove_handler(sd_netlink *rtnl, sd_netlink_message *m, Link *link) {
+static int neighbor_remove_handler(sd_netlink *rtnl, sd_netlink_message *m, RemoveRequest *rreq) {
         int r;
 
         assert(m);
-        assert(link);
+        assert(rreq);
 
-        if (IN_SET(link->state, LINK_STATE_FAILED, LINK_STATE_LINGER))
-                return 1;
+        Link *link = ASSERT_PTR(rreq->link);
+        Neighbor *neighbor = ASSERT_PTR(rreq->userdata);
+
+        if (link->state == LINK_STATE_LINGER)
+                return 0;
 
         r = sd_netlink_message_get_errno(m);
-        if (r < 0 && r != -ESRCH)
+        if (r < 0) {
                 /* Neighbor may not exist because it already got deleted, ignore that. */
-                log_link_message_warning_errno(link, m, r, "Could not remove neighbor");
+                log_link_message_full_errno(link, m,
+                                            (r == -ESRCH || !neighbor->link) ? LOG_DEBUG : LOG_WARNING,
+                                            r, "Could not remove neighbor");
+
+                if (neighbor->link) {
+                        /* If the neighbor cannot be removed, then assume the neighbor is already removed. */
+                        log_neighbor_debug(neighbor, "Forgetting", link);
+
+                        Request *req;
+                        if (neighbor_get_request(link, neighbor, &req) >= 0)
+                                neighbor_enter_removed(req->userdata);
+
+                        neighbor_detach(neighbor);
+                }
+        }
 
         return 1;
 }
@@ -436,12 +453,9 @@ int neighbor_remove(Neighbor *neighbor, Link *link) {
         if (r < 0)
                 return log_link_error_errno(link, r, "Could not append NDA_DST attribute: %m");
 
-        r = netlink_call_async(link->manager->rtnl, NULL, m, neighbor_remove_handler,
-                               link_netlink_destroy_callback, link);
+        r = link_remove_request_add(link, neighbor, neighbor, link->manager->rtnl, m, neighbor_remove_handler);
         if (r < 0)
-                return log_link_error_errno(link, r, "Could not send rtnetlink message: %m");
-
-        link_ref(link);
+                return log_link_error_errno(link, r, "Could not queue rtnetlink message: %m");
 
         neighbor_enter_removing(neighbor);
         return 0;

src/network/networkd-nexthop.c

@@ -436,19 +436,32 @@ static void log_nexthop_debug(const NextHop *nexthop, const char *str, Manager *
                        yes_no(nexthop->blackhole), strna(group), strna(flags));
 }
 
-static int nexthop_remove_handler(sd_netlink *rtnl, sd_netlink_message *m, Link *link) {
+static int nexthop_remove_handler(sd_netlink *rtnl, sd_netlink_message *m, RemoveRequest *rreq) {
         int r;
 
         assert(m);
+        assert(rreq);
 
-        /* link may be NULL. */
-
-        if (link && IN_SET(link->state, LINK_STATE_FAILED, LINK_STATE_LINGER))
-                return 1;
+        Manager *manager = ASSERT_PTR(rreq->manager);
+        NextHop *nexthop = ASSERT_PTR(rreq->userdata);
 
         r = sd_netlink_message_get_errno(m);
-        if (r < 0 && r != -ENOENT)
-                log_link_message_warning_errno(link, m, r, "Could not drop nexthop, ignoring");
+        if (r < 0) {
+                log_message_full_errno(m,
+                                       (r == -ENOENT || !nexthop->manager) ? LOG_DEBUG : LOG_WARNING,
+                                       r, "Could not drop nexthop, ignoring");
+
+                if (nexthop->manager) {
+                        /* If the nexthop cannot be removed, then assume the nexthop is already removed. */
+                        log_nexthop_debug(nexthop, "Forgetting", manager);
+
+                        Request *req;
+                        if (nexthop_get_request_by_id(manager, nexthop->id, &req) >= 0)
+                                nexthop_enter_removed(req->userdata);
+
+                        nexthop_detach(nexthop);
+                }
+        }
 
         return 1;
 }
@@ -475,12 +488,9 @@ int nexthop_remove(NextHop *nexthop, Manager *manager) {
         if (r < 0)
                 return log_link_error_errno(link, r, "Could not append NHA_ID attribute: %m");
 
-        r = netlink_call_async(manager->rtnl, NULL, m, nexthop_remove_handler,
-                               link ? link_netlink_destroy_callback : NULL, link);
+        r = manager_remove_request_add(manager, nexthop, nexthop, manager->rtnl, m, nexthop_remove_handler);
         if (r < 0)
-                return log_link_error_errno(link, r, "Could not send rtnetlink message: %m");
-
-        link_ref(link); /* link may be NULL, link_ref() is OK with that */
+                return log_link_error_errno(link, r, "Could not queue rtnetlink message: %m");
 
         nexthop_enter_removing(nexthop);
         return 0;

src/network/networkd-queue.c

@@ -224,6 +224,10 @@ int manager_process_requests(Manager *manager) {
 
         assert(manager);
 
+        /* Process only when no remove request is queued. */
+        if (!ordered_set_isempty(manager->remove_request_queue))
+                return 0;
+
         manager->request_queued = false;
 
         ORDERED_SET_FOREACH(req, manager->request_queue) {
@@ -339,3 +343,110 @@ static const char *const request_type_table[_REQUEST_TYPE_MAX] = {
 };
 
 DEFINE_STRING_TABLE_LOOKUP_TO_STRING(request_type, RequestType);
+
+static RemoveRequest* remove_request_free(RemoveRequest *req) {
+        if (!req)
+                return NULL;
+
+        if (req->manager)
+                ordered_set_remove(req->manager->remove_request_queue, req);
+
+        if (req->unref_func)
+                req->unref_func(req->userdata);
+
+        link_unref(req->link);
+        sd_netlink_unref(req->netlink);
+        sd_netlink_message_unref(req->message);
+
+        return mfree(req);
+}
+
+DEFINE_TRIVIAL_CLEANUP_FUNC(RemoveRequest*, remove_request_free);
+DEFINE_TRIVIAL_DESTRUCTOR(remove_request_destroy_callback, RemoveRequest, remove_request_free);
+DEFINE_PRIVATE_HASH_OPS_WITH_KEY_DESTRUCTOR(
+                remove_request_hash_ops,
+                void,
+                trivial_hash_func,
+                trivial_compare_func,
+                remove_request_free);
+
+int remove_request_add(
+                Manager *manager,
+                Link *link,
+                void *userdata,
+                mfree_func_t unref_func,
+                sd_netlink *netlink,
+                sd_netlink_message *message,
+                remove_request_netlink_handler_t netlink_handler) {
+
+        _cleanup_(remove_request_freep) RemoveRequest *req = NULL;
+        int r;
+
+        assert(manager);
+        assert(userdata);
+        assert(netlink);
+        assert(message);
+
+        req = new(RemoveRequest, 1);
+        if (!req)
+                return -ENOMEM;
+
+        *req = (RemoveRequest) {
+                .link = link_ref(link), /* link may be NULL, but link_ref() handles it gracefully. */
+                .userdata = userdata,
+                .netlink = sd_netlink_ref(netlink),
+                .message = sd_netlink_message_ref(message),
+                .netlink_handler = netlink_handler,
+        };
+
+        r = ordered_set_ensure_put(&manager->remove_request_queue, &remove_request_hash_ops, req);
+        if (r < 0)
+                return r;
+        assert(r > 0);
+
+        req->manager = manager;
+        req->unref_func = unref_func;
+
+        TAKE_PTR(req);
+        return 0;
+}
+
+int manager_process_remove_requests(Manager *manager) {
+        RemoveRequest *req;
+        int r;
+
+        assert(manager);
+
+        while ((req = ordered_set_first(manager->remove_request_queue))) {
+
+                /* Do not make the reply callback queue in sd-netlink full. */
+                if (netlink_get_reply_callback_count(req->netlink) >= REPLY_CALLBACK_COUNT_THRESHOLD)
+                        return 0;
+
+                r = netlink_call_async(
+                                req->netlink, NULL, req->message,
+                                req->netlink_handler,
+                                remove_request_destroy_callback,
+                                req);
+                if (r < 0) {
+                        _cleanup_(link_unrefp) Link *link = link_ref(req->link);
+
+                        log_link_warning_errno(link, r, "Failed to call netlink message: %m");
+
+                        /* First free the request. */
+                        remove_request_free(req);
+
+                        /* Then, make the link enter the failed state. */
+                        if (link)
+                                link_enter_failed(link);
+
+                } else {
+                        /* On success, netlink needs to be unref()ed. Otherwise, the netlink and remove
+                         * request may not freed on shutting down. */
+                        req->netlink = sd_netlink_unref(req->netlink);
+                        ordered_set_remove(manager->remove_request_queue, req);
+                }
+        }
+
+        return 0;
+}

src/network/networkd-queue.h

@@ -139,3 +139,52 @@ int manager_process_requests(Manager *manager);
 int request_call_netlink_async(sd_netlink *nl, sd_netlink_message *m, Request *req);
 
 const char* request_type_to_string(RequestType t) _const_;
+
+typedef struct RemoveRequest RemoveRequest;
+typedef int (*remove_request_netlink_handler_t)(sd_netlink *nl, sd_netlink_message *m, RemoveRequest *req);
+
+struct RemoveRequest {
+        Manager *manager;
+        Link *link;
+        void *userdata; /* e.g. Address */
+        mfree_func_t unref_func; /* e.g. address_unref() */
+        sd_netlink *netlink;
+        sd_netlink_message *message;
+        remove_request_netlink_handler_t netlink_handler;
+};
+
+int remove_request_add(
+                Manager *manager,
+                Link *link,
+                void *userdata, /* This is unref()ed when the call failed. */
+                mfree_func_t unref_func,
+                sd_netlink *netlink,
+                sd_netlink_message *message,
+                remove_request_netlink_handler_t netlink_handler);
+
+#define _remove_request_add(manager, link, data, name, nl, m, handler)  \
+        ({                                                              \
+                typeof(*data) *_data = (data);                          \
+                int _r;                                                 \
+                                                                        \
+                _r = remove_request_add(manager, link, _data,           \
+                                        (mfree_func_t) name##_unref,    \
+                                        nl, m, handler);                \
+                if (_r >= 0)                                            \
+                        name##_ref(_data);                              \
+                _r;                                                     \
+        })
+
+
+#define link_remove_request_add(link, data, name, nl, m, handler)       \
+        ({                                                              \
+                Link *_link = (link);                                   \
+                                                                        \
+                _remove_request_add(_link->manager, _link, data, name,  \
+                                    nl, m, handler);                    \
+        })
+
+#define manager_remove_request_add(manager, data, name, nl, m, handler) \
+        _remove_request_add(manager, NULL, data, name, nl, m, handler)
+
+int manager_process_remove_requests(Manager *manager);