Hi Vu,

See attached diff for comments.
Note: I found one possible error

Thanks
Lennart

> -----Original Message-----
> From: Vu Minh Nguyen [mailto:[email protected]]
> Sent: den 12 september 2017 15:57
> To: Lennart Lund <[email protected]>; Canh Van Truong
> <[email protected]>
> Cc: [email protected]; Vu Minh Nguyen
> <[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

Attachment: lldcomments_170912.diff
Description: lldcomments_170912.diff

------------------------------------------------------------------------------
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

Reply via email to