Hi Vu Yes, I am aware of that and was thinking about removing the isRtStream flag but decided to make the fix as simple as possible and keep all existing logic. However it is probably better to do as you suggest.
Thanks Lennart > -----Original Message----- > From: Vu Minh Nguyen [mailto:vu.m.ngu...@dektech.com.au] > Sent: den 28 februari 2018 08:34 > 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, > > 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
