Hi Vu There have been discussions about creating a better wrapper for IMM APIs (probably C++) than the current immutil but there is no planned activity for now meaning that we have to cope with what we have for a while. In this case it is the immutilWrapperProfile that's the problem and as I mentioned we can set the profile once in the log service (during init phase) and use this setting without change in all threads. In practice this means that the only usage of immutil as an API wrapper is for it to provide a try again loop with a preset timeout that cannot be changed. If anything else is needed in some place the IMM API may have to be used directly. Also all error handling must be done where the API is used also if the only error handling needed is an abort. To use macros also as a temporary solution is not good and we must not add more of them. There is also a big risk that this is not going to be a short time solution. So I suggest that this problem is handled by as described above (probably best choice). An alternative is to use the IMM APIs directly (already done in many places) and not use immutil at all or maybe use immutil in main thread only.
Thanks Lennart > -----Original Message----- > From: Vu Minh Nguyen [mailto:[email protected]] > Sent: den 13 maj 2016 11:54 > To: Lennart Lund; [email protected] > Cc: [email protected] > Subject: RE: [PATCH 1 of 1] log: make using immutilWrapperProfile thread > safe [#1807] > > Hi Lennart, > > Creating macro is a temporary solution. Later, I hope IMM should provide > other immutil utility > which do same things but thread-safe. As now, most SAF services have > multiple threads. > > To verify the macro work correctly, I used GCC with option "-E" to generate > pre-processing code. > E.g: > me@:~/opensaf/opensaf-staging/osaf/services/saf/logsv/lgs$ touch > lgs_imm_gcfg.cc > me@:~/opensaf/opensaf-staging/osaf/services/saf/logsv/lgs$ make > AM_DEFAULT_VERBOSITY=1 > > Then, generate the pre-processing code by using the same command for > compiler lgs_imm_gcfg.cc, but using "-E" option. > > Regarding your proposal, I am afraid of applying the "standard" solution to > LOG now could be high risk. > To avoid using macro, other way, is to not using immutil in > `applier_thread`, but calling to IMM API directly > and create our own retry mechanism in `applier_thread`. > > How about your idea? > > Regards, Vu. > > >-----Original Message----- > >From: Lennart Lund [mailto:[email protected]] > >Sent: Friday, May 13, 2016 3:26 PM > >To: Vu Minh Nguyen; [email protected] > >Cc: [email protected]; Lennart Lund > >Subject: RE: [PATCH 1 of 1] log: make using immutilWrapperProfile thread > safe > >[#1807] > > > >Hi Vu, > > > >The ticket points out a problem that has to be fixed and your patch is a > >solution that probably will work (I have not tested) but... > >We should not create new macros since they do create problems e.g. not > clean > >code, may hide problems that could have been detected by compiler etc. > Also, > >we shall follow the Google style guide that says that macros shall be > avoided > >(also explains why). > > > >The "standard" solution in OpenSAF for handling immutil (see e.g. smf) is > to > >setup the immutilWrapperProfile during init of service before any threads > are > >created and after that the immutilWrapperProfile is never changed. To do > this > >is log service should work. In some places changes of > immutilWrapperProfile > >must be removed and in places when an abort is wanted if not SA_AIS_OK > it > >must be handled in place and not letting immutil do that. > > > >Thanks > >Lennart > > > >> -----Original Message----- > >> From: Vu Minh Nguyen [mailto:[email protected]] > >> Sent: den 11 maj 2016 12:32 > >> To: [email protected]; Lennart Lund > >> Cc: [email protected] > >> Subject: [PATCH 1 of 1] log: make using immutilWrapperProfile thread > safe > >> [#1807] > >> > >> osaf/services/saf/logsv/lgs/lgs_imm_gcfg.cc | 110 ++++++++++++++++-- > --- > >> ------ > >> osaf/services/saf/logsv/lgs/lgs_util.cc | 18 ++++ > >> osaf/services/saf/logsv/lgs/lgs_util.h | 46 +++++++++++ > >> 3 files changed, 130 insertions(+), 44 deletions(-) > >> > >> > >> LOG has several threads which were using same global > >> `immutilWrapperProfile` variable > >> when invoking `immutil_` utility. This caused race condition. > >> > >> This patch created an macro which have same functionalities as `immutil_` > >> does, > >> but thread safe. This macro is used by `applier_thread`, the main thread > is > >> still using `immutil`. By this way, there is no race condition in > >> using `immutilWrapperProfile` between main thread and `applier_thread`. > >> > >> diff --git a/osaf/services/saf/logsv/lgs/lgs_imm_gcfg.cc > >> b/osaf/services/saf/logsv/lgs/lgs_imm_gcfg.cc > >> --- a/osaf/services/saf/logsv/lgs/lgs_imm_gcfg.cc > >> +++ b/osaf/services/saf/logsv/lgs/lgs_imm_gcfg.cc > >> @@ -31,6 +31,7 @@ > >> #include "osaf_time.h" > >> #include "osaf_poll.h" > >> > >> +#include "lgs_util.h" > >> /* > >> * Implements an IMM applier for the OpensafConfig class. > >> * Used for detecting changes of opensafNetworkName attribute. > >> @@ -117,8 +118,6 @@ typedef enum { > >> > >> static th_state_t th_state = TH_NOT_STARTED; > >> > >> -extern struct ImmutilWrapperProfile immutilWrapperProfile; > >> - > >> > >> > /********************************************************** > >> ******************** > >> * Outside applier thread > >> > >> > ********************************************************** > >> ********************/ > >> @@ -610,13 +609,23 @@ static int read_network_name() > >> * Initialize an IMM object manager > >> */ > >> // Wait up to 500ms > >> - immutilWrapperProfile.errorsAreFatal = 0; > >> - immutilWrapperProfile.nTries = 500; > >> - immutilWrapperProfile.retryInterval = 1000; > >> + ImmutilWrapperProfile_t profile; > >> + profile.errorsAreFatal = 0; > >> + profile.nTries = 500; > >> + profile.retryInterval = 1000; > >> > >> - ais_rc = immutil_saImmOmInitialize(&om_handle, NULL, > >> &immVersion); > >> + SaVersionT localVer = immVersion; > >> + ais_rc = saImmOmInitialize(&om_handle, NULL, > >> &immVersion); > >> + unsigned int nTries = 1; > >> + while (ais_rc == SA_AIS_ERR_TRY_AGAIN > >> + && nTries < profile.nTries) { > >> + usleep(profile.retryInterval * 1000); > >> + localVer = immVersion; > >> + ais_rc = saImmOmInitialize(&om_handle, NULL, > >> &localVer); > >> + nTries++; > >> + } > >> if (ais_rc != SA_AIS_OK) { > >> - TRACE("immutil_saImmOmInitialize FAIL %s", > >> saf_error(ais_rc)); > >> + TRACE("saImmOmInitialize FAIL %s", > >> saf_error(ais_rc)); > >> rc = -1; > >> goto done; > >> } > >> @@ -627,15 +636,14 @@ static int read_network_name() > >> /* Find the opensafNetworkName attribute in the > >> OpensafConfig object and > >> * get its value > >> */ > >> - ais_rc = immutil_saImmOmSearchInitialize_2( > >> - om_handle, > >> - NULL, > >> - SA_IMM_SUBTREE, > >> - SA_IMM_SEARCH_ONE_ATTR | > >> SA_IMM_SEARCH_GET_SOME_ATTR, > >> - &searchParam, > >> - attributeNames, > >> - &searchHandle > >> - ); > >> + ais_rc = IMMUTIL_F(profile, saImmOmSearchInitialize_2, > >> + om_handle, > >> + NULL, > >> + SA_IMM_SUBTREE, > >> + SA_IMM_SEARCH_ONE_ATTR | > >> SA_IMM_SEARCH_GET_SOME_ATTR, > >> + &searchParam, > >> + attributeNames, > >> + &searchHandle); > >> if (ais_rc != SA_AIS_OK) { > >> TRACE("immutil_saImmOmSearchInitialize_2 > >> FAIL %s", saf_error(ais_rc)); > >> rc = -1; > >> @@ -645,7 +653,11 @@ static int read_network_name() > >> /* > >> * Get the value for the searched attribute > >> */ > >> - ais_rc = immutil_saImmOmSearchNext_2(searchHandle, > >> &object_name, &attributes); > >> + ais_rc = IMMUTIL_F(profile, > >> + saImmOmSearchNext_2, > >> + searchHandle, > >> + &object_name, > >> + &attributes); > >> if (ais_rc != SA_AIS_OK) { > >> TRACE("immutil_saImmOmSearchNext_2 FAIL > >> %s", saf_error(ais_rc)); > >> rc = -1; > >> @@ -671,7 +683,7 @@ static int read_network_name() > >> > >> done: > >> /* Finalize search */ > >> - ais_rc = immutil_saImmOmFinalize(om_handle); > >> + ais_rc = IMMUTIL_F(profile, saImmOmFinalize, om_handle); > >> if ((ais_rc != SA_AIS_OK) && (ais_rc != > >> SA_AIS_ERR_BAD_HANDLE)) { > >> /* BAD HANDLE is not a fault here since finalize > >> is called > >> * also if initialize failed > >> @@ -723,7 +735,7 @@ static int applier_init(SaImmOiHandleT * > >> if (th_do_cancel()) { > >> TRACE("Canceled at 1"); > >> /* Cancellation point no error */ > >> - goto done; > >> + return rc; > >> } > >> > >> /* Finalize applier OI handle if needed */ > >> @@ -732,47 +744,56 @@ static int applier_init(SaImmOiHandleT * > >> if (th_do_cancel()) { > >> TRACE("Canceled at 2"); > >> /* Cancellation point no error */ > >> - goto done; > >> + return rc; > >> } > >> > >> // Wait up to 500ms > >> - immutilWrapperProfile.errorsAreFatal = 0; > >> - immutilWrapperProfile.nTries = 500; > >> - immutilWrapperProfile.retryInterval = 1000; > >> + ImmutilWrapperProfile_t profile; > >> + profile.errorsAreFatal = 0; > >> + profile.nTries = 500; > >> + profile.retryInterval = 1000; > >> > >> /* Initialize OI for applier and get OI handle */ > >> - ais_rc = immutil_saImmOiInitialize_2(imm_appl_hdl, > >> &callbacks, &immVersion); > >> + ais_rc = saImmOiInitialize_2(imm_appl_hdl, &callbacks, > >> &immVersion); > >> + SaVersionT localVer = immVersion; > >> + unsigned int nTries = 1; > >> + while (ais_rc == SA_AIS_ERR_TRY_AGAIN > >> + && nTries < profile.nTries) { > >> + usleep(profile.retryInterval * 1000); > >> + localVer = immVersion; > >> + ais_rc = saImmOiInitialize_2(imm_appl_hdl, > >> &callbacks, &immVersion); > >> + nTries++; > >> + } > >> if (ais_rc != SA_AIS_OK) { > >> - LOG_WA("immutil_saImmOiInitialize_2 Failed > >> %s", saf_error(ais_rc)); > >> + LOG_WA("saImmOiInitialize_2 Failed %s", > >> saf_error(ais_rc)); > >> rc = -1; > >> - goto done; > >> + return rc; > >> } > >> > >> if (th_do_cancel()) { > >> TRACE("Canceled at 3"); > >> /* Cancellation point no error */ > >> applier_finalize(*imm_appl_hdl); > >> - goto done; > >> + return rc; > >> } > >> > >> /* Get selection object for event handling */ > >> - ais_rc = immutil_saImmOiSelectionObjectGet(*imm_appl_hdl, > >> imm_appl_selobj); > >> + ais_rc = IMMUTIL_F(profile, saImmOiSelectionObjectGet, > >> *imm_appl_hdl, imm_appl_selobj); > >> if (ais_rc != SA_AIS_OK) { > >> LOG_WA("immutil_saImmOiSelectionObjectGet > >> Failed %s", saf_error(ais_rc)); > >> applier_finalize(*imm_appl_hdl); > >> imm_appl_hdl = 0; > >> rc = -1; > >> - goto done; > >> + return rc; > >> } > >> > >> if (th_do_cancel()) { > >> TRACE("Canceled at 3"); > >> /* Cancellation point no error */ > >> applier_finalize(*imm_appl_hdl); > >> - goto done; > >> + return rc; > >> } > >> > >> -done: > >> TRACE_LEAVE2("rc = %d", rc); > >> return rc; > >> } > >> @@ -800,31 +821,32 @@ static int applier_set_name_class(SaImmO > >> TRACE("Canceled at 1"); > >> /* Cancellation point no error */ > >> applier_finalize(imm_appl_hdl); > >> - goto done; > >> + return rc; > >> } > >> > >> /* Become applier */ > >> - immutilWrapperProfile.errorsAreFatal = 0; > >> - immutilWrapperProfile.nTries = 500; > >> - immutilWrapperProfile.retryInterval = 1000; > >> + ImmutilWrapperProfile_t profile; > >> + profile.errorsAreFatal = 0; > >> + profile.nTries = 500; > >> + profile.retryInterval = 1000; > >> > >> - ais_rc = immutil_saImmOiImplementerSet(imm_appl_hdl, > >> applier_name); > >> + ais_rc = IMMUTIL_F(profile, saImmOiImplementerSet, > >> imm_appl_hdl, applier_name); > >> if (ais_rc != SA_AIS_OK) { > >> LOG_WA("immutil_saImmOiImplementerSet > >> Failed %s", saf_error(ais_rc)); > >> applier_finalize(imm_appl_hdl); > >> rc = -1; > >> - goto done; > >> + return rc; > >> } > >> > >> if (th_do_cancel()) { > >> TRACE("Canceled at 2"); > >> /* Cancellation point no error */ > >> applier_finalize(imm_appl_hdl); > >> - goto done; > >> + return rc; > >> } > >> > >> /* Become class implementer (applier) for the OpensafConfig > >> class */ > >> - ais_rc = > >> immutil_saImmOiClassImplementerSet(imm_appl_hdl, gcfg_class); > >> + ais_rc = IMMUTIL_F(profile, saImmOiClassImplementerSet, > >> imm_appl_hdl, gcfg_class); > >> if (ais_rc != SA_AIS_OK) { > >> > >> LOG_WA("immutil_saImmOiClassImplementerSet Failed %s", > >> saf_error(ais_rc)); > >> applier_finalize(imm_appl_hdl); > >> @@ -852,7 +874,6 @@ done: > >> static void applier_finalize(SaImmOiHandleT imm_appl_hdl) > >> { > >> SaAisErrorT ais_rc = SA_AIS_OK; > >> - > >> TRACE_ENTER(); > >> > >> /* Finalize applier OI handle if needed */ > >> @@ -860,11 +881,12 @@ static void applier_finalize(SaImmOiHand > >> /* We have a handle to finalize */ > >> TRACE("saImmOiFinalize"); > >> > >> - immutilWrapperProfile.errorsAreFatal = 0; > >> - immutilWrapperProfile.nTries = 500; > >> - immutilWrapperProfile.retryInterval = 1000; > >> + ImmutilWrapperProfile_t profile; > >> + profile.errorsAreFatal = 0; > >> + profile.nTries = 500; > >> + profile.retryInterval = 1000; > >> > >> - ais_rc = > >> immutil_saImmOiFinalize(imm_appl_hdl); > >> + ais_rc = IMMUTIL_F(profile, saImmOiFinalize, > >> imm_appl_hdl); > >> if (ais_rc != SA_AIS_OK) { > >> TRACE("immutil_saImmOiFinalize > >> Failed %s", saf_error(ais_rc)); > >> } > >> diff --git a/osaf/services/saf/logsv/lgs/lgs_util.cc > >> b/osaf/services/saf/logsv/lgs/lgs_util.cc > >> --- a/osaf/services/saf/logsv/lgs/lgs_util.cc > >> +++ b/osaf/services/saf/logsv/lgs/lgs_util.cc > >> @@ -48,6 +48,24 @@ > >> #define LOG_TAIL_MAX > >> (std::string("_yyyymmdd_hhmmss_yyyymmdd_hhmmss.log").size()) > >> > >> /** > >> + * Report to stderr and syslog and abort process > >> + * @param fmt > >> + */ > >> +void logError(char const *fmt, ...) > >> +{ > >> + va_list ap; > >> + va_list ap2; > >> + > >> + va_start(ap, fmt); > >> + va_copy(ap2, ap); > >> + vfprintf(stderr, fmt, ap); > >> + vsyslog(LOG_ERR, fmt, ap2); > >> + va_end(ap2); > >> + va_end(ap); > >> + abort(); > >> +} > >> + > >> +/** > >> * Create config file according to spec. > >> * @param root_path[in] > >> * @param stream[in] > >> diff --git a/osaf/services/saf/logsv/lgs/lgs_util.h > >> b/osaf/services/saf/logsv/lgs/lgs_util.h > >> --- a/osaf/services/saf/logsv/lgs/lgs_util.h > >> +++ b/osaf/services/saf/logsv/lgs/lgs_util.h > >> @@ -29,6 +29,7 @@ > >> > >> #include "lgs_stream.h" > >> #include "lgs_evt.h" > >> +#include "immutil.h" > >> > >> /* > >> > ========================================================== > >> ============== > >> * DEFINITIONS > >> @@ -40,11 +41,55 @@ > >> #define CFG_EXP_FIXED_LOG_REC_SIZE "FIXED_LOG_REC_SIZE:" > >> #define CFG_EXP_LOG_FULL_ACTION "LOG_FULL_ACTION:" > >> > >> +/** > >> + * This macro is used to generate the retry machanism > >> + * which has the same effects as immutil_ utility, but thread safe. > >> + * > >> + * @param P ImmutilWrapperProfile_t > >> + * @param F IMM API > >> + * @param ... Arguments to IMM API > >> + * @return return the AIS error code > >> + * > >> + * NOTE: > >> + * This macro is not applied for saImmOmInitialize()/saImmOiInitialize() > >> + */ > >> +#define IMMUTIL_F(P, F, ...) > >> \ > >> + ({ > >> \ > >> + SaAisErrorT ais_ret_m = F(__VA_ARGS__); > >> \ > >> + unsigned int nTries_m = 1; > >> \ > >> + while (ais_ret_m == SA_AIS_ERR_TRY_AGAIN > >> \ > >> + && nTries_m < P.nTries) { > >> \ > >> + usleep(P.retryInterval * 1000); > >> \ > >> + ais_ret_m = F(__VA_ARGS__); > >> \ > >> + nTries_m++; > >> \ > >> + } > >> \ > >> + if (ais_ret_m != SA_AIS_OK && P.errorsAreFatal) > >> \ > >> + logError("(%s) %s FAILED, rc = %d", __func__, > >> #F, (int) ais_ret_m); \ > >> + ais_ret_m; > >> \ > >> + }) > >> + > >> /* > >> > ========================================================== > >> ============== > >> * TYPE DEFINITIONS > >> * > >> > ========================================================== > >> ============== > >> */ > >> > >> +/** > >> + * Controls the behavior of the wrapper functions. > >> + */ > >> +typedef struct lgs_ImmutilWrapperProfile { > >> + int errorsAreFatal; > >> + /**< If set the calling program is aborted on an error return > >> from SAF. */ > >> + unsigned int nTries; > >> + /**< Number of tries before we give up. */ > >> + unsigned int retryInterval; > >> + /**< Interval in milli-seconds between tries. */ > >> + lgs_ImmutilWrapperProfile() { > >> + errorsAreFatal = 1; > >> + nTries = 5; > >> + retryInterval = 400; > >> + } > >> +} ImmutilWrapperProfile_t; > >> + > >> /* > >> > ========================================================== > >> ============== > >> * DATA DECLARATIONS > >> * > >> > ========================================================== > >> ============== > >> @@ -80,5 +125,6 @@ bool lgs_is_valid_pathlength(const std:: > >> /* Timer functions */ > >> int lgs_init_timer(time_t timeout_s); > >> void lgs_close_timer(int ufd); > >> +void logError(char const *fmt, ...); > >> > >> #endif /* ifndef __LGS_UTIL_H */ ------------------------------------------------------------------------------ Mobile security can be enabling, not merely restricting. Employees who bring their own devices (BYOD) to work are irked by the imposition of MDM restrictions. Mobile Device Manager Plus allows you to control only the apps on BYO-devices by containerizing them, leaving personal data untouched! https://ad.doubleclick.net/ddm/clk/304595813;131938128;j _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
