Hi Lennart, See my comments inline, with [Vu].
Regards, Vu > -----Original Message----- > From: Lennart Lund [mailto:[email protected]] > Sent: Monday, February 26, 2018 8:40 PM > To: [email protected]; [email protected] > Cc: [email protected]; Lennart Lund > <[email protected]> > 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. > - 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? > > /* 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 [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
