Hi,
I added the logic `(!(attr_def->attrFlags & SA_IMM_ATTR_CACHED)` to make the code generic - fetch all attributes, except pure runtime. Later on, if we introduce any new attributes and want to fetch their values from IMM at startup, we will not touch that logic again. Below is my responses to your comments: > is the value fetched from "saLogStreamCreationTimestamp" used in any way? In the method ` stream_create_and_configure`, I see LOG is interested in this attribute value. So, I thought the attribute value could be useful somewhere, but not seems so as the value is updated afterward. Even fetching ` saLogStreamCreationTimestamp` value is nonsense for now, I prefer to keep the logic than ignore fetching cached runtime attributes. What is your opinion? Regards, Vu From: Canh Van Truong [mailto:[email protected]] Sent: Wednesday, September 13, 2017 6:56 PM To: 'Lennart Lund' <[email protected]>; 'Vu Minh Nguyen' <[email protected]> Cc: [email protected] Subject: RE: [PATCH 1/1] log: fix log server fail to start with old IMM model [#2580] Hi Lennart, As I understand that almost the value of "saLogStreamCreationTimestamp" is updated with current time at creating stream. But in case headless, its value should be fetch from IMM, and the log file of stream is just re-opened, NOT creating new log file. Later, logsv just use this attribute value to update its value in IMM generally " ais_rc = immutil_update_one_rattr( cb->immOiHandle, stream->name.c_str(), const_cast<SaImmAttrNameT>("saLogStreamCreationTimestamp"), SA_IMM_ATTR_SATIMET, &stream->creationTimeStamp); " In case, log file is re-opened (headless), it may not need to use this function to re-updated this attribute value to IMM. I understand your mean that the fetched valued may not need to use any way. And logsv don't need to fetch this value But if logsv don't fetch the value of "saLogStreamCreationTimestamp" , its value is not consistent between in IMM and in logsv (stream->creationTimeStamp). Logically it is not right ?? Maybe in the future, if logsv use the value of this attribute, the value in "creationTimeStamp" cannot be used. It need to fetch from IMM again. Or logsv may design new attribute with flag with "SA_IMM_ATTR_CACHED" and use this value, it is useful if we fetch the attribute with flag "SA_IMM_ATTR_CACHED" here ? Regards Canh From: Lennart Lund [mailto:[email protected]] Sent: Wednesday, September 13, 2017 5:42 PM To: Vu Minh Nguyen <[email protected] <mailto:[email protected]> >; Canh Van Truong <[email protected] <mailto:[email protected]> > Cc: [email protected] <mailto:[email protected]> Subject: RE: [PATCH 1/1] log: fix log server fail to start with old IMM model [#2580] Hi Vu You are right that my comment about the attribute storing loop was not correct. The code in this patch is easier to follow and now it is easy to see that all attributes that are not pure runtime is collected which seems correct. However when creating a configuration stream only the configuration attributes should be needed. The only cached runtime attribute that exist in the configuration stream class is "saLogStreamCreationTimestamp". A configuration stream is always created when the log server is started meaning that "saLogStreamCreationTimestamp" shall always be set to this time. There should be no use for the cached value. In case of headless only the application (runtime) streams should be recovered, configuration streams should also in this case be re-opened. The question is, is the value fetched from "saLogStreamCreationTimestamp" used in any way? If not, only attributes with flag SA_IMM_ATTR_CACHED should be of any interest Thanks Lennart From: Vu Minh Nguyen [mailto:[email protected]] Sent: den 13 september 2017 07:11 To: Lennart Lund <[email protected] <mailto:[email protected]> >; Canh Van Truong <[email protected] <mailto:[email protected]> > Cc: [email protected] <mailto:[email protected]> Subject: RE: [PATCH 1/1] log: fix log server fail to start with old IMM model [#2580] Hi Lennart, Thanks for your comments. Below is the SaLogStreamConfig class description - result of `immlist -c SaLogStreamConfig`. If we collect attribute names based on `SA_IMM_ATTR_RUNTIME`, we will miss lot of LOG's interested attribute names. << SaLogStreamConfig - CONFIG >> safLgStrCfg : SA_STRING_T [1] {RDN, CONFIG, INITIALIZED} saLogStreamSeverityFilter : SA_UINT32_T [0] = Empty {CONFIG, WRITEABLE} saLogStreamPathName : SA_STRING_T [1] {CONFIG, INITIALIZED} saLogStreamNumOpeners : SA_UINT32_T [0] {RUNTIME} saLogStreamMaxLogFileSize : SA_UINT64_T [0] = 5000000 (0x4c4b40) {CONFIG, WRITEABLE} saLogStreamMaxFilesRotated : SA_UINT32_T [0] = 4 (0x4) {CONFIG, WRITEABLE} saLogStreamLogFullHaltThreshold : SA_UINT32_T [0] = 75 (0x4b) {CONFIG, WRITEABLE} saLogStreamLogFullAction : SA_UINT32_T [0] = 3 (0x3) {CONFIG, WRITEABLE} saLogStreamLogFileFormat : SA_STRING_T [0] = Empty {CONFIG, WRITEABLE} saLogStreamFixedLogRecordSize : SA_UINT32_T [0] = 150 (0x96) {CONFIG, WRITEABLE} saLogStreamFileName : SA_STRING_T [1] {CONFIG, WRITEABLE, INITIALIZED} saLogStreamCreationTimestamp : SA_TIME_T [0] {RUNTIME, CACHED} saLogRecordDestination : SA_STRING_T [0..*] = Empty {CONFIG, WRITEABLE, MULTI_VALUE, NO_DUPLICATES} logStreamDiscardedCounter : SA_UINT64_T [0] {RUNTIME} I expect to get all attribute names, expect pure runtime attributes. The reason is, fetching pure runtime attribute will lead to deadlock. So, below logic is used to ignore pure runtime attribute names. + if ((attr_def->attrFlags & SA_IMM_ATTR_RUNTIME) && + (!(attr_def->attrFlags & SA_IMM_ATTR_CACHED))) continue; In the final list, `list_attr_names`, will contain other attribute name LOG don't have interested in, LOG will ignore them. Regarding your other comments, I edited them even some belongs to legacy code. I sent out version 02, please have a look and give your comments. Thanks. Regards, Vu > -----Original Message----- > From: Lennart Lund [mailto:[email protected]] > Sent: Tuesday, September 12, 2017 10:06 PM > To: Vu Minh Nguyen <[email protected] <mailto:[email protected]> >; Canh Van Truong > <[email protected] <mailto:[email protected]> > > Cc: [email protected] <mailto:[email protected]> ; Vu Minh Nguyen > <[email protected] <mailto:[email protected]> > > Subject: RE: [PATCH 1/1] log: fix log server fail to start with old IMM model > [#2580] > > Hi Vu, > > See attached diff for comments. > Note: I found one possible error > > Thanks > Lennart > > > -----Original Message----- > > From: Vu Minh Nguyen [ <mailto:[email protected]> mailto:[email protected]] > > Sent: den 12 september 2017 15:57 > > To: Lennart Lund < <mailto:[email protected]> [email protected]>; Canh Van Truong > > < <mailto:[email protected]> [email protected]> > > Cc: <mailto:[email protected]> [email protected]; Vu Minh Nguyen > > < <mailto:[email protected]> [email protected]> > > Subject: [PATCH 1/1] log: fix log server fail to start with old IMM model > > [#2580] > > > > LOG introduced the `saLogRecordDestination` attribute since OpenSAF 5.2 > > to handle the alternative destinations of log records, ticket [#2258]. > > During upgrade, if LOG server comes up before IMM model is updated > > to new one which has saLogRecordDestination in, LOG server will be > crashed > > > > The solution is if not able to fetching new added attribute value from IMM, > > LOG will try one more time with the attribute list without new added ones. > > --- > > src/log/logd/lgs_amf.cc | 1 + > > src/log/logd/lgs_amf.h | 2 + > > src/log/logd/lgs_imm.cc | 113 ++++++++++++++++++++---------------------- > - > > --- > > src/log/logd/lgs_mbcsv.cc | 3 +- > > src/log/logd/lgs_mbcsv.h | 3 ++ > > src/log/logd/lgs_stream.h | 2 + > > 6 files changed, 59 insertions(+), 65 deletions(-) > > > > diff --git a/src/log/logd/lgs_amf.cc b/src/log/logd/lgs_amf.cc > > index d62be99..30a4d5b 100644 > > --- a/src/log/logd/lgs_amf.cc > > +++ b/src/log/logd/lgs_amf.cc > > @@ -19,6 +19,7 @@ > > * other modules. > > */ > > > > +#include "log/logd/lgs_amf.h" > > #include "nid/agent/nid_start_util.h" > > #include "osaf/immutil/immutil.h" > > #include "log/logd/lgs.h" > > diff --git a/src/log/logd/lgs_amf.h b/src/log/logd/lgs_amf.h > > index 91bd2de..2a7f0ff 100644 > > --- a/src/log/logd/lgs_amf.h > > +++ b/src/log/logd/lgs_amf.h > > @@ -18,6 +18,8 @@ > > #ifndef LOG_LOGD_LGS_AMF_H_ > > #define LOG_LOGD_LGS_AMF_H_ > > > > +#include <saAis.h> > > +#include "log/logd/lgs_cb.h" > > > > SaAisErrorT lgs_amf_init(lgs_cb_t *cb); > > > > diff --git a/src/log/logd/lgs_imm.cc b/src/log/logd/lgs_imm.cc > > index c856ec2..76faf6c 100644 > > --- a/src/log/logd/lgs_imm.cc > > +++ b/src/log/logd/lgs_imm.cc > > @@ -34,7 +34,6 @@ > > #include <stdio.h> > > #include <errno.h> > > #include <string.h> > > - > > #include <saImmOm.h> > > #include <saImmOi.h> > > > > @@ -2733,33 +2732,16 @@ done: > > * @return SaAisErrorT > > */ > > static SaAisErrorT stream_create_and_configure( > > - const std::string &dn, log_stream_t **in_stream, int stream_id, > > - SaImmAccessorHandleT accessorHandle) { > > + const std::string &dn, int stream_id, > > + SaImmAttrValuesT_2** attributes) { > > SaAisErrorT rc = SA_AIS_OK; > > - SaNameT objectName; > > SaImmAttrValuesT_2 *attribute; > > - SaImmAttrValuesT_2 **attributes; > > int i = 0; > > log_stream_t *stream; > > - char *attribute_names[] = { > > - const_cast<char *>("saLogStreamFileName"), > > - const_cast<char *>("saLogStreamPathName"), > > - const_cast<char *>("saLogStreamMaxLogFileSize"), > > - const_cast<char *>("saLogStreamFixedLogRecordSize"), > > - const_cast<char *>("saLogStreamLogFullAction"), > > - const_cast<char *>("saLogStreamLogFullHaltThreshold"), > > - const_cast<char *>("saLogStreamMaxFilesRotated"), > > - const_cast<char *>("saLogStreamLogFileFormat"), > > - const_cast<char *>("saLogRecordDestination"), > > - const_cast<char *>("saLogStreamSeverityFilter"), > > - const_cast<char *>("saLogStreamCreationTimestamp"), > > - NULL}; > > > > TRACE_ENTER2("(%s)", dn.c_str()); > > > > - osaf_extended_name_lend(dn.c_str(), &objectName); > > - > > - *in_stream = stream = log_stream_new(dn, stream_id); > > + stream = log_stream_new(dn, stream_id); > > > > if (stream == NULL) { > > rc = SA_AIS_ERR_NO_MEMORY; > > @@ -2775,15 +2757,6 @@ static SaAisErrorT > stream_create_and_configure( > > else > > stream->streamType = STREAM_TYPE_APPLICATION; > > > > - /* Get all attributes of the object */ > > - if ((rc = immutil_saImmOmAccessorGet_2(accessorHandle, > &objectName, > > - attribute_names, &attributes)) != > > - SA_AIS_OK) { > > - LOG_ER("Configuration for %s not found: %s", dn.c_str(), saf_error(rc)); > > - rc = SA_AIS_ERR_NOT_EXIST; > > - goto done; > > - } > > - > > while ((attribute = attributes[i++]) != NULL) { > > void *value; > > > > @@ -2910,11 +2883,11 @@ SaAisErrorT > lgs_imm_init_configStreams(lgs_cb_t > > *cb) { > > int int_rc = 0; > > log_stream_t *stream; > > SaImmHandleT omHandle; > > - SaImmAccessorHandleT accessorHandle; > > SaVersionT immVersion = {'A', 2, 1}; > > SaImmSearchHandleT immSearchHandle; > > SaImmSearchParametersT_2 objectSearch; > > SaImmAttrValuesT_2 **attributes; > > + SaImmAttrDefinitionT_2** attr_definitions; > > int wellknownStreamId = 0; > > int appStreamId = 3; > > uint32_t streamId = 0; > > @@ -2929,12 +2902,28 @@ SaAisErrorT > lgs_imm_init_configStreams(lgs_cb_t > > *cb) { > > osaf_abort(0); > > } > > > > - om_rc = immutil_saImmOmAccessorInitialize(omHandle, > > &accessorHandle); > > - if (om_rc != SA_AIS_OK) { > > - LOG_ER("immutil_saImmOmAccessorInitialize failed %s", > > saf_error(om_rc)); > > - osaf_abort(0); > > + // We don't know the IMM model we are working with is the latest > model > > + // or the old ones that could not have our addedd new attribute names. > > + // So, we ask helps from `saImmOmClassDescriptionGet_2`. > > + SaImmClassCategoryT category = SA_IMM_CLASS_CONFIG; > > + om_rc = immutil_saImmOmClassDescriptionGet_2( > > + omHandle, const_cast<char*>(className), &category, > > &attr_definitions); > > + osafassert(om_rc == SA_AIS_OK); > > + > > + // Get list of non-pure of `SaLogStreamConfig` class. > > + unsigned ii = 0; > > + SaImmAttrDefinitionT_2* temp; > > + std::vector<char*> list_temp; > > + while ((temp = attr_definitions[ii++]) != nullptr) { > > + if (temp->attrFlags & SA_IMM_ATTR_RUNTIME) continue; > > + list_temp.push_back(temp->attrName); > > } > > > > + ii = 0; > > + char* list_attributes[list_temp.size() + 1]; > > + for (auto& item : list_temp) list_attributes[ii++] = item; > > + list_attributes[ii] = nullptr; > > + > > /* Search for all objects of class "SaLogStreamConfig". */ > > /** > > * Should not base on the attribute name `safLgStrCfg` > > @@ -2951,29 +2940,32 @@ SaAisErrorT > lgs_imm_init_configStreams(lgs_cb_t > > *cb) { > > * `safApp=safLogService` might miss configurable app stream eg: app > > stream > > * with DN `saLgStrCfg=test` > > */ > > - if ((om_rc = immutil_saImmOmSearchInitialize_2( > > - omHandle, NULL, SA_IMM_SUBTREE, > > - SA_IMM_SEARCH_ONE_ATTR | SA_IMM_SEARCH_GET_NO_ATTR, > > &objectSearch, > > - NULL, /* Get no attributes */ > > - &immSearchHandle)) == SA_AIS_OK) { > > - while (immutil_saImmOmSearchNext_2(immSearchHandle, > > &objectName, > > - &attributes) == SA_AIS_OK) { > > - /** > > - * With headless mode enabled, when lgsv restarts, there could be > other > > - * configurable app streams. It differs from legacy mode in which no > app > > - * stream could be exist at startup. > > - * > > - * With well-known streams, stream ID is in reserved numbers [0-2]. > > - */ > > - SaConstStringT name = osaf_extended_name_borrow(&objectName); > > - streamId = > > - is_well_know_stream(name) ? wellknownStreamId++ : > > appStreamId++; > > - ais_rc = > > - stream_create_and_configure(name, &stream, streamId, > > accessorHandle); > > - if (ais_rc != SA_AIS_OK) { > > - LOG_WA("stream_create_and_configure failed %d", ais_rc); > > - goto done; > > - } > > + > > + om_rc = immutil_saImmOmSearchInitialize_2( > > + omHandle, NULL, SA_IMM_SUBTREE, > > + SA_IMM_SEARCH_ONE_ATTR | SA_IMM_SEARCH_GET_SOME_ATTR, > > + &objectSearch, > > + list_attributes, > > + &immSearchHandle); > > + osafassert(om_rc == SA_AIS_OK); > > + > > + while (immutil_saImmOmSearchNext_2(immSearchHandle, > > &objectName, > > + &attributes) == SA_AIS_OK) { > > + /** > > + * With headless mode enabled, when lgsv restarts, there could be > other > > + * configurable app streams. It differs from legacy mode in which no > app > > + * stream could be exist at startup. > > + * > > + * With well-known streams, stream ID is in reserved numbers [0-2]. > > + */ > > + SaConstStringT name = osaf_extended_name_borrow(&objectName); > > + streamId = > > + is_well_know_stream(name) ? wellknownStreamId++ : > > appStreamId++; > > + ais_rc = > > + stream_create_and_configure(name, streamId, attributes); > > + if (ais_rc != SA_AIS_OK) { > > + LOG_WA("stream_create_and_configure failed %d", ais_rc); > > + goto done; > > } > > } > > > > @@ -3022,11 +3014,6 @@ SaAisErrorT > lgs_imm_init_configStreams(lgs_cb_t > > *cb) { > > > > done: > > /* Do not abort if error when finalizing */ > > - om_rc = immutil_saImmOmAccessorFinalize(accessorHandle); > > - if (om_rc != SA_AIS_OK) { > > - LOG_NO("%s immutil_saImmOmAccessorFinalize() Fail %d", > > __FUNCTION__, om_rc); > > - } > > - > > om_rc = immutil_saImmOmSearchFinalize(immSearchHandle); > > if (om_rc != SA_AIS_OK) { > > LOG_NO("%s immutil_saImmOmSearchFinalize() Fail %d", > > __FUNCTION__, om_rc); > > diff --git a/src/log/logd/lgs_mbcsv.cc b/src/log/logd/lgs_mbcsv.cc > > index 175858b..33149de 100644 > > --- a/src/log/logd/lgs_mbcsv.cc > > +++ b/src/log/logd/lgs_mbcsv.cc > > @@ -16,9 +16,8 @@ > > * > > */ > > > > +#include "log/logd/lgs_mbcsv.h" > > #include "log/logd/lgs.h" > > -#include "base/ncssysf_def.h" > > -#include "base/ncssysf_mem.h" > > #include "base/osaf_time.h" > > > > #include "osaf/immutil/immutil.h" > > diff --git a/src/log/logd/lgs_mbcsv.h b/src/log/logd/lgs_mbcsv.h > > index 30ee47b..e602378 100644 > > --- a/src/log/logd/lgs_mbcsv.h > > +++ b/src/log/logd/lgs_mbcsv.h > > @@ -22,6 +22,9 @@ > > > > #include <stdint.h> > > #include <saAmf.h> > > +#include "log/logd/lgs_cb.h" > > +#include "base/ncssysf_def.h" > > +#include "base/ncssysf_mem.h" > > > > /* Version 1: Logservice check-point version older versions than OpenSAF > > 4.4. > > * Cannot be configured for split filesystem. > > diff --git a/src/log/logd/lgs_stream.h b/src/log/logd/lgs_stream.h > > index 3c52534..efc39d4 100644 > > --- a/src/log/logd/lgs_stream.h > > +++ b/src/log/logd/lgs_stream.h > > @@ -89,6 +89,8 @@ typedef struct log_stream { > > uint32_t stb_curFileSize; /* Bytes written to current log file */ > > > > // Hold vector of destname string {"name1", "name2", etc.} > > + // Default value is empty. It MUST be synced with > > + // `saLogRecordDestination` default value. > > std::vector<std::string> dest_names; > > // Hold a list of strings separated by semicolon "name1;name2;etc" > > // This data is used to checkpoint to standby > > -- > > 1.9.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
