Hi Lennart, See my response inline, started with [Vu2]
Regards, Vu > -----Original Message----- > From: Lennart Lund [mailto:lennart.l...@ericsson.com] > Sent: Wednesday, February 28, 2018 8:20 PM > To: Vu Minh Nguyen <vu.m.ngu...@dektech.com.au>; Canh Van Truong > <canh.v.tru...@dektech.com.au>; Lennart Lund <lennart.l...@ericsson.com> > Cc: opensaf-devel@lists.sourceforge.net > Subject: RE: [PATCH 1/1] log: Fix cyclic crash when starting standby and OI is > not active [#2711] > > > > > -----Original Message----- > > From: Vu Minh Nguyen [mailto:vu.m.ngu...@dektech.com.au] > > Sent: den 28 februari 2018 08:10 > > To: Lennart Lund <lennart.l...@ericsson.com>; Canh Van Truong > > <canh.v.tru...@dektech.com.au> > > Cc: opensaf-devel@lists.sourceforge.net > > Subject: RE: [PATCH 1/1] log: Fix cyclic crash when starting standby and OI is > > not active [#2711] > > > > Hi Lennart, > > > > See my comments inline, with [Vu]. > > > > Regards, Vu > > > > > -----Original Message----- > > > From: Lennart Lund [mailto:lennart.l...@ericsson.com] > > > Sent: Monday, February 26, 2018 8:40 PM > > > To: vu.m.ngu...@dektech.com.au; canh.v.tru...@dektech.com.au > > > Cc: opensaf-devel@lists.sourceforge.net; Lennart Lund > > > <lennart.l...@ericsson.com> > > > Subject: [PATCH 1/1] log: Fix cyclic crash when starting standby and OI is > > not > > > active [#2711] > > > > > > Fix cyclic reboot caused by reading an IMM RT object when the OI is down > > > --- > > > src/log/apitest/logtest.c | 2 -- > > > src/log/logd/lgs_config.cc | 2 +- > > > src/log/logd/lgs_evt.cc | 13 ++++++------- > > > src/log/logd/lgs_fmt.h | 3 ++- > > > src/log/logd/lgs_imm.cc | 13 +++++++------ > > > src/log/logd/lgs_mbcsv.cc | 18 ++++-------------- > > > src/log/logd/lgs_mbcsv.h | 3 ++- > > > src/log/logd/lgs_recov.cc | 6 ++++-- > > > src/log/logd/lgs_stream.cc | 28 +++++++++++----------------- > > > src/log/logd/lgs_stream.h | 2 +- > > > src/osaf/immutil/immutil.h | 4 +++- > > > 11 files changed, 41 insertions(+), 53 deletions(-) > > > > > > diff --git a/src/log/apitest/logtest.c b/src/log/apitest/logtest.c > > > index afa1fcf57..f8fe135cb 100644 > > > --- a/src/log/apitest/logtest.c > > > +++ b/src/log/apitest/logtest.c > > > @@ -33,8 +33,6 @@ > > > #include "base/osaf_extended_name.h" > > > #include <saAis.h> > > > > > > -#define LLDTEST > > > - > > > SaNameT systemStreamName; > > > SaNameT alarmStreamName; > > > SaNameT globalConfig; > > > diff --git a/src/log/logd/lgs_config.cc b/src/log/logd/lgs_config.cc > > > index a70a2f6c6..4190e3048 100644 > > > --- a/src/log/logd/lgs_config.cc > > > +++ b/src/log/logd/lgs_config.cc > > > @@ -586,7 +586,7 @@ int lgs_cfg_verify_log_file_format(const char > > > *log_file_format) { > > > SaBoolT dummy; > > > > > > if (!lgs_is_valid_format_expression((const SaStringT)log_file_format, > > > - STREAM_TYPE_APPLICATION, &dummy)) { > > > + STREAM_TYPE_APPLICATION_RT, > > &dummy)) { > > > LOG_NO("logStreamFileFormat has invalid value = %s", > > log_file_format); > > > rc = -1; > > > } > > > diff --git a/src/log/logd/lgs_evt.cc b/src/log/logd/lgs_evt.cc > > > index b8840b436..4b735875d 100644 > > > --- a/src/log/logd/lgs_evt.cc > > > +++ b/src/log/logd/lgs_evt.cc > > > @@ -877,7 +877,7 @@ SaAisErrorT > > > create_new_app_stream(lgsv_stream_open_req_t *open_sync_param, > > > > > > /* Check the format string */ > > > if (!lgs_is_valid_format_expression(open_sync_param->logFileFmt, > > > - STREAM_TYPE_APPLICATION, > > > + STREAM_TYPE_APPLICATION_RT, > > > &twelveHourModeFlag)) { > > > TRACE("format expression failure"); > > > rc = SA_AIS_ERR_INVALID_PARAM; > > > @@ -947,16 +947,15 @@ SaAisErrorT > > > create_new_app_stream(lgsv_stream_open_req_t *open_sync_param, > > > open_sync_param->logFileName, open_sync_param- > > >logFilePathName, > > > open_sync_param->maxLogFileSize, open_sync_param- > > > >maxLogRecordSize, > > > open_sync_param->logFileFullAction, open_sync_param- > > > >maxFilesRotated, > > > - open_sync_param->logFileFmt, STREAM_TYPE_APPLICATION, > > > twelveHourModeFlag, > > > - 0, > > > - *o_stream); // output > > > + open_sync_param->logFileFmt, STREAM_TYPE_APPLICATION_RT, > > > + twelveHourModeFlag, 0, *o_stream); // output > > > if (err == -1) { > > > log_stream_delete(o_stream); > > > rc = SA_AIS_ERR_NO_MEMORY; > > > goto done; > > > } > > > > > > - rc = lgs_create_rt_appstream(*o_stream); > > > + rc = lgs_create_appstream_rt_object(*o_stream); > > > if (rc != SA_AIS_OK) log_stream_delete(o_stream); > > > > > > done: > > > @@ -1055,7 +1054,7 @@ static uint32_t proc_stream_open_msg(lgs_cb_t > > > *cb, lgsv_lgs_evt_t *evt) { > > > logStream = log_stream_get_by_name(name); > > > if (logStream != nullptr) { > > > TRACE("existing stream - id %u", logStream->streamId); > > > - if (logStream->streamType == STREAM_TYPE_APPLICATION) { > > > + if (logStream->streamType == STREAM_TYPE_APPLICATION_RT) { > > > /* Verify the creation attributes for an existing appl. stream */ > > > if (open_sync_param->lstr_open_flags & SA_LOG_STREAM_CREATE) { > > > ais_rv = file_attribute_cmp(open_sync_param, logStream); > > > @@ -1218,7 +1217,7 @@ static uint32_t proc_stream_close_msg(lgs_cb_t > > > *cb, lgsv_lgs_evt_t *evt) { > > > > > > // This check is to avoid the client getting SA_AIS_BAD_OPERATION > > > // as there is no IMM OI implementer set. > > > - if ((stream->streamType == STREAM_TYPE_APPLICATION) && > > > + if ((stream->streamType == STREAM_TYPE_APPLICATION_RT) && > > > (cb->immOiHandle == 0)) { > > > TRACE("IMM service unavailable, close stream failed"); > > > ais_rc = SA_AIS_ERR_TRY_AGAIN; > > > diff --git a/src/log/logd/lgs_fmt.h b/src/log/logd/lgs_fmt.h > > > index f493e2c48..6a52d2986 100644 > > > --- a/src/log/logd/lgs_fmt.h > > > +++ b/src/log/logd/lgs_fmt.h > > > @@ -172,7 +172,8 @@ typedef enum { > > > STREAM_TYPE_ALARM = 0, > > > STREAM_TYPE_NOTIFICATION = 1, > > > STREAM_TYPE_SYSTEM = 2, > > > - STREAM_TYPE_APPLICATION = 3 > > > + STREAM_TYPE_APPLICATION_RT = 3, > > > + STREAM_TYPE_APPLICATION_CFG = 4 > > > } logStreamTypeT; > > > > > > extern SaBoolT lgs_is_valid_format_expression(const SaStringT, > > > logStreamTypeT, > > > diff --git a/src/log/logd/lgs_imm.cc b/src/log/logd/lgs_imm.cc > > > index 314568133..60870fa17 100644 > > > --- a/src/log/logd/lgs_imm.cc > > > +++ b/src/log/logd/lgs_imm.cc > > > @@ -399,7 +399,7 @@ static void adminOperationCallback( > > > > > > if (opId == SA_LOG_ADMIN_CHANGE_FILTER) { > > > /* Only allowed to update runtime objects (application streams) */ > > > - if (stream->streamType != STREAM_TYPE_APPLICATION) { > > > + if (stream->streamType != STREAM_TYPE_APPLICATION_RT) { > > > report_om_error(immOiHandle, invocation, > > > "Admin op change filter for non app stream"); > > > goto done; > > > @@ -1644,7 +1644,8 @@ static SaAisErrorT check_attr_validity( > > > SaBoolT dummy; > > > if (opdata->operationType == CCBUTIL_CREATE) { > > > if (!lgs_is_valid_format_expression(i_logFileFormat, > > > - STREAM_TYPE_APPLICATION, > > &dummy)) { > > > + STREAM_TYPE_APPLICATION_RT, > > > + &dummy)) { > > > report_oi_error(immOiHandle, opdata->ccbId, > > > "Invalid logFileFormat: %s", i_logFileFormat); > > > rc = SA_AIS_ERR_BAD_OPERATION; > > > @@ -2276,7 +2277,7 @@ static SaAisErrorT > > stream_create_and_configure1( > > > i = 0; > > > > > > // a configurable application stream. > > > - (*stream)->streamType = STREAM_TYPE_APPLICATION; > > > + (*stream)->streamType = STREAM_TYPE_APPLICATION_CFG; > > > > > > while (ccb->param.create.attrValues[i] != NULL) { > > > if (ccb->param.create.attrValues[i]->attrValuesNumber > 0) { > > > @@ -2775,7 +2776,7 @@ static SaAisErrorT > stream_create_and_configure( > > > else if (dn == SA_LOG_STREAM_SYSTEM) > > > stream->streamType = STREAM_TYPE_SYSTEM; > > > else > > > - stream->streamType = STREAM_TYPE_APPLICATION; > > > + stream->streamType = STREAM_TYPE_APPLICATION_CFG; > > > > > > while ((attribute = attributes[i++]) != NULL) { > > > void *value; > > > @@ -2815,7 +2816,7 @@ static SaAisErrorT > stream_create_and_configure( > > > LOG_WA("Invalid logFileFormat for stream %s, using default", > > > stream->name.c_str()); > > > > > > - if (stream->streamType == STREAM_TYPE_APPLICATION) { > > > + if (stream->streamType == STREAM_TYPE_APPLICATION_CFG) { > > > logFileFormat = const_cast<char *>(static_cast<const char *>( > > > lgs_cfg_get(LGS_IMM_LOG_STREAM_FILE_FORMAT))); > > > } else { > > > @@ -2864,7 +2865,7 @@ static SaAisErrorT > stream_create_and_configure( > > > } > > > > > > if (stream->logFileFormat == NULL) { > > > - if (stream->streamType == STREAM_TYPE_APPLICATION) { > > > + if (stream->streamType == STREAM_TYPE_APPLICATION_CFG) { > > > const char *logFileFormat = static_cast<const char *>( > > > lgs_cfg_get(LGS_IMM_LOG_STREAM_FILE_FORMAT)); > > > stream->logFileFormat = strdup(logFileFormat); > > > diff --git a/src/log/logd/lgs_mbcsv.cc b/src/log/logd/lgs_mbcsv.cc > > > index 175858b6f..4250f7fc6 100644 > > > --- a/src/log/logd/lgs_mbcsv.cc > > > +++ b/src/log/logd/lgs_mbcsv.cc > > > @@ -2037,7 +2037,6 @@ uint32_t ckpt_proc_open_stream(lgs_cb_t *cb, > > void > > > *data) { > > > lgs_ckpt_stream_open_t *param; > > > log_stream_t *stream; > > > int pos = 0, err = 0; > > > - SaNameT objectName; > > > > > > TRACE_ENTER(); > > > > > > @@ -2072,7 +2071,6 @@ uint32_t ckpt_proc_open_stream(lgs_cb_t *cb, > > void > > > *data) { > > > } else { > > > TRACE("\tNew stream %s, id %u", param->logStreamName, param- > > > >streamId); > > > > > > - SaAisErrorT rc = SA_AIS_OK; > > > stream = log_stream_new(param->logStreamName, param->streamId); > > > if (stream == NULL) { > > > LOG_ER("Failed to create log stream %s", param->logStreamName); > > > @@ -2092,26 +2090,18 @@ uint32_t ckpt_proc_open_stream(lgs_cb_t > > *cb, > > > void *data) { > > > goto done; > > > } > > > > > > - rc = lgs_create_rt_appstream(stream); > > > - if (rc != SA_AIS_OK) { > > > - log_stream_delete(&stream); > > > - goto done; > > > - } > > > - > > > stream->numOpeners = param->numOpeners; > > > stream->creationTimeStamp = param->creationTimeStamp; > > > stream->stb_curFileSize = 0; > > > stream->logFileCurrent = param->logFileCurrent; > > > stream->stb_prev_actlogFileCurrent = param->logFileCurrent; > > > stream->stb_logFileCurrent = param->logFileCurrent; > > > - osaf_extended_name_lend(param->logStreamName, &objectName); > > > - SaImmClassNameT className = > > immutil_get_className(&objectName); > > > - if (className != nullptr && strcmp(className, "SaLogStreamConfig") > == > > 0) > > > { > > > - stream->isRtStream = SA_FALSE; > > > - } else { > > > + > > > + if (stream->streamType == STREAM_TYPE_APPLICATION_RT) { > > > stream->isRtStream = SA_TRUE; > > > + } else { > > > + stream->isRtStream = SA_FALSE; > > > } > > [Vu] We might have trouble - not able to close runtime app streams if user > > creates the streams during upgrade (standby LOGsv runs this new code and > > active runs with legacy code). > > When creating runtime application stream, stream information will be > > checkpointed to standby with default `streamType(0)` - Actually > > `streamType` > > have not been checkpointed to standby. > > (refer to lgs_ckpt_stream_open @ lgs_stream.cc). Therefore, > > stream->isRtStream is always set to SA_FALSE on standby LOGsv even the > > stream is runtime. > > > > When standby LOGsv takes active role, request to close/delete such rt > > application stream will not be performed at LOG server side as isRtStream = > > SA_FALSE (refer to log_stream_delete @ lgs_stream.cc). > > In other words, these streams will be leaked. > [Lennart] Note that new STREAM_TYPE_APPLICATION_RT == old > STREAM_TYPE_APPLICATION. This means that if the node running old code > checkpoints a RT stream isRtStream will be set to SA_TRUE but in all other > cases it will be set to SA_FALSE. This will give the same result as reading the > class name of the stream object and set isRtStream to SA_TRUE if the class > name is " SaLogStreamConfig" [Vu2] When creating *config* application stream, active LOGsv with legacy code will checkpoint streamType = STREAM_TYPE_APPLICATION to standby, then standby LOGsv will set isRtStream = SA_TRUE ('cause STREAM_TYPE_APPLICATION_RT(3) == STREAM_TYPE_APPLICATION). But in fact, that stream data belongs to config application stream, not rt app stream. When getting active role, we may have trouble such as generating wrong msgid (?). > > > > > - if (className != nullptr) free(className); > > > > > > // Only update destination names if peer is v6 or upper. > > > if (lgs_is_peer_v6()) { > > > diff --git a/src/log/logd/lgs_mbcsv.h b/src/log/logd/lgs_mbcsv.h > > > index 30ee47b81..7cb4c98a5 100644 > > > --- a/src/log/logd/lgs_mbcsv.h > > > +++ b/src/log/logd/lgs_mbcsv.h > > > @@ -39,9 +39,10 @@ > > > #define LGS_MBCSV_VERSION_4 4 > > > #define LGS_MBCSV_VERSION_5 5 > > > #define LGS_MBCSV_VERSION_6 6 > > > +#define LGS_MBCSV_VERSION_7 7 > > [Vu] Seems there is no place in this patch using this new version? > [Lennart] Yes, this is wrong and shall be removed > > > > > > > > /* Current version */ > > > -#define LGS_MBCSV_VERSION 6 > > > +#define LGS_MBCSV_VERSION 7 > > > #define LGS_MBCSV_VERSION_MIN 1 > > > > > > /* Checkpoint message types(Used as 'reotype' w.r.t mbcsv) */ > > > diff --git a/src/log/logd/lgs_recov.cc b/src/log/logd/lgs_recov.cc > > > index ed4a6e733..dcaad188d 100644 > > > --- a/src/log/logd/lgs_recov.cc > > > +++ b/src/log/logd/lgs_recov.cc > > > @@ -524,12 +524,14 @@ int lgs_restore_one_app_stream(const > std::string > > > &stream_name, > > > > > > /* Get twelveHourModeFlag from the logFileFmt */ > > > lgs_is_valid_format_expression(open_stream_param.logFileFmt, > > > - STREAM_TYPE_APPLICATION, > > &twelveHourModeFlag); > > > + STREAM_TYPE_APPLICATION_RT, > > > + &twelveHourModeFlag); > > > rc_out = lgs_populate_log_stream( > > > open_stream_param.logFileName, > > > open_stream_param.logFilePathName, > > > open_stream_param.maxLogFileSize, > > > open_stream_param.maxLogRecordSize, > > > open_stream_param.logFileFullAction, > > > open_stream_param.maxFilesRotated, > > > - open_stream_param.logFileFmt, STREAM_TYPE_APPLICATION, > > > twelveHourModeFlag, > > > + open_stream_param.logFileFmt, STREAM_TYPE_APPLICATION_RT, > > > + twelveHourModeFlag, > > > 0, > > > log_stream); // output > > > if (rc_out == -1) { > > > diff --git a/src/log/logd/lgs_stream.cc b/src/log/logd/lgs_stream.cc > > > index 65689d658..0c83e305b 100644 > > > --- a/src/log/logd/lgs_stream.cc > > > +++ b/src/log/logd/lgs_stream.cc > > > @@ -394,7 +394,7 @@ void log_stream_print(log_stream_t *stream) { > > > TRACE_2(" pathName: %s", stream->pathName.c_str()); > > > TRACE_2(" maxLogFileSize: %llu", stream->maxLogFileSize); > > > TRACE_2(" fixedLogRecordSize: %u", stream->fixedLogRecordSize); > > > - if (stream->streamType == STREAM_TYPE_APPLICATION) > > > + if (stream->streamType >= STREAM_TYPE_APPLICATION_RT) > > > TRACE_2(" haProperty: %u", stream->haProperty); > > > TRACE_2(" logFullAction: %u", stream->logFullAction); > > > TRACE_2(" logFullHaltThreshold: %u", stream->logFullHaltThreshold); > > > @@ -520,7 +520,7 @@ int lgs_populate_log_stream( > > > * @param stream runtimem app stream > > > * @return SaAisErrorT > > > */ > > > -SaAisErrorT lgs_create_rt_appstream(log_stream_t *const stream) { > > > +SaAisErrorT lgs_create_appstream_rt_object(log_stream_t *const > > stream) { > > > SaAisErrorT rc = SA_AIS_OK; > > > TRACE_ENTER2("%s, l: %zu", stream->name.c_str(), stream- > > >name.size()); > > > > > > @@ -622,20 +622,14 @@ SaAisErrorT > > lgs_create_rt_appstream(log_stream_t > > > *const stream) { > > > &attr_saLogStreamCreationTimestamp, > > > NULL}; > > > > > > - { > > > - /** > > > - * Have to have retry for Rt creation. > > > - * Rt update could consider removing retry to avoid blocking > > > - */ > > > - rc = immutil_saImmOiRtObjectCreate_2( > > > - lgs_cb->immOiHandle, > > > const_cast<SaImmClassNameT>("SaLogStream"), > > > - parentName, attrValues); > > > - free(dndup); > > > - > > > - if (rc != SA_AIS_OK) { > > > - LOG_WA("saImmOiRtObjectCreate_2 returned %u for %s, parent > %s", > > > rc, > > > - stream->name.c_str(), parent_name); > > > - } > > > + rc = immutil_saImmOiRtObjectCreate_2( > > > + lgs_cb->immOiHandle, > > const_cast<SaImmClassNameT>("SaLogStream"), > > > + parentName, attrValues); > > > + free(dndup); > > > + > > > + if (rc != SA_AIS_OK) { > > > + LOG_WA("saImmOiRtObjectCreate_2 returned %u for %s, parent %s", > > rc, > > > + stream->name.c_str(), parent_name); > > > } > > > } > > > > > > @@ -799,7 +793,7 @@ void log_stream_close(log_stream_t **s, time_t > > > *close_time_ptr) { > > > /* Delete stream if last opener > > > * Note: standard streams can never be deleted > > > */ > > > - osafassert(stream->streamType == STREAM_TYPE_APPLICATION); > > > + osafassert(stream->streamType >= STREAM_TYPE_APPLICATION_RT); > > > > > > if (*stream->p_fd != -1) { > > > /* Note: Stream fd is always initiated to -1 > > > diff --git a/src/log/logd/lgs_stream.h b/src/log/logd/lgs_stream.h > > > index efc39d44f..ad8c7412f 100644 > > > --- a/src/log/logd/lgs_stream.h > > > +++ b/src/log/logd/lgs_stream.h > > > @@ -109,7 +109,7 @@ extern int lgs_populate_log_stream( > > > SaBoolT twelveHourModeFlag, uint32_t logRecordId, > > > log_stream_t *const o_stream); > > > > > > -extern SaAisErrorT lgs_create_rt_appstream(log_stream_t *const rt); > > > +extern SaAisErrorT lgs_create_appstream_rt_object(log_stream_t *const > > rt); > > > extern log_stream_t *log_stream_new(const std::string &name, int > > > stream_id); > > > > > > extern void log_stream_open_fileinit(log_stream_t *stream); > > > diff --git a/src/osaf/immutil/immutil.h b/src/osaf/immutil/immutil.h > > > index 40300434d..dc6449bbf 100644 > > > --- a/src/osaf/immutil/immutil.h > > > +++ b/src/osaf/immutil/immutil.h > > > @@ -361,9 +361,11 @@ EXTERN_C SaAisErrorT > > > immutil_update_one_rattr(SaImmOiHandleT immOiHandle, > > > > > > /** > > > * Get class name from object name. > > > + * > > > * @param objectName > > > * > > > - * @return SaImmClassNameT > > > + * @return SaImmClassNameT or NULL if the object does not exist > > > + * NOTE: Will segv if object does exist, is a RT object and there is no > > OI > > > */ > > > EXTERN_C SaImmClassNameT immutil_get_className(const SaNameT > > > *objectName); > > > > > > -- > > > 2.16.1 > > ------------------------------------------------------------------------------ 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
