Hi Mahesh, Ack with comments, [Vu].
Regards, Vu > -----Original Message----- > From: [email protected] [mailto:[email protected]] > Sent: Friday, February 24, 2017 10:34 AM > To: [email protected]; [email protected]; > [email protected] > Cc: [email protected] > Subject: [PATCH 1 of 1] logd: replace client DB patricia tree with cpp Map > [#1984] V1 > > src/log/README | 4 +- > src/log/README_LONGDN | 15 +--- > src/log/logd/lgs_cb.h | 2 - > src/log/logd/lgs_clm.cc | 25 +++----- > src/log/logd/lgs_evt.cc | 123 ++++++++++++++++++++++++++++++---------- > ----- > src/log/logd/lgs_evt.h | 7 ++ > src/log/logd/lgs_mbcsv.cc | 12 +-- > src/log/logd/lgs_util.cc | 30 ++++------ > 8 files changed, 124 insertions(+), 94 deletions(-) > > > Replaced client DB NCS PATRICIA TREE with C++ Map > for improve efficiency. > > diff --git a/src/log/README b/src/log/README > --- a/src/log/README > +++ b/src/log/README > @@ -66,10 +66,10 @@ lgs uses the following data structures: > pointer variable. > > - Stream descriptors. Dynamically created, many instances. Accessed using > -stream name or ID. Stored in patricia tree and array. > +stream name or ID. Stored in Map and array. > > - Client descriptors. Dynamically created, many instances. Accessed using a > -client ID. Stored in patricia tree. Contains the MDS address of the client. > +client ID. Stored in Map. Contains the MDS address of the client. > Also contains a list of Client-Stream association objects, see below. > > - Client-Stream association object. Dynamically created, many instances. > One > diff --git a/src/log/README_LONGDN b/src/log/README_LONGDN > --- a/src/log/README_LONGDN > +++ b/src/log/README_LONGDN > @@ -73,16 +73,12 @@ done. Refer to chapter 4 of OpenSAF_Exte > 3) LOG Service > ---------------------------------------- > > -In original code, logsv used NCS PATRICIA TREE as an database to hold all > +Logsv used Maps as an database to hold all > existing LOG stream names, and taking stream name as the keyword for > -searching/adding/deleting tree elements. > +searching/adding/deleting elements. > > -NCS PATRICIA TREE has a limitation with keyword size. It is only supported > the > -key size up to 600 bytes. With long stream name, its length could be up to > -2024(kOsafMaxDnLength) bytes. The existing tree is not applicable for > holing > -long stream names. > > -Beside that tree, logsv is providing other database named `stream_array` > which > +Beside that map, logsv is providing other database named `stream_array` > which > is used to hold all LOG existing streams. When there is any new stream > created, > a new `log_stream_t` object is created. It holds information of that stream. > It later is added to that database. And when the stream is closed and no > client > @@ -96,11 +92,10 @@ That database is a two-dimensional array > } > > Logsv is able to query below infos from `stream_array` that logsv used to do > -based on NCS PATRICIA TREE: > +based on Maps: > - Is there any current log stream with specific DN in system? > - Or what are the current log existing streams in system? > -`stream_array` database can play the same role as NCS_PATRICIA_TREE. > Therefore, > -NCS PATRICIA TREE is removed from logsv code. > +`stream_array` database can play the same role as Maps. > > More on `stream_array` database. All rooms in `stream_array` is clean/reset > at > startup. First three rooms are reserved for well-known streams. > diff --git a/src/log/logd/lgs_cb.h b/src/log/logd/lgs_cb.h > --- a/src/log/logd/lgs_cb.h > +++ b/src/log/logd/lgs_cb.h > @@ -57,7 +57,6 @@ typedef struct lgs_stream_list { > } lgs_stream_list_t; > > typedef struct { > - NCS_PATRICIA_NODE pat_node; > uint32_t client_id; > uint32_t client_id_net; > SaVersionT client_ver; > @@ -75,7 +74,6 @@ typedef struct lgs_cb { > V_DEST_RL mds_role; /* Current MDS role - ACTIVE/STANDBY */ > MDS_DEST vaddr; /* My identification in MDS */ > SaVersionT log_version; /* The version currently supported */ > - NCS_PATRICIA_TREE client_tree; /* LGA/Library/Client instantiation pat. > tree */ > SaNameT comp_name; /* Components's name LGS */ > SaAmfHandleT amf_hdl; /* AMF handle, obtained thru AMF init */ > SaSelectionObjectT amfSelectionObject; /* Selection Object to wait for > AMF events */ > diff --git a/src/log/logd/lgs_clm.cc b/src/log/logd/lgs_clm.cc > --- a/src/log/logd/lgs_clm.cc > +++ b/src/log/logd/lgs_clm.cc > @@ -17,6 +17,7 @@ > #include <cinttypes> > > #include "log/logd/lgs.h" > +#include "log/logd/lgs_evt.h" > #include "log/logd/lgs_clm.h" > #include "base/time.h" > > @@ -209,26 +210,20 @@ static uint32_t send_clm_node_status_lib > static uint32_t send_cluster_membership_msg_to_clients( > SaClmClusterChangesT clusterChange, NODE_ID clm_node_id) { > uint32_t rc = NCSCC_RC_SUCCESS; > - log_client_t *rp = NULL; > - uint32_t client_id_net; > + log_client_t *rec = NULL; [Vu] For added code, consider using "nullptr" instead of NULL. > > TRACE_ENTER(); > TRACE_3("clm_node_id: %x, change:%u", clm_node_id, clusterChange); > - > - rp = reinterpret_cast<log_client_t *> > - (ncs_patricia_tree_getnext(&lgs_cb->client_tree, NULL)); > - > - while (rp != NULL) { > - /** Store the client_id_net for get Next */ > - client_id_net = rp->client_id_net; > - NODE_ID tmp_clm_node_id = m_LGS_GET_NODE_ID_FROM_ADEST(rp- > >mds_dest); > + /* Loop through Client DB */ > + ClientMap *clientMap(reinterpret_cast<ClientMap *>(client_db)); [Vu] Any reason why not using "ClientMap* client_db" to avoid typecast? > + ClientMap::iterator pos; > + for (pos = clientMap->begin(); pos != clientMap->end(); pos++) { [Vu] Consider replacing above 02 lines by using "for (const auto& pos : *clientMap) { > + rec = pos->second; > + NODE_ID tmp_clm_node_id = m_LGS_GET_NODE_ID_FROM_ADEST(rec- > >mds_dest); > // Do not send to A11 client. Send only to specific Node > if (tmp_clm_node_id == clm_node_id) > - rc = send_clm_node_status_lib(clusterChange, rp->client_id, > - rp->mds_dest); > - > - rp = reinterpret_cast<log_client_t *>(ncs_patricia_tree_getnext( > - &lgs_cb->client_tree, reinterpret_cast<uint8_t *>(&client_id_net))); > + rc = send_clm_node_status_lib(clusterChange, rec->client_id, > + rec->mds_dest); > } > > TRACE_LEAVE(); > diff --git a/src/log/logd/lgs_evt.cc b/src/log/logd/lgs_evt.cc > --- a/src/log/logd/lgs_evt.cc > +++ b/src/log/logd/lgs_evt.cc > @@ -30,6 +30,8 @@ > #include "base/osaf_extended_name.h" > #include "lgs_clm.h" > > +void *client_db = NULL; /* used for C++ STL map */ [Vu] Using nullptr instead > + > static uint32_t process_api_evt(lgsv_lgs_evt_t *evt); > static uint32_t proc_lga_updn_mds_msg(lgsv_lgs_evt_t *evt); > static uint32_t proc_mds_quiesced_ack_msg(lgsv_lgs_evt_t *evt); > @@ -69,19 +71,47 @@ static bool is_log_version_valid(const S > } > > /** > + * Name : lgs_client_map_init > + * Description : This routine is used to initialize the client_map > + * Arguments : lgs_cb - pointer to the lgs Control Block > + * Return Values : NCSCC_RC_SUCCESS/NCSCC_RC_FAILURE > + * Notes : None > + */ > +uint32_t lgs_client_map_init(lgs_cb_t *lgs_cb) { [Vu] lgs_cb is not referred to anywhere in the function. Consider using non parameter. > + TRACE_ENTER(); > + if (client_db) { > + TRACE("Client Map already exists"); > + return NCSCC_RC_FAILURE; > + } > + /*Client DB */ > + client_db = new ClientMap; [Vu] Add assertion to make sure client_db is not nullptr. > + TRACE_LEAVE(); > + return NCSCC_RC_SUCCESS; > +} > + > +/** > * Get client record from client ID > * @param client_id > * > * @return log_client_t* > */ > log_client_t *lgs_client_get_by_id(uint32_t client_id) { > - uint32_t client_id_net; > - log_client_t *rec; > + log_client_t *rec = NULL; [Vu] Using nullptr > > - client_id_net = m_NCS_OS_HTONL(client_id); > - rec = reinterpret_cast<log_client_t *>( > - ncs_patricia_tree_get(&lgs_cb->client_tree, > - reinterpret_cast<uint8_t *>(&client_id_net))); > + /* Client DB */ > + ClientMap *clientMap(reinterpret_cast<ClientMap *> > + (client_db)); > + if (clientMap) { > + auto it = (clientMap->find(client_id)); > + > + if (it != clientMap->end()) { > + rec = it->second; > + } else { > + TRACE("clm_node_id delete failed : %x", client_id); > + } > + } else { > + TRACE("clm_node_id delete to map not exist failed : %x", client_id); > + } > if (NULL == rec) > TRACE("client_id: %u lookup failed", client_id); > > @@ -100,14 +130,16 @@ log_client_t *lgs_client_new(MDS_DEST md > log_client_t *client; > > TRACE_ENTER2("MDS dest %" PRIx64, mds_dest); > - > + /* Client DB */ > + ClientMap *clientMap(reinterpret_cast<ClientMap *> > + (client_db)); > if (client_id == 0) { > lgs_cb->last_client_id++; > if (lgs_cb->last_client_id == 0) > lgs_cb->last_client_id++; > } > > - client = static_cast<log_client_t *>(calloc(1, sizeof(log_client_t))); > + client = new log_client_t(); > > if (NULL == client) { > LOG_WA("lgs_client_new calloc FAILED"); > @@ -119,18 +151,22 @@ log_client_t *lgs_client_new(MDS_DEST md > lgs_cb->last_client_id = client_id; > client->client_id = lgs_cb->last_client_id; > client->mds_dest = mds_dest; > - client->client_id_net = m_NCS_OS_HTONL(client->client_id); > - client->pat_node.key_info = (uint8_t *)&client->client_id_net; > client->stream_list_root = stream_list; > - > - /** Insert the record into the patricia tree **/ > - if (NULL == ncs_patricia_tree_get(&lgs_cb->client_tree,client- > >pat_node.key_info)){ > - if (NCSCC_RC_SUCCESS != ncs_patricia_tree_add(&lgs_cb->client_tree, > &client->pat_node)) { > - LOG_WA("FAILED: ncs_patricia_tree_add, client_id %u", client_id); > + > + if (clientMap) { > + std::pair<ClientMap::iterator, bool> p(clientMap->insert( > + std::make_pair(client->client_id, client))); > + > + if (!p.second) { > + TRACE("unable to add clm node info map - the id %x already existed", > + client->client_id); > free(client); > client = NULL; > - goto done; > } > + } else { > + TRACE("can't find local sec map in lgs_clm_node_add"); > + free(client); > + client = NULL; > } > > done: > @@ -152,9 +188,12 @@ int lgs_client_delete(uint32_t client_id > lgs_stream_list_t *cur_rec; > time_t closetime = 0; > struct timespec closetime_tspec; > + log_client_t *client_rec; > > TRACE_ENTER2("client_id %u", client_id); > - > + /* Client DB */ > + ClientMap *clientMap(reinterpret_cast<ClientMap *> > + (client_db)); > /* Initiate close time value if not provided via closetime_ptr */ > if (closetime_ptr == NULL) { > osaf_clock_gettime(CLOCK_REALTIME, &closetime_tspec); > @@ -182,15 +221,22 @@ int lgs_client_delete(uint32_t client_id > cur_rec = tmp_rec; > } > > - if (ncs_patricia_tree_get(&lgs_cb->client_tree,client- > >pat_node.key_info)){ > - if (NCSCC_RC_SUCCESS != ncs_patricia_tree_del(&lgs_cb->client_tree, > &client->pat_node)) { > - LOG_WA("ncs_patricia_tree_del FAILED,client_id %u",client_id); > - status = -2; > - goto done; > + if (clientMap) { > + auto it = (clientMap->find(client_id)); > + > + if (it != clientMap->end()) { > + client_rec = it->second; > + clientMap->erase(it); > + delete client_rec; > + } else { > + TRACE("clm_node_id delete failed : %x", client_id); > + status = -2; > } > + } else { > + TRACE("clm_node_id delete to map not exist failed : %x", client_id); > + status = -2; > + } > > - free(client); > - } > done: > TRACE_LEAVE(); > return status; > @@ -289,19 +335,17 @@ done: > int lgs_client_delete_by_mds_dest(MDS_DEST mds_dest, time_t > *closetime_ptr) { > uint32_t rc = 0; > log_client_t *rp = NULL; > - uint32_t client_id_net; > > TRACE_ENTER2("mds_dest %" PRIx64, mds_dest); > - rp = reinterpret_cast<log_client_t *>(ncs_patricia_tree_getnext(&lgs_cb- > >client_tree, NULL)); > + /* Loop through Client DB */ > + ClientMap *clientMap(reinterpret_cast<ClientMap *> > + (client_db)); > + ClientMap::iterator pos; > + for (pos = clientMap->begin(); pos != clientMap->end(); pos++) { [Vu] Consider using const auto& pos : *clientMap > + rp = pos->second; > > - while (rp != NULL) { > - /** Store the client_id_net for get Next */ > - client_id_net = rp->client_id_net; > if (m_NCS_MDS_DEST_EQUAL(&rp->mds_dest, &mds_dest)) > rc = lgs_client_delete(rp->client_id, closetime_ptr); > - > - rp = reinterpret_cast<log_client_t *>(ncs_patricia_tree_getnext( > - &lgs_cb->client_tree, reinterpret_cast<uint8_t *>(&client_id_net))); > } > > TRACE_LEAVE(); > @@ -564,7 +608,7 @@ static uint32_t proc_rda_cb_msg(lgsv_lgs > * Name : lgs_cb_init > * > * Description : This function initializes the LGS_CB including the > - * Patricia trees. > + * Maps. > * > * > * Arguments : lgs_cb * - Pointer to the LGS_CB. > @@ -574,15 +618,10 @@ static uint32_t proc_rda_cb_msg(lgsv_lgs > * Notes : None. > > ************************************************************** > ***************/ > uint32_t lgs_cb_init(lgs_cb_t *lgs_cb) { > - NCS_PATRICIA_PARAMS reg_param; > uint32_t rc = NCSCC_RC_SUCCESS; > > TRACE_ENTER(); > > - memset(®_param, 0, sizeof(NCS_PATRICIA_PARAMS)); > - > - reg_param.key_size = sizeof(uint32_t); > - > lgs_cb->fully_initialized = false; > lgs_cb->amfSelectionObject = -1; > lgs_cb->immSelectionObject = -1; > @@ -600,9 +639,11 @@ uint32_t lgs_cb_init(lgs_cb_t *lgs_cb) { > goto done; > } > > - /* Initialize patricia tree for reg list */ > - if (NCSCC_RC_SUCCESS != ncs_patricia_tree_init(&lgs_cb->client_tree, > ®_param)) > - return NCSCC_RC_FAILURE; > + /* Initialize CLM Node map*/ > + if (NCSCC_RC_SUCCESS != lgs_client_map_init(lgs_cb)) { > + LOG_ER("LGS: CLM Client map_init FAILED"); > + rc = NCSCC_RC_FAILURE; > + } > > /* Initialize CLM Node map*/ > if (NCSCC_RC_SUCCESS != lgs_clm_node_map_init(lgs_cb)) { > diff --git a/src/log/logd/lgs_evt.h b/src/log/logd/lgs_evt.h > --- a/src/log/logd/lgs_evt.h > +++ b/src/log/logd/lgs_evt.h > @@ -18,6 +18,8 @@ > #ifndef LOG_LOGD_LGS_EVT_H_ > #define LOG_LOGD_LGS_EVT_H_ > > +#include <map> > +#include <utility> > #include "rde/agent/rda_papi.h" > #include "mds/mds_papi.h" > #include "log/common/lgsv_msg.h" > @@ -62,6 +64,10 @@ typedef struct lgsv_lgs_evt { > } info; > } lgsv_lgs_evt_t; > > +/* Client DB */ > +extern void *client_db; /* used for C++ STL map */ [Vu] Any reason not using ClientMap* client_db? > +typedef std::map<NODE_ID, log_client_t *> ClientMap; > + > /* These are the function prototypes for event handling */ > typedef uint32_t (*LGSV_LGS_LGA_API_MSG_HANDLER) (lgs_cb_t *, > lgsv_lgs_evt_t *evt); > typedef uint32_t (*LGSV_LGS_EVT_HANDLER) (lgsv_lgs_evt_t *evt); > @@ -80,4 +86,5 @@ extern void lgs_free_write_log(const lgs > > SaAisErrorT create_new_app_stream(lgsv_stream_open_req_t > *open_sync_param, log_stream_t **o_stream); > > +uint32_t lgs_client_map_init(lgs_cb_t *lgs_cb); > #endif // LOG_LOGD_LGS_EVT_H_ > diff --git a/src/log/logd/lgs_mbcsv.cc b/src/log/logd/lgs_mbcsv.cc > --- a/src/log/logd/lgs_mbcsv.cc > +++ b/src/log/logd/lgs_mbcsv.cc > @@ -705,11 +705,12 @@ static uint32_t edu_enc_reg_list(lgs_cb_ > return (rc = EDU_ERR_MEM_FAIL); > } > ncs_enc_claim_space(uba, sizeof(lgsv_ckpt_header_t)); > + /* Loop through Client DB */ > + ClientMap *clientMap(reinterpret_cast<ClientMap *>(client_db)); > + ClientMap::iterator pos; > + for (pos = clientMap->begin(); pos != clientMap->end(); pos++) { [Vu] Consider using for (const auto& pos : *clientMap) > + client = pos->second; > > - client = reinterpret_cast<log_client_t *>(ncs_patricia_tree_getnext(&cb- > >client_tree, NULL)); > - > - /* Walk through the reg list and encode record by record */ > - while (client != NULL) { > if (lgs_is_peer_v6()) { > ckpt_reg_rec_v6.client_id = client->client_id; > ckpt_reg_rec_v6.mds_dest = client->mds_dest; > @@ -734,9 +735,6 @@ static uint32_t edu_enc_reg_list(lgs_cb_ > } > ++num_rec; > > - /* length+=lgs_edp_ed_reg_rec(reg_rec,o_ub); */ > - client = reinterpret_cast<log_client_t *>(ncs_patricia_tree_getnext(&cb- > >client_tree, > - reinterpret_cast<uint8_t *>(&client- > >client_id_net))); > } /* End while RegRec */ > > /* Encode RegHeader */ > diff --git a/src/log/logd/lgs_util.cc b/src/log/logd/lgs_util.cc > --- a/src/log/logd/lgs_util.cc > +++ b/src/log/logd/lgs_util.cc > @@ -347,7 +347,7 @@ void lgs_exit(const char *msg, SaAmfReco > * > * lgs_lga_entry_valid > * > - * Searches the cb->client_tree for an reg_id entry whos MDS_DEST equals > + * Searches the ClientMap an reg_id entry whos MDS_DEST equals > * that passed DEST and returns true if itz found. > * > * This routine is typically used to find the validity of the lga down rec from > standby > @@ -356,17 +356,15 @@ void lgs_exit(const char *msg, SaAmfReco > > ************************************************************** > **************/ > bool lgs_lga_entry_valid(lgs_cb_t *cb, MDS_DEST mds_dest) { > log_client_t *rp = NULL; > - > - rp = reinterpret_cast<log_client_t *>(ncs_patricia_tree_getnext(&cb- > >client_tree, NULL)); > - > - while (rp != NULL) { > + /* Loop through Client DB */ > + ClientMap *clientMap(reinterpret_cast<ClientMap *> > + (client_db)); > + ClientMap::iterator pos; > + for (pos = clientMap->begin(); pos != clientMap->end(); pos++) { [Vu] Consider using for (const auto& pos : *clientMap) > + rp = pos->second; > if (m_NCS_MDS_DEST_EQUAL(&rp->mds_dest, &mds_dest)) { > return true; > } > - > - rp = reinterpret_cast<log_client_t *>( > - ncs_patricia_tree_getnext(&cb->client_tree, > - reinterpret_cast<uint8_t *>(&rp->client_id_net))); > } > > return false; > @@ -902,17 +900,17 @@ static void lgs_send_filter_msg(uint32_t > void lgs_send_severity_filter_to_clients(uint32_t stream_id, > SaLogSeverityFlagsT severity_filter) { > log_client_t *rp = NULL; > - uint32_t client_id_net; > lgs_stream_list_t *stream; > > TRACE_ENTER(); > TRACE_3("stream_id: %u, severity filter:%u", stream_id, severity_filter); > > - rp = reinterpret_cast<log_client_t *> > - (ncs_patricia_tree_getnext(&lgs_cb->client_tree, NULL)); > - while (rp != NULL) { > - /* Store the client_id_net for getting next */ > - client_id_net = rp->client_id_net; > + /* Loop through Client DB */ > + ClientMap *clientMap(reinterpret_cast<ClientMap *> > + (client_db)); > + ClientMap::iterator pos; > + for (pos = clientMap->begin(); pos != clientMap->end(); pos++) { [Vu] Consider using for (const auto& pos : *clientMap) > + rp = pos->second; > /* Do not send to all client. Send to clients that need filter > callback and associate with this stream */ > stream = rp->stream_list_root; > @@ -925,8 +923,6 @@ void lgs_send_severity_filter_to_clients > stream = stream->next; > } > } > - rp = reinterpret_cast<log_client_t *>(ncs_patricia_tree_getnext( > - &lgs_cb->client_tree, reinterpret_cast<uint8_t *>(&client_id_net))); > } > > TRACE_LEAVE(); ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
