Hi Lennart,
I have just pushed a new version of this ticket based on Vu Minh Nguyen
comments.
It would be nice if you take your time to review the new one.
But I still consider your comments of the old version for adapting into the
new one if it is necessary.
Thank you in advance!
Best regards,
Danh Vo
-----Original Message-----
From: Lennart Lund <lennart.l...@ericsson.com>
Sent: Monday, July 30, 2018 6:36 PM
To: Danh Cong Vo <danh.c...@dektech.com.au>; Hans Nordebäck
<hans.nordeb...@ericsson.com>; Gary Lee <gary....@dektech.com.au>; Vu Minh
Nguyen <vu.m.ngu...@dektech.com.au>
Cc: opensaf-devel@lists.sourceforge.net; Danh Cong Vo
<danh.c...@dektech.com.au>; Lennart Lund <lennart.l...@ericsson.com>
Subject: RE: [PATCH 1/1] imm: syslog recent fevs evts when immnd down
[#2898]
Hi Danh Vo
Most of my comments can be found in the attached .diff file. This diff can
be applied on top of the review branch
A summary of my comments:
Do not add a feature as code distributed in several files and functions
not related to the feature. Keep the feature in in one file and create an
API.
Do not add a feature to an already existing file created for some other
purpose even if this new feature is used there.
Use C++. In this case C++ vector of strings can be used instead of a char
array. In this case C functions are needed as a part of the API but that is
not a problem to have in a file compiled with a C++ compiler
Do not add and handle global variables inline in code that has a
completely different purpose. Instead call a function with a descriptive
name
Give all new functions descriptive names. By descriptive name means a name
that tells a reader what the function does not what data it handles. See
example in the attached comments
In the file that contains all the new log handling, write a comment
describing the purpose of the new feature
Thanks
Lennart
> -----Original Message-----
> From: Danh Vo <danh.c...@dektech.com.au>
> Sent: den 27 juli 2018 06:46
> To: Hans Nordebäck <hans.nordeb...@ericsson.com>; Lennart Lund
> <lennart.l...@ericsson.com>; Gary Lee <gary....@dektech.com.au>; Vu
> Minh Nguyen <vu.m.ngu...@dektech.com.au>
> Cc: opensaf-devel@lists.sourceforge.net; Danh Cong Vo
> <danh.c...@dektech.com.au>
> Subject: [PATCH 1/1] imm: syslog recent fevs evts when immnd down
> [#2898]
>
> An enhancement for IMM to print out 5 last FEVS events into syslog
> when IMMND down for any reason.
> ---
> src/imm/common/immsv_evt.c | 2 +-
> src/imm/common/immsv_evt.h | 2 +
> src/imm/immnd/ImmModel.cc | 285
> +++++++++++++++++++++++++++++++++++++++++++++
> src/imm/immnd/ImmModel.h | 1 +
> src/imm/immnd/immnd_evt.c | 34 +++++-
> src/imm/immnd/immnd_init.h | 2 +
> 6 files changed, 322 insertions(+), 4 deletions(-)
>
> diff --git a/src/imm/common/immsv_evt.c b/src/imm/common/immsv_evt.c
> index b0e36d3..03a7f81 100644
> --- a/src/imm/common/immsv_evt.c
> +++ b/src/imm/common/immsv_evt.c
> @@ -189,7 +189,7 @@ static const char *immnd_evt_names[] = {
> "IMMND_EVT_D2ND_IMPLDELETE",
> "undefined (high)"};
>
> -static const char *immsv_get_immnd_evt_name(unsigned int id)
> +const char *immsv_get_immnd_evt_name(unsigned int id)
> {
> if (id < IMMND_EVT_MAX)
> return immnd_evt_names[id];
> diff --git a/src/imm/common/immsv_evt.h b/src/imm/common/immsv_evt.h
> index 3e96c98..a1dabc5 100644
> --- a/src/imm/common/immsv_evt.h
> +++ b/src/imm/common/immsv_evt.h
> @@ -689,6 +689,8 @@ void
> immsv_free_attrdefs_list(IMMSV_ATTR_DEF_LIST *adp); void
> immsv_evt_free_name_list(IMMSV_OBJ_NAME_LIST *p); void
> immsv_evt_free_ccbOutcomeList(IMMSV_CCB_OUTCOME_LIST *o);
>
> +const char *immsv_get_immnd_evt_name(unsigned int id);
> +
> #ifdef __cplusplus
> }
> #endif
> diff --git a/src/imm/immnd/ImmModel.cc b/src/imm/immnd/ImmModel.cc
> index 21f48ab..80fdf84 100644
> --- a/src/imm/immnd/ImmModel.cc
> +++ b/src/imm/immnd/ImmModel.cc
> @@ -35,6 +35,7 @@
> #define PRT_HIGH_THRESHOLD 4 /* See ImmModel::immNotPbeWritable
> */
> #define CCB_CRIT_THRESHOLD 8 /* See ImmModel::immNotPbeWritable
> */
> #define SEARCH_TIMEOUT_SEC 600 /* Search timeout */
> +#define MAX_LENGTH_EVT_DATA 256
>
> struct ContinuationInfo2 {
> ContinuationInfo2()
> @@ -20171,3 +20172,287 @@ void ImmModel::isolateThisNode(unsigned int
> thisNode, bool isAtCoord) {
> immnd_proc_imma_discard_connection()
> */
> }
> +
> +
> +/*********************************************************
> *************
> + * An enhancement from ticket #2898 to get more info from events
> +
> **********************************************************
> ************/
> +static std::string getInfoForObjCreate(const ImmsvOmCcbObjectCreate
> *req)
> +{
> + std::string additionalInfo = "";
> + size_t sz = strnlen(req->className.buf,
> +(size_t)req->className.size);
> + std::string className((const char*)req->className.buf, sz);
> +
> + if (className.empty()) {
> + additionalInfo.append("No info"); } else {
> + additionalInfo.append("Create obj of class ").append(className);
> + }
> +
> + return additionalInfo;
> +}
> +
> +static std::string getInfoForObjModify(const ImmsvOmCcbObjectModify*
> req)
> +{
> + std::string additionalInfo = "";
> + size_t sz = strnlen(req->objectName.buf,
> +(size_t)req->objectName.size);
> + std::string objName((const char*)req->objectName.buf, sz);
> +
> + if (objName.empty()) {
> + additionalInfo.append("No info"); } else {
> + additionalInfo.append(objName);
> + }
> +
> + return additionalInfo;
> +}
> +
> +static std::string getInfoForObjDelete(const ImmsvOmCcbObjectDelete*
> req)
> +{
> + std::string additionalInfo = "";
> + size_t sz = strnlen(req->objectName.buf,
> +(size_t)req->objectName.size);
> + std::string objName((const char*)req->objectName.buf, sz);
> +
> + if (objName.empty()) {
> + additionalInfo.append("No info"); } else {
> + additionalInfo.append(objName);
> + }
> +
> + return additionalInfo;
> +}
> +
> +static std::string getInfoForObjSync(const ImmsvOmObjectSync *req) {
> + std::string additionalInfo = "";
> + size_t sz = strnlen(req->objectName.buf,
> +(size_t)req->objectName.size);
> + std::string objName((const char*)req->objectName.buf, sz);
> +
> + if (objName.empty()) {
> + additionalInfo.append("No info"); } else {
> + additionalInfo.append(objName);
> + }
> +
> + return additionalInfo;
> +}
> +
> +static std::string getInfoForAdmOp(const
> ImmsvOmAdminOperationInvoke* req)
> +{
> + std::string additionalInfo = "";
> + size_t sz = strnlen(req->objectName.buf,
> +(size_t)req->objectName.size);
> + std::string objName((const char*)req->objectName.buf, sz);
> + std::string opId = std::to_string(req->operationId);
> +
> + if (objName.empty()) {
> + additionalInfo.append("No info"); } else {
> + additionalInfo.append(objName).append("(opId:").append(opId);
> + }
> +
> + return additionalInfo;
> +}
> +
> +static std::string getInfoForClassDesc(const ImmsvOmClassDescr *req)
> +{
> + std::string additionalInfo = "";
> + size_t sz = strnlen((char*)req->className.buf, (size_t)req-
> >className.size);
> + std::string className((const char*)req->className.buf, sz);
> +
> + if (className.empty()) {
> + additionalInfo.append("No info"); } else {
> + additionalInfo.append(className); }
> +
> + return additionalInfo;
> +}
> +
> +static std::string getInfoForAdmInit(const immsv_d2nd_adminit *req) {
> + std::string additionalInfo = "";
> + std::string ownerId = std::to_string(req->globalOwnerId);
> + std::string ownerName = osaf_extended_name_borrow(&(req-
> >i.adminOwnerName));
> +
> + if (ownerId.empty()) {
> + additionalInfo.append("No info"); } else {
> +
> additionalInfo.append(ownerId).append("(").append(ownerName).append(
> ")");
> + }
> +
> + return additionalInfo;
> +}
> +
> +static std::string getInfoForImplSet(const ImmsvOiImplSetReq *req,
> + bool isSet = false) {
> + std::string additionalInfo = "";
> + size_t sz = strnlen((const char*)req->impl_name.buf, req-
> >impl_name.size);
> + std::string clsOrObjName((const char*)req->impl_name.buf, sz);
> +
> + if (clsOrObjName.empty()) {
> + additionalInfo.append("No info"); } else {
> + if (isSet) {
> + additionalInfo.append("Set impl ")
> + .append(std::to_string(req->impl_id))
> + .append(" to ").append(clsOrObjName);
> + } else {
> + additionalInfo.append("Release impl ")
> + .append(std::to_string(req->impl_id))
> + .append(" out of ").append(clsOrObjName);
> + }
> + }
> +
> + return additionalInfo;
> +}
> +
> +static std::string getInfoForSafeReadLock(const ImmsvOmSearchInit
> +*req) {
> + std::string additionalInfo = "";
> + std::string ccbId = std::to_string(req->ccbId);
> + size_t sz = strnlen(req->rootName.buf, (size_t)req->rootName.size);
> + std::string objName((const char*)req->rootName.buf, sz);
> +
> + additionalInfo.append("CCB ").append(ccbId).append(" lock
> ").append(objName);
> + return additionalInfo;
> +}
> +
> +void immModel_getEvtData(IMMND_CB *cb, IMMND_EVT *evt, char*
> evt_data)
> +{
> + std::string additionalInfo = "";
> + std::string eventName(immsv_get_immnd_evt_name(evt->type));
> +
> + switch (evt->type) {
> + case IMMND_EVT_A2ND_OBJ_CREATE:
> + case IMMND_EVT_A2ND_OBJ_CREATE_2:
> + case IMMND_EVT_A2ND_OI_OBJ_CREATE:
> + case IMMND_EVT_A2ND_OI_OBJ_CREATE_2:
> + additionalInfo = getInfoForObjCreate(&(evt->info.objCreate));
> + break;
> +
> + case IMMND_EVT_A2ND_OBJ_MODIFY:
> + case IMMND_EVT_A2ND_OI_OBJ_MODIFY:
> + additionalInfo = getInfoForObjModify(&(evt->info.objModify));
> + break;
> +
> + case IMMND_EVT_A2ND_OBJ_DELETE:
> + case IMMND_EVT_A2ND_OI_OBJ_DELETE:
> + additionalInfo = getInfoForObjDelete(&(evt->info.objDelete));
> + break;
> +
> + case IMMND_EVT_A2ND_OBJ_SYNC:
> + case IMMND_EVT_A2ND_OBJ_SYNC_2:
> + additionalInfo = getInfoForObjSync(&(evt->info.obj_sync));
> + break;
> +
> + case IMMND_EVT_A2ND_IMM_ADMOP:
> + case IMMND_EVT_A2ND_IMM_ADMOP_ASYNC:
> + additionalInfo = getInfoForAdmOp(&(evt->info.admOpReq));
> + break;
> +
> + case IMMND_EVT_A2ND_CLASS_CREATE:
> + case IMMND_EVT_A2ND_CLASS_DELETE:
> + additionalInfo = getInfoForClassDesc(&(evt->info.classDescr));
> + break;
> +
> + case IMMND_EVT_D2ND_DISCARD_IMPL:
> + case IMMND_EVT_D2ND_IMPLSET_RSP:
> + case IMMND_EVT_D2ND_IMPLSET_RSP_2:
> + case IMMND_EVT_A2ND_OI_IMPL_CLR:
> + additionalInfo.append("impl_id:")
> + .append(std::to_string(evt->info.implSet.impl_id));
> + break;
> +
> + case IMMND_EVT_D2ND_ADMINIT:
> + additionalInfo = getInfoForAdmInit(&(evt->info.adminitGlobal));
> + break;
> +
> + case IMMND_EVT_D2ND_CCBINIT:
> + additionalInfo.append("ccbId:")
> + .append(std::to_string(evt->info.ccbinitGlobal.globalCcbId));
> + break;
> +
> + case IMMND_EVT_A2ND_AUG_ADMO:
> + additionalInfo.append("Add admo_id ")
> + .append(std::to_string(evt->info.objDelete.adminOwnerId))
> + .append(" to CCB ")
> + .append(std::to_string(evt->info.objDelete.ccbId));
> + break;
> +
> + case IMMND_EVT_A2ND_OI_CL_IMPL_SET:
> + case IMMND_EVT_A2ND_OI_OBJ_IMPL_SET:
> + additionalInfo = getInfoForImplSet(&(evt->info.implSet), true);
> + break;
> +
> + case IMMND_EVT_A2ND_OI_CL_IMPL_REL:
> + case IMMND_EVT_A2ND_OI_OBJ_IMPL_REL:
> + additionalInfo = getInfoForImplSet(&(evt->info.implSet));
> + break;
> +
> + case IMMND_EVT_A2ND_ADMO_FINALIZE:
> + case IMMND_EVT_D2ND_ADMO_HARD_FINALIZE:
> + case IMMND_EVT_A2ND_ADMO_SET:
> + case IMMND_EVT_A2ND_ADMO_RELEASE:
> + case IMMND_EVT_A2ND_ADMO_CLEAR:
> + additionalInfo.append("admo_id:")
> + .append(std::to_string(evt->info.admFinReq.adm_owner_id));
> + break;
> +
> + case IMMND_EVT_A2ND_OI_CCB_AUG_INIT:
> + case IMMND_EVT_A2ND_CCB_COMPLETED_RSP:
> + case IMMND_EVT_A2ND_CCB_COMPLETED_RSP_2:
> + case IMMND_EVT_A2ND_CCB_OBJ_CREATE_RSP:
> + case IMMND_EVT_A2ND_CCB_OBJ_CREATE_RSP_2:
> + case IMMND_EVT_A2ND_CCB_OBJ_MODIFY_RSP:
> + case IMMND_EVT_A2ND_CCB_OBJ_MODIFY_RSP_2:
> + case IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP:
> + case IMMND_EVT_A2ND_CCB_OBJ_DELETE_RSP_2:
> + additionalInfo.append("ccbId:")
> + .append(std::to_string(evt->info.ccbUpcallRsp.ccbId));
> + break;
> +
> + case IMMND_EVT_A2ND_CCB_APPLY:
> + case IMMND_EVT_A2ND_CCB_VALIDATE:
> + case IMMND_EVT_A2ND_CCB_FINALIZE:
> + case IMMND_EVT_D2ND_ABORT_CCB:
> + additionalInfo.append("ccbId:")
> + .append(std::to_string(evt->info.ccbId));
> + break;
> +
> + case IMMND_EVT_A2ND_PBE_ADMOP_RSP:
> + additionalInfo.append("admop result:")
> + .append(std::to_string(evt->info.admOpRsp.result));
> + break;
> +
> + case IMMND_EVT_D2ND_SYNC_FEVS_BASE:
> + additionalInfo.append("sync fevs base:")
> + .append(std::to_string(evt->info.syncFevsBase));
> + break;
> +
> + case IMMND_EVT_A2ND_OBJ_SAFE_READ:
> + additionalInfo = getInfoForSafeReadLock(&(evt->info.searchInit));
> + break;
> +
> + case IMMND_EVT_D2ND_IMPLDELETE:
> + additionalInfo.append("Delete ")
> + .append(std::to_string(evt->info.impl_delete.size))
> + .append(" implementer(s).");
> + break;
> +
> + case IMMND_EVT_D2ND_DISCARD_NODE:
> + additionalInfo.append("nodeId: ")
> + .append(std::to_string(evt->info.ctrl.nodeId));
> + break;
> + default:
> + additionalInfo = "";
> + break;
> + }
> +
> + if (additionalInfo.empty()) {
> + snprintf(evt_data, MAX_LENGTH_EVT_DATA, "%s", eventName.c_str());
> + } else {
> + snprintf(evt_data, MAX_LENGTH_EVT_DATA,
> + "%s -> %s", eventName.c_str(), additionalInfo.c_str());
> + }
> +}
> diff --git a/src/imm/immnd/ImmModel.h b/src/imm/immnd/ImmModel.h index
> c8b0f4e..23b3692 100644
> --- a/src/imm/immnd/ImmModel.h
> +++ b/src/imm/immnd/ImmModel.h
> @@ -60,6 +60,7 @@ struct ImmOiRtObjectCreate; struct
> immsv_oi_ccb_upcall_rsp; struct immsv_attr_values_list; struct
> immsv_attr_mods_list;
> +struct immsv_d2nd_adminit;
>
> struct ImmsvOmRspSearchNext;
>
> diff --git a/src/imm/immnd/immnd_evt.c b/src/imm/immnd/immnd_evt.c
> index 730c490..db94036 100644
> --- a/src/imm/immnd/immnd_evt.c
> +++ b/src/imm/immnd/immnd_evt.c
> @@ -39,6 +39,20 @@
> #define IMMND_SEARCH_BUNDLE_SIZE ((MDS_DIRECT_BUF_MAXSIZE /
> 100) * 90)
> #define IMMND_MAX_SEARCH_RESULT (IMMND_SEARCH_BUNDLE_SIZE /
> 300)
>
> +#define MAX_INDEX_FEVS_MSG 5
> +#define MAX_LENGTH_FEVS_MSG 256
> +
> +static char
> latest_fevs[MAX_INDEX_FEVS_MSG][MAX_LENGTH_FEVS_MSG] = {
> + "(none)", "(none)", "(none)", "(none)", "(none)"
> +};
> +
> +static void syslog_recent_fevs()
> +{
> + for (int i = 0; i < MAX_INDEX_FEVS_MSG; i++) {
> + LOG_NO("Recent fevs: %s", latest_fevs[i]);
> + }
> +}
> +
> static SaAisErrorT immnd_fevs_local_checks(IMMND_CB *cb, IMMSV_FEVS
> *fevsReq,
> const IMMSV_SEND_INFO *sinfo);
static uint32_t
> immnd_evt_proc_cb_dump(IMMND_CB *cb); @@ -9601,7 +9615,8 @@ static
> uint32_t immnd_restricted_ok(IMMND_CB *cb, uint32_t id) static
> SaAisErrorT immnd_evt_proc_fevs_dispatch(IMMND_CB *cb,
> IMMSV_OCTET_STRING *msg,
> bool originatedAtThisNd, SaImmHandleT clnt_hdl,
> - MDS_DEST reply_dest, SaUint64T msgNo)
> + MDS_DEST reply_dest, SaUint64T msgNo,
> + char* evt_data)
> {
> SaAisErrorT error = SA_AIS_OK;
> IMMSV_EVT frwrd_evt;
> @@ -9665,7 +9680,7 @@ immnd_evt_proc_fevs_dispatch(IMMND_CB *cb,
> IMMSV_OCTET_STRING *msg,
>
> /*Dispatch the unpacked FEVS message */
> immsv_msg_trace_rec(frwrd_evt.sinfo.dest, &frwrd_evt);
> -
> + immModel_getEvtData(immnd_cb, &frwrd_evt.info.immnd,
> evt_data);
> switch (frwrd_evt.info.immnd.type) {
> case IMMND_EVT_A2ND_OBJ_CREATE:
> case IMMND_EVT_A2ND_OBJ_CREATE_2:
> @@ -10735,7 +10750,11 @@ static uint32_t
> immnd_evt_proc_fevs_rcv(IMMND_CB *cb, IMMND_EVT *evt,
> bool isObjSync = (evt->type ==
> IMMND_EVT_D2ND_GLOB_FEVS_REQ_2)
> ? evt->info.fevsReq.isObjSync
> : false;
> + static int index = 0;
> + char evt_data[MAX_LENGTH_FEVS_MSG];
> +
> TRACE_ENTER();
> + memset(evt_data, 0, sizeof(evt_data));
>
> if (cb->highestProcessed >= msgNo) {
> /*We have already received this message, discard it. */ @@
-10784,6
> +10803,7 @@ static uint32_t immnd_evt_proc_fevs_rcv(IMMND_CB *cb,
> IMMND_EVT *evt,
> LOG_ER(
> "MESSAGE:%llu OUT OF ORDER my highest
processed:%llu -
> exiting",
> msgNo, cb->highestProcessed);
> + syslog_recent_fevs();
> immnd_ackToNid(NCSCC_RC_FAILURE);
> exit(1);
> } else if (cb
> @@ -10792,6 +10812,7 @@ static uint32_t
> immnd_evt_proc_fevs_rcv(IMMND_CB *cb, IMMND_EVT *evt,
> LOG_ER(
> "Sync MESSAGE:%llu OUT OF ORDER my highest
processed:%llu -
> exiting",
> msgNo, cb->highestProcessed);
> + syslog_recent_fevs();
> immnd_ackToNid(NCSCC_RC_FAILURE);
> exit(1);
> } else if (cb->mState < IMM_SERVER_LOADING_PENDING) { @@
-10816,7
> +10837,8 @@ static uint32_t immnd_evt_proc_fevs_rcv(IMMND_CB *cb,
> IMMND_EVT *evt,
> TRACE("Coord discards object sync message");
> } else {
> err = immnd_evt_proc_fevs_dispatch(cb, msg,
originatedAtThisNd,
> - clnt_hdl, reply_dest,
> msgNo);
> + clnt_hdl, reply_dest,
> + msgNo, evt_data);
> }
>
> if (err != SA_AIS_OK) {
> @@ -10826,7 +10848,11 @@ static uint32_t
> immnd_evt_proc_fevs_rcv(IMMND_CB *cb, IMMND_EVT *evt,
> LOG_ER("PROBLEM %u WITH msg no:%llu", err, msgNo);
> }
> }
> + memset(latest_fevs[index], 0, MAX_LENGTH_FEVS_MSG);
> + snprintf(latest_fevs[index], MAX_LENGTH_FEVS_MSG,
> + "<%llu>[%s]", msgNo, evt_data);
>
> + if (++index > MAX_INDEX_FEVS_MSG - 1) index = 0;
> done:
> cb->highestProcessed++;
> dequeue_outgoing(cb);
> @@ -10912,6 +10938,8 @@ static void
> immnd_evt_proc_discard_node(IMMND_CB *cb, IMMND_EVT *evt,
> cb->node_id);
> exit(1);
> }
> +
> + syslog_recent_fevs();
> LOG_NO("Global discard node received for nodeId:%x pid:%u",
> evt->info.ctrl.nodeId, evt->info.ctrl.ndExecPid);
> /* We should remember the nodeId/pid pair to avoid a redundant
> message diff --git a/src/imm/immnd/immnd_init.h
> b/src/imm/immnd/immnd_init.h index 1d0126e..b626df9 100644
> --- a/src/imm/immnd/immnd_init.h
> +++ b/src/imm/immnd/immnd_init.h
> @@ -485,6 +485,8 @@ void immModel_sendSyncAbortAt(IMMND_CB *cb, struct
> timespec time);
>
> void immModel_getSyncAbortRsp(IMMND_CB *cb);
>
> +void immModel_getEvtData(IMMND_CB *cb, IMMND_EVT *evt, char*
> evt_data);
> +
> bool is_regular_name(const char* name, bool strict); bool
> is_valid_schema_name(const char* name);
>
> --
> 2.7.4
------------------------------------------------------------------------------
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
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
