Hi Lennart, Other comment is, isRtStream field is used to distinguish that stream data belongs to configurable stream or runtime stream.
With this patch, you introduce STREAM_TYPE_APPLICATION_RT/_CFG, then I think that field may be unnecessary. Regards, Vu > -----Original Message----- > From: Vu Minh Nguyen [mailto:vu.m.ngu...@dektech.com.au] > Sent: Wednesday, February 28, 2018 2:10 PM > To: 'Lennart Lund' <lennart.l...@ericsson.com>; > 'canh.v.tru...@dektech.com.au' <canh.v.tru...@dektech.com.au> > Cc: 'opensaf-devel@lists.sourceforge.net' <opensaf- > de...@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. > > > - 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 Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
