misc fixes termination

src/CMakeLists.txt

@@ -64,10 +64,10 @@ set(VERBOSE false)
 set(CMAKE_VERBOSE_MAKEFILE true)
 
 if(CMAKE_BUILD_TYPE STREQUAL Debug)
-   set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wno-missing-declarations")
+   set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wno-missing-declarations -Wno-error=pedantic")
    set(DEBUG_BUILD true)
 else()
-   set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wno-missing-declarations")
+   set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wno-missing-declarations -Wno-error=pedantic")
    set(DEBUG_BUILD false)
 endif()
 

src/hh_dp_comm.c

@@ -58,6 +58,7 @@ static uint64_t synt = 0;
 /* global */
 struct fmt_buff *fb = NULL;
 bool log_dataplane_msg = true;
+bool finalizing = false;
 
 /* tell if a unix path is valid */
 static bool is_valid_unix_path(const char *path)
@@ -156,7 +157,11 @@ static int dp_unix_sock_open(const char *bind_path)
     /* build bind address */
     struct sockaddr_un un_src = {0};
     size_t path_len = strlen(bind_path);
-    BUG(!path_len || path_len >= sizeof(un_src.sun_path), -1);
+    if (!path_len || path_len >= sizeof(un_src.sun_path)) {
+        zlog_err("Invalid path len %zu", path_len);
+        goto fail;
+    }
+
     un_src.sun_family = AF_UNIX;
     strncpy(un_src.sun_path, bind_path, sizeof(un_src.sun_path));
 
@@ -365,6 +370,7 @@ static void wakeon_dp_write_avail(void)
 /* Main function to send an RPC message. The dp_msg is not sent straight but queued in the
  * unsent queue (to preserve ordering) and any prior pending messages are sent first, by
  * calling send_pending_rpc_msgs(). Connect requests overtake any pending messsages.
+ * This function takes ownership of dp_msg and is responsible for recycling it or queueing it.
  */
 int send_rpc_msg(struct dp_msg *dp_msg)
 {
@@ -499,17 +505,15 @@ void fini_dplane_rpc(void)
     /* close Unix sock */
     dp_unix_sock_close();
 
-    /* politeness */
-    event_cancel(&ev_recv);
-    event_cancel(&ev_send);
-    event_cancel(&ev_connect_timer);
-    event_cancel(&ev_keepalive);
-
     /* destroy rx / tx buffers */
-    if (tx_buff)
+    if (tx_buff) {
         buff_free(tx_buff);
-    if (rx_buff)
+        tx_buff = NULL;
+    }
+    if (rx_buff) {
         buff_free(rx_buff);
+        rx_buff = NULL;
+    }
 
     /* finalize message cache */
     fini_dp_msg_cache();
@@ -519,7 +523,6 @@ void fini_dplane_rpc(void)
         fini_fmt_buff(fb);
         fb = NULL;
     }
-
 }
 
 /* allocate Tx/Rx buffers for RPC encoding/decoding */
@@ -558,11 +561,11 @@ int init_dplane_rpc(void)
     /* open unix socket and bind it */
     dp_sock = dp_unix_sock_open(plugin_sock_path);
     if (dp_sock == NO_SOCK)
-        return -1;
+        goto fail;
 
     /* allocate Tx/Rx buffers for RPC encoding/decoding */
     if (init_rpc_buffers() != 0)
-        return -1;
+        goto fail;
 
     /* initialize msg cache */
     init_dp_msg_cache();
@@ -578,4 +581,10 @@ int init_dplane_rpc(void)
 
     /* success */
     return 0;
+
+fail:
+    dp_unix_sock_close();
+    fini_fmt_buff(fb);
+    fb = NULL;
+    return -1;
 }

src/hh_dp_comm.h

@@ -8,6 +8,7 @@
 #include "hh_dp_msg_cache.h"
 
 extern bool log_dataplane_msg;
+extern bool finalizing;
 
 /* set dp unix sock local path */
 int set_dp_sock_local_path(const char *path);

src/hh_dp_internal.h

@@ -3,7 +3,6 @@
 #ifndef SRC_HH_DP_INTERNAL_H_
 #define SRC_HH_DP_INTERNAL_H_
 
-#include "lib/assert/assert.h"
 #include "lib/zlog.h"
 #include "hh_dp_config.h"
 

src/hh_dp_msg.c

@@ -10,9 +10,7 @@
 
 #include "zebra/zebra_dplane.h"
 #include "lib/libfrr.h"
-#include "lib/assert/assert.h"
 #include "zebra/debug.h"
-
 #include "hh_dp_internal.h"
 #include "hh_dp_comm.h" /* send_rpc_msg() && dplane_is_ready() */
 #include "hh_dp_process.h" /* struct zebra_dplane_provider */
@@ -299,6 +297,18 @@ static void handle_rpc_connect_response(struct RpcResponse *resp, bool purged)
         abort();
     }
 }
+
+void dp_msg_hand_off(struct dp_msg *dp_msg, enum zebra_dplane_result result) {
+
+    /* set result */
+    dplane_ctx_set_status(dp_msg->ctx, result);
+
+    /* queue back to zebra */
+    dplane_provider_enqueue_out_ctx(prov_p, dp_msg->ctx);
+    dplane_provider_work_ready();
+    dp_msg->ctx = NULL; /* imposed to allow recycle */
+}
+
 static void handle_rpc_ctx_response(struct dp_msg *m, RpcResultCode rescode)
 {
     BUG(!prov_p);
@@ -307,12 +317,7 @@ static void handle_rpc_ctx_response(struct dp_msg *m, RpcResultCode rescode)
     if (m->ctx) {
         /* set the result - we treat ignored requests as successes for the time being */
         enum zebra_dplane_result result = (rescode == Ok || rescode == Ignored) ? ZEBRA_DPLANE_REQUEST_SUCCESS : ZEBRA_DPLANE_REQUEST_FAILURE;
-        dplane_ctx_set_status(m->ctx, result);
-
-        /* queue back to zebra */
-        dplane_provider_enqueue_out_ctx(prov_p, m->ctx);
-        dplane_provider_work_ready();
-        m->ctx = NULL; /* imposed to allow recycle */
+        dp_msg_hand_off(m, result);
     }
 }
 static void do_handle_rpc_response(struct RpcResponse *resp, struct dp_msg *m, bool purged) {

src/hh_dp_msg.h

@@ -16,4 +16,6 @@ int send_rpc_control(uint8_t refresh);
 /* Entry point for RPC msg processing */
 void handle_rpc_msg(struct RpcMsg *msg);
 
+void dp_msg_hand_off(struct dp_msg *dp_msg, enum zebra_dplane_result result);
+
 #endif /* SRC_HH_DP_MSG_H_ */

src/hh_dp_msg_cache.c

@@ -4,6 +4,7 @@
 #include "lib/libfrr.h"
 #include "hh_dp_internal.h"
 #include "hh_dp_msg_cache.h"
+#include "zebra/zebra_dplane.h" /* dplane_ctx_reset() */
 
 DEFINE_MTYPE(ZEBRA, HH_DP_MSG, "HH Dataplane msg");
 
@@ -18,11 +19,23 @@ struct dp_msg_cache {
 static void dp_msg_del(struct dp_msg *msg)
 {
     BUG(!msg);
+    if (msg->ctx) {
+	zlog_warn("Deleting dp_msg referring to a ctx...");
+        /* dplane_ctx_free is private, hence we cannot free the context. This is only
+        * called on shutdown, so all good */
 #if 0
-    /* dplane_ctx_free is private, hence we cannot free the context */
-    if (msg->ctx)
         dplane_ctx_free(&msg->ctx);
+#else
+        /* Workaround that avoids needing to patch frr:
+         * call dplane_ctx_reset() and free ourselves.
+         * XFREE wraps free(). The danger here is that the memory tracking will
+         * not be aware of this free. But that's fine since we do that on termination.
+         */
+        dplane_ctx_reset(msg->ctx);
+        free(msg->ctx);
+        msg->ctx = NULL;
 #endif
+    }
     XFREE(MTYPE_HH_DP_MSG, msg);
 }
 
@@ -34,7 +47,6 @@ static void empty_dp_msg_list(struct dp_msg_list_head *list, const char *name)
     zlog_debug("Emptying cache list '%s' (%zu messages)", name, dp_msg_list_count(list));
     while((msg = dp_msg_list_pop(list)) != NULL)
         dp_msg_del(msg);
-    dp_msg_list_fini(list);
 }
 
 /* allocate a dp_msg */
@@ -53,6 +65,13 @@ struct dp_msg *dp_msg_new(void)
     return m;
 }
 
+void log_dp_msg_lists(void) {
+    zlog_debug("pool: %zu unsent: %zu in-flight: %zu",
+        dp_msg_pool_count(),
+        dp_msg_unsent_count(),
+        dp_msg_in_flight_count());
+}
+
 /* recycle a dp_msg for later reuse */
 void dp_msg_recycle(struct dp_msg *msg)
 {
@@ -88,10 +107,21 @@ size_t dp_msg_unsent_count(void) {
     return dp_msg_list_count(&msg_cache.unsent);
 }
 
+/* length of pool list */
+size_t dp_msg_pool_count(void) {
+    return dp_msg_list_count(&msg_cache.pool);
+}
+
+/* length of inflight list */
+size_t dp_msg_in_flight_count(void) {
+    return dp_msg_list_count(&msg_cache.in_flight);
+}
+
 /* Cache a message (e.g. a Request) until we get the corresponding response */
 void dp_msg_cache_inflight(struct dp_msg *msg)
 {
     BUG(!msg);
+    assert(msg->msg.type == Request);
     dp_msg_list_add_tail(&msg_cache.in_flight, msg);
 }
 
@@ -118,7 +148,7 @@ int init_dp_msg_cache(void)
         if (msg)
             dp_msg_list_add_tail(&msg_cache.pool, msg);
     }
-    zlog_debug("Initialized cache with pool of %zu messages", dp_msg_list_count(&msg_cache.pool));
+    zlog_debug("Initialized cache with pool of %zu messages", dp_msg_pool_count());
     return 0;
 }
 

src/hh_dp_msg_cache.h

@@ -41,8 +41,10 @@ void dp_msg_unsent_push_back(struct dp_msg *msg);
 
 struct dp_msg *dp_msg_pop_unsent(void);
 
-/* length of unsent list */
+/* length of lists */
+size_t dp_msg_pool_count(void);
 size_t dp_msg_unsent_count(void);
+size_t dp_msg_in_flight_count(void);
 
 /* put message in list of sent messages */
 void dp_msg_cache_inflight(struct dp_msg *msg);

src/hh_dp_plugin.c

@@ -3,7 +3,6 @@
 #include "config.h" /* FRR config: must include */
 #include "lib/zebra.h"
 #include "lib/libfrr.h"
-#include "lib/assert/assert.h"
 #include "zebra/zebra_dplane.h"
 #include "zebra/debug.h"
 
@@ -78,9 +77,12 @@ static int zd_hh_start(struct zebra_dplane_provider *prov)
  */
 static int zd_hh_fini(struct zebra_dplane_provider *prov, bool early)
 {
-    zlog_info("%s: Finalizing...", dplane_provider_get_name(prov));
-
-    fini_dplane_rpc();
+    if (early) {
+        zlog_info("%s: Finalizing...", dplane_provider_get_name(prov));
+        finalizing = true;
+    } else {
+        fini_dplane_rpc();
+    }
     return 0;
 }
 
@@ -104,8 +106,17 @@ static int zd_hh_process(struct zebra_dplane_provider *prov)
         if (!ctx)
             break;
 
-        zd_hh_process_update(prov, ctx);
+        if (!finalizing) {
+            zd_hh_process_update(prov, ctx);
+        } else {
+            // tell zebra deletions failed
+            dplane_ctx_set_status(ctx, ZEBRA_DPLANE_REQUEST_FAILURE);
+            dplane_provider_enqueue_out_ctx(prov, ctx);
+        }
     }
+
+    if (finalizing)
+        dplane_provider_work_ready();
     return 0;
 }
 

src/hh_dp_process.c

@@ -3,7 +3,6 @@
 #include "config.h" /* Include this explicitly */
 #include "lib/zebra.h"
 #include "lib/libfrr.h"
-#include "lib/assert/assert.h"
 #include "zebra/zebra_dplane.h"
 #include "zebra/debug.h"
 
@@ -271,6 +270,8 @@ static hh_dp_res_t hh_process(struct zebra_dplane_ctx *ctx)
         case DPLANE_OP_TC_FILTER_ADD:
         case DPLANE_OP_TC_FILTER_DELETE:
         case DPLANE_OP_TC_FILTER_UPDATE:
+        case DPLANE_OP_VLAN_INSTALL:
+        case DPLANE_OP_PROVIDER_REFRESH:
             return HH_IGNORED;
 
         /* Startup Control */