zebra,pim : Fixing Review comments in PIM_MLAG

Signed-off-by: Satheesh Kumar K <sathk@cumulusnetworks.com>

diff --git a/lib/mlag.c b/lib/mlag.c
index 7aac571da679a50224fb28f39e1ddab006dc5b66..1daf2907252210d18022081390f124261b5d4ed4 100644 (file)
--- a/lib/mlag.c
+++ b/lib/mlag.c
@@ -40,8 +40,7 @@ char *mlag_role2str(enum mlag_role role, char *buf, size_t size)
        return buf;
 }
 
-char *zebra_mlag_lib_msgid_to_str(enum mlag_msg_type msg_type, char *buf,
-                                 size_t size)
+char *mlag_lib_msgid_to_str(enum mlag_msg_type msg_type, char *buf, size_t size)
 {
        switch (msg_type) {
        case MLAG_REGISTER:
@@ -82,7 +81,7 @@ char *zebra_mlag_lib_msgid_to_str(enum mlag_msg_type msg_type, char *buf,
 }
 
 
-int zebra_mlag_lib_decode_mlag_hdr(struct stream *s, struct mlag_msg *msg)
+int mlag_lib_decode_mlag_hdr(struct stream *s, struct mlag_msg *msg)
 {
        if (s == NULL || msg == NULL)
                return -1;
@@ -95,8 +94,7 @@ stream_failure:
        return -1;
 }
 
-int zebra_mlag_lib_decode_mroute_add(struct stream *s,
-                                    struct mlag_mroute_add *msg)
+int mlag_lib_decode_mroute_add(struct stream *s, struct mlag_mroute_add *msg)
 {
        if (s == NULL || msg == NULL)
                return -1;
@@ -115,8 +113,7 @@ stream_failure:
        return -1;
 }
 
-int zebra_mlag_lib_decode_mroute_del(struct stream *s,
-                                    struct mlag_mroute_del *msg)
+int mlag_lib_decode_mroute_del(struct stream *s, struct mlag_mroute_del *msg)
 {
        if (s == NULL || msg == NULL)
                return -1;
@@ -132,7 +129,7 @@ stream_failure:
        return -1;
 }
 
-int zebra_mlag_lib_decode_mlag_status(struct stream *s, struct mlag_status *msg)
+int mlag_lib_decode_mlag_status(struct stream *s, struct mlag_status *msg)
 {
        if (s == NULL || msg == NULL)
                return -1;
@@ -145,7 +142,7 @@ stream_failure:
        return -1;
 }
 
-int zebra_mlag_lib_decode_vxlan_update(struct stream *s, struct mlag_vxlan *msg)
+int mlag_lib_decode_vxlan_update(struct stream *s, struct mlag_vxlan *msg)
 {
        if (s == NULL || msg == NULL)
                return -1;
@@ -158,8 +155,7 @@ stream_failure:
        return -1;
 }
 
-int zebra_mlag_lib_decode_frr_status(struct stream *s,
-                                    struct mlag_frr_status *msg)
+int mlag_lib_decode_frr_status(struct stream *s, struct mlag_frr_status *msg)
 {
        if (s == NULL || msg == NULL)
                return -1;

diff --git a/lib/mlag.h b/lib/mlag.h
index c8a3456980cc35803023961d3ff13e37c8ef6e97..c531fb5b68772f87d26c257f5403c7b9a577dd8b 100644 (file)
--- a/lib/mlag.h
+++ b/lib/mlag.h
@@ -119,24 +119,23 @@ struct mlag_msg {
        uint16_t data_len;
        uint16_t msg_cnt;
        uint8_t data[0];
-}__attribute__((packed));
+} __attribute__((packed));
 
 
 extern char *mlag_role2str(enum mlag_role role, char *buf, size_t size);
-extern char *zebra_mlag_lib_msgid_to_str(enum mlag_msg_type msg_type, char *buf,
-                                        size_t size);
-extern int zebra_mlag_lib_decode_mlag_hdr(struct stream *s,
-                                         struct mlag_msg *msg);
-extern int zebra_mlag_lib_decode_mroute_add(struct stream *s,
-                                           struct mlag_mroute_add *msg);
-extern int zebra_mlag_lib_decode_mroute_del(struct stream *s,
-                                           struct mlag_mroute_del *msg);
-extern int zebra_mlag_lib_decode_mlag_status(struct stream *s,
-                                            struct mlag_status *msg);
-extern int zebra_mlag_lib_decode_vxlan_update(struct stream *s,
-                                             struct mlag_vxlan *msg);
-extern int zebra_mlag_lib_decode_frr_status(struct stream *s,
-                                           struct mlag_frr_status *msg);
+extern char *mlag_lib_msgid_to_str(enum mlag_msg_type msg_type, char *buf,
+                                  size_t size);
+extern int mlag_lib_decode_mlag_hdr(struct stream *s, struct mlag_msg *msg);
+extern int mlag_lib_decode_mroute_add(struct stream *s,
+                                     struct mlag_mroute_add *msg);
+extern int mlag_lib_decode_mroute_del(struct stream *s,
+                                     struct mlag_mroute_del *msg);
+extern int mlag_lib_decode_mlag_status(struct stream *s,
+                                      struct mlag_status *msg);
+extern int mlag_lib_decode_vxlan_update(struct stream *s,
+                                       struct mlag_vxlan *msg);
+extern int mlag_lib_decode_frr_status(struct stream *s,
+                                     struct mlag_frr_status *msg);
 #ifdef __cplusplus
 }
 #endif

diff --git a/pimd/pim_mlag.c b/pimd/pim_mlag.c
index a66f604d52b12b59344c39df25ec43fea4a41aa4..f60c18204b6d5179c1886445eb6c6d5869634b3d 100644 (file)
--- a/pimd/pim_mlag.c
+++ b/pimd/pim_mlag.c
@@ -87,22 +87,22 @@ int pim_zebra_mlag_handle_msg(struct stream *s, int len)
        char buf[ZLOG_FILTER_LENGTH_MAX];
        int rc = 0;
 
-       rc = zebra_mlag_lib_decode_mlag_hdr(s, &mlag_msg);
+       rc = mlag_lib_decode_mlag_hdr(s, &mlag_msg);
        if (rc)
                return (rc);
 
        if (PIM_DEBUG_MLAG)
                zlog_debug("%s: Received msg type: %s length: %d, bulk_cnt: %d",
                           __func__,
-                          zebra_mlag_lib_msgid_to_str(mlag_msg.msg_type, buf,
-                                                      sizeof(buf)),
+                          mlag_lib_msgid_to_str(mlag_msg.msg_type, buf,
+                                                sizeof(buf)),
                           mlag_msg.data_len, mlag_msg.msg_cnt);
 
        switch (mlag_msg.msg_type) {
        case MLAG_STATUS_UPDATE: {
                struct mlag_status msg;
 
-               rc = zebra_mlag_lib_decode_mlag_status(s, &msg);
+               rc = mlag_lib_decode_mlag_status(s, &msg);
                if (rc)
                        return (rc);
                pim_mlag_process_mlagd_state_change(msg);
@@ -110,7 +110,7 @@ int pim_zebra_mlag_handle_msg(struct stream *s, int len)
        case MLAG_PEER_FRR_STATUS: {
                struct mlag_frr_status msg;
 
-               rc = zebra_mlag_lib_decode_frr_status(s, &msg);
+               rc = mlag_lib_decode_frr_status(s, &msg);
                if (rc)
                        return (rc);
                pim_mlag_process_peer_frr_state_change(msg);
@@ -118,7 +118,7 @@ int pim_zebra_mlag_handle_msg(struct stream *s, int len)
        case MLAG_VXLAN_UPDATE: {
                struct mlag_vxlan msg;
 
-               rc = zebra_mlag_lib_decode_vxlan_update(s, &msg);
+               rc = mlag_lib_decode_vxlan_update(s, &msg);
                if (rc)
                        return rc;
                pim_mlag_process_vxlan_update(&msg);
@@ -126,7 +126,7 @@ int pim_zebra_mlag_handle_msg(struct stream *s, int len)
        case MLAG_MROUTE_ADD: {
                struct mlag_mroute_add msg;
 
-               rc = zebra_mlag_lib_decode_mroute_add(s, &msg);
+               rc = mlag_lib_decode_mroute_add(s, &msg);
                if (rc)
                        return (rc);
                pim_mlag_process_mroute_add(msg);
@@ -134,7 +134,7 @@ int pim_zebra_mlag_handle_msg(struct stream *s, int len)
        case MLAG_MROUTE_DEL: {
                struct mlag_mroute_del msg;
 
-               rc = zebra_mlag_lib_decode_mroute_del(s, &msg);
+               rc = mlag_lib_decode_mroute_del(s, &msg);
                if (rc)
                        return (rc);
                pim_mlag_process_mroute_del(msg);
@@ -145,7 +145,7 @@ int pim_zebra_mlag_handle_msg(struct stream *s, int len)
 
                for (i = 0; i < mlag_msg.msg_cnt; i++) {
 
-                       rc = zebra_mlag_lib_decode_mroute_add(s, &msg);
+                       rc = mlag_lib_decode_mroute_add(s, &msg);
                        if (rc)
                                return (rc);
                        pim_mlag_process_mroute_add(msg);
@@ -157,7 +157,7 @@ int pim_zebra_mlag_handle_msg(struct stream *s, int len)
 
                for (i = 0; i < mlag_msg.msg_cnt; i++) {
 
-                       rc = zebra_mlag_lib_decode_mroute_del(s, &msg);
+                       rc = mlag_lib_decode_mroute_del(s, &msg);
                        if (rc)
                                return (rc);
                        pim_mlag_process_mroute_del(msg);

diff --git a/zebra/zebra_mlag.c b/zebra/zebra_mlag.c
index 1cbe54b178714c250561ac39265fd03cacd35e2a..1a911e429f66855c940d322e85483742d93af716 100644 (file)
--- a/zebra/zebra_mlag.c
+++ b/zebra/zebra_mlag.c
@@ -153,7 +153,7 @@ static int zebra_mlag_client_msg_handler(struct thread *event)
 
        wr_count = stream_fifo_count_safe(zrouter.mlag_info.mlag_fifo);
        if (IS_ZEBRA_DEBUG_MLAG)
-               zlog_debug(":%s: Processing MLAG write, %d messages in queue",
+               zlog_debug(":%s: Processing MLAG write, %u messages in queue",
                           __func__, wr_count);
 
        max_count = MIN(wr_count, ZEBRA_MLAG_POST_LIMIT);
@@ -240,16 +240,17 @@ void zebra_mlag_handle_process_state(enum zebra_mlag_state state)
  */
 static int zebra_mlag_signal_write_thread(void)
 {
-       frr_with_mutex (&zrouter.mlag_info.mlag_th_mtx) {
-               if (zrouter.mlag_info.zebra_pth_mlag) {
-                       if (IS_ZEBRA_DEBUG_MLAG)
-                               zlog_debug(":%s: Scheduling MLAG write",
-                                          __func__);
-                       thread_add_event(zrouter.mlag_info.th_master,
-                                        zebra_mlag_client_msg_handler, NULL, 0,
-                                        &zrouter.mlag_info.t_write);
-               }
-       }
+       if (IS_ZEBRA_DEBUG_MLAG)
+               zlog_debug(":%s: Scheduling MLAG write", __func__);
+       /*
+        * This api will be called from Both main & MLAG Threads.
+        * main thread writes, "zrouter.mlag_info.th_master" only
+        * during Zebra Init/after MLAG thread is destroyed.
+        * so it is safe to use without any locking
+        */
+       thread_add_event(zrouter.mlag_info.th_master,
+                        zebra_mlag_client_msg_handler, NULL, 0,
+                        &zrouter.mlag_info.t_write);
        return 0;
 }
 
@@ -593,13 +594,14 @@ DEFUN_HIDDEN (show_mlag,
        return CMD_SUCCESS;
 }
 
-DEFPY(test_mlag, test_mlag_cmd,
-      "test zebra mlag <none$none|primary$primary|secondary$secondary>",
-      "Test code\n" ZEBRA_STR
-      "Modify the Mlag state\n"
-      "Mlag is not setup on the machine\n"
-      "Mlag is setup to be primary\n"
-      "Mlag is setup to be the secondary\n")
+DEFPY_HIDDEN(test_mlag, test_mlag_cmd,
+            "test zebra mlag <none$none|primary$primary|secondary$secondary>",
+            "Test code\n"
+            ZEBRA_STR
+            "Modify the Mlag state\n"
+            "Mlag is not setup on the machine\n"
+            "Mlag is setup to be primary\n"
+            "Mlag is setup to be the secondary\n")
 {
        enum mlag_role orig = zrouter.mlag_info.role;
        char buf1[MLAG_ROLE_STRSIZE], buf2[MLAG_ROLE_STRSIZE];
@@ -621,12 +623,8 @@ DEFPY(test_mlag, test_mlag_cmd,
                if (zrouter.mlag_info.role != MLAG_ROLE_NONE) {
                        if (zrouter.mlag_info.clients_interested_cnt == 0
                            && test_mlag_in_progress == false) {
-                               frr_with_mutex (
-                                       &zrouter.mlag_info.mlag_th_mtx) {
-                                       if (zrouter.mlag_info.zebra_pth_mlag
-                                           == NULL)
-                                               zebra_mlag_spawn_pthread();
-                               }
+                               if (zrouter.mlag_info.zebra_pth_mlag == NULL)
+                                       zebra_mlag_spawn_pthread();
                                zrouter.mlag_info.clients_interested_cnt++;
                                test_mlag_in_progress = true;
                                zebra_mlag_private_open_channel();
@@ -649,8 +647,8 @@ void zebra_mlag_init(void)
        install_element(ENABLE_NODE, &test_mlag_cmd);
 
        /*
-        * Intialiaze teh MLAG Global variableis
-        * write thread will be craeted during actual registration with MCLAG
+        * Intialiaze the MLAG Global variables
+        * write thread will be created during actual registration with MCLAG
         */
        zrouter.mlag_info.clients_interested_cnt = 0;
        zrouter.mlag_info.connected = false;
@@ -662,7 +660,6 @@ void zebra_mlag_init(void)
        zrouter.mlag_info.t_write = NULL;
        test_mlag_in_progress = false;
        zebra_mlag_reset_read_buffer();
-       pthread_mutex_init(&zrouter.mlag_info.mlag_th_mtx, NULL);
 }
 
 void zebra_mlag_terminate(void)
@@ -692,18 +689,16 @@ int zebra_mlag_protobuf_encode_client_data(struct stream *s, uint32_t *msg_type)
        if (IS_ZEBRA_DEBUG_MLAG)
                zlog_debug("%s: Entering..", __func__);
 
-       rc = zebra_mlag_lib_decode_mlag_hdr(s, &mlag_msg);
+       rc = mlag_lib_decode_mlag_hdr(s, &mlag_msg);
        if (rc)
                return rc;
 
        if (IS_ZEBRA_DEBUG_MLAG)
-               zlog_debug("%s: Decoded msg length:%d..", __func__,
+               zlog_debug("%s: Mlag ProtoBuf encoding of message:%s, len:%d",
+                          __func__,
+                          mlag_lib_msgid_to_str(mlag_msg.msg_type, buf,
+                                                sizeof(buf)),
                           mlag_msg.data_len);
-
-       if (IS_ZEBRA_DEBUG_MLAG)
-               zlog_debug("%s: Mlag ProtoBuf encoding of message:%s", __func__,
-                          zebra_mlag_lib_msgid_to_str(mlag_msg.msg_type, buf,
-                                                      sizeof(buf)));
        *msg_type = mlag_msg.msg_type;
        switch (mlag_msg.msg_type) {
        case MLAG_MROUTE_ADD: {
@@ -711,7 +706,7 @@ int zebra_mlag_protobuf_encode_client_data(struct stream *s, uint32_t *msg_type)
                ZebraMlagMrouteAdd pay_load = ZEBRA_MLAG_MROUTE_ADD__INIT;
                uint32_t vrf_name_len = 0;
 
-               rc = zebra_mlag_lib_decode_mroute_add(s, &msg);
+               rc = mlag_lib_decode_mroute_add(s, &msg);
                if (rc)
                        return rc;
                vrf_name_len = strlen(msg.vrf_name) + 1;
@@ -743,7 +738,7 @@ int zebra_mlag_protobuf_encode_client_data(struct stream *s, uint32_t *msg_type)
                ZebraMlagMrouteDel pay_load = ZEBRA_MLAG_MROUTE_DEL__INIT;
                uint32_t vrf_name_len = 0;
 
-               rc = zebra_mlag_lib_decode_mroute_del(s, &msg);
+               rc = mlag_lib_decode_mroute_del(s, &msg);
                if (rc)
                        return rc;
                vrf_name_len = strlen(msg.vrf_name) + 1;
@@ -783,7 +778,7 @@ int zebra_mlag_protobuf_encode_client_data(struct stream *s, uint32_t *msg_type)
 
                        uint32_t vrf_name_len = 0;
 
-                       rc = zebra_mlag_lib_decode_mroute_add(s, &msg);
+                       rc = mlag_lib_decode_mroute_add(s, &msg);
                        if (rc) {
                                cleanup = true;
                                break;
@@ -848,7 +843,7 @@ int zebra_mlag_protobuf_encode_client_data(struct stream *s, uint32_t *msg_type)
 
                        uint32_t vrf_name_len = 0;
 
-                       rc = zebra_mlag_lib_decode_mroute_del(s, &msg);
+                       rc = mlag_lib_decode_mroute_del(s, &msg);
                        if (rc) {
                                cleanup = true;
                                break;
@@ -903,8 +898,8 @@ int zebra_mlag_protobuf_encode_client_data(struct stream *s, uint32_t *msg_type)
        if (IS_ZEBRA_DEBUG_MLAG)
                zlog_debug("%s: length of Mlag ProtoBuf encoded message:%s, %d",
                           __func__,
-                          zebra_mlag_lib_msgid_to_str(mlag_msg.msg_type, buf,
-                                                      sizeof(buf)),
+                          mlag_lib_msgid_to_str(mlag_msg.msg_type, buf,
+                                                sizeof(buf)),
                           len);
        hdr.type = (ZebraMlagHeader__MessageType)mlag_msg.msg_type;
        if (len != 0) {
@@ -934,8 +929,8 @@ int zebra_mlag_protobuf_encode_client_data(struct stream *s, uint32_t *msg_type)
                zlog_debug(
                        "%s: length of Mlag ProtoBuf message:%s with Header  %d",
                        __func__,
-                       zebra_mlag_lib_msgid_to_str(mlag_msg.msg_type, buf,
-                                                   sizeof(buf)),
+                       mlag_lib_msgid_to_str(mlag_msg.msg_type, buf,
+                                             sizeof(buf)),
                        len);
        if (hdr.data.data)
                XFREE(MTYPE_MLAG_PBUF, hdr.data.data);
@@ -963,7 +958,7 @@ int zebra_mlag_protobuf_decode_message(struct stream *s, uint8_t *data,
 
        if (IS_ZEBRA_DEBUG_MLAG)
                zlog_debug("%s: Mlag ProtoBuf decoding of message:%s", __func__,
-                          zebra_mlag_lib_msgid_to_str(msg_type, buf, 80));
+                          mlag_lib_msgid_to_str(msg_type, buf, 80));
 
        /*
         * Internal MLAG Message-types & MLAG.proto message types should

diff --git a/zebra/zebra_mlag_private.c b/zebra/zebra_mlag_private.c
index 6cb40a9c12a03a66d59cb9686866ecbcd63ffd02..6a31d47055cc1bef19976a29fe464034e4463b2b 100644 (file)
--- a/zebra/zebra_mlag_private.c
+++ b/zebra/zebra_mlag_private.c
@@ -71,10 +71,8 @@ int zebra_mlag_private_write_data(uint8_t *data, uint32_t len)
 
 static void zebra_mlag_sched_read(void)
 {
-       frr_with_mutex (&zrouter.mlag_info.mlag_th_mtx) {
-               thread_add_read(zmlag_master, zebra_mlag_read, NULL,
-                               mlag_socket, &zrouter.mlag_info.t_read);
-       }
+       thread_add_read(zmlag_master, zebra_mlag_read, NULL, mlag_socket,
+                       &zrouter.mlag_info.t_read);
 }
 
 static int zebra_mlag_read(struct thread *thread)

diff --git a/zebra/zebra_router.h b/zebra/zebra_router.h
index 6fe3e4840f3f8346a1b94be65bfbe6c0dffd1f5f..437cab5bbd242cd1ec6ab2c8ea98a560976917fe 100644 (file)
--- a/zebra/zebra_router.h
+++ b/zebra/zebra_router.h
@@ -100,7 +100,6 @@ struct zebra_mlag_info {
        /* Threads for read/write. */
        struct thread *t_read;
        struct thread *t_write;
-       pthread_mutex_t mlag_th_mtx;
 };
 
 struct zebra_router {