Hi Canh See my comments inline tagged [Lennart]
Thanks Lennart > -----Original Message----- > From: Canh Van Truong > Sent: den 19 januari 2017 12:29 > To: Lennart Lund <[email protected]>; Vu Minh Nguyen > <[email protected]>; [email protected] > Cc: [email protected] > Subject: [PATCH 3 of 3] log: fix checking version in log agent [#2146] > > src/log/agent/lga_api.c | 55 > ++++++++++++++++++++------ > src/log/apitest/tet_saLogFilterSetCallbackT.c | 53 > ++++++++++++++++++++++++++ > src/log/apitest/tet_saLogInitialize.c | 23 +++++++++++ > 3 files changed, 117 insertions(+), 14 deletions(-) > > > Fix checking version in log agent > > The agent report highest supported version except getting ERR_VERSION > from > server. if agent get ERR_VERSION, it will set supported version to A.02.03 > > diff --git a/src/log/agent/lga_api.c b/src/log/agent/lga_api.c > --- a/src/log/agent/lga_api.c > +++ b/src/log/agent/lga_api.c > @@ -73,6 +73,36 @@ static bool is_lgs_state(lgs_state_t sta > return rc; > } > > +/** > + * Check if the log version is valid > + * @param version > + * @param client_ver > + * > + * @return true if log version is valid > + */ > +static bool log_version_is_valid(SaVersionT *ver, SaVersionT *client_ver) { > + bool rc = true; > + if ((ver->releaseCode == LOG_RELEASE_CODE) && > + (ver->majorVersion <= LOG_MAJOR_VERSION) && > + (ver->majorVersion > 0) && > + (ver->minorVersion <= LOG_MINOR_VERSION)) { > + *client_ver = *ver; > + if (ver->minorVersion < LOG_MINOR_VERSION_0) > + client_ver->minorVersion = > LOG_MINOR_VERSION_0; > + ver->majorVersion = LOG_MAJOR_VERSION; > + ver->minorVersion = LOG_MINOR_VERSION; > + } else { > + TRACE("version FAILED, required: %c.%u.%u, supported: > %c.%u.%u\n", > + ver->releaseCode, ver->majorVersion, ver- > >minorVersion, > + LOG_RELEASE_CODE, LOG_MAJOR_VERSION, > LOG_MINOR_VERSION); > + ver->releaseCode = LOG_RELEASE_CODE; > + ver->majorVersion = LOG_MAJOR_VERSION; > + ver->minorVersion = LOG_MINOR_VERSION; > + rc = false; > + } > + return rc; > +} > + [Lennart] This function is doing much more than "Check if the log version is valid". This means that both the name of the function and the description in the function header is misleading. What this function actually seems to do is to check the version given by client. The check is not generic, by that I mean that if e.g. release code is stepped the function has to be changed, and the only way to see that is by analyzing the code. If the check pass, the content of the ver structure is copied to client_ver structure and the ver structure is changed so that it contains the versions defined in lgsv_defs.h. If the check fails, ver is also updated but client_ver is not updated except for minor version if minor verion is 0. If we now look at the initialize api function where this function is used we can see that client_ver is used with the message sent to the log server and then we can guess that the server needs this information as it was given by the client before the version in parameter from the client was modified by the verify function except that it cannot handle minor version 0. I suggest that you create a "clean" versification function that only verifies the version. Its name should then be is_log_version_valid() (name should start with key word "is") its in parameter (the version to be verified) should be const since the function should not be allowed to change the value. All the modifications of client version and version sent to server shall be done if the verification pass and directly in the initialize api function where the modified values are used. Make it as clear as possible why the values are modified. > static void populate_open_params(lgsv_stream_open_req_t *open_param, > const char *logStreamName, > lga_client_hdl_rec_t *hdl_rec, > @@ -151,6 +181,7 @@ SaAisErrorT saLogInitialize(SaLogHandleT > int rc; > uint32_t client_id = 0; > bool is_locked = false; > + SaVersionT client_ver; > > TRACE_ENTER(); > > @@ -162,17 +193,7 @@ SaAisErrorT saLogInitialize(SaLogHandleT > } > > /* validate the version */ > - if ((version->releaseCode == LOG_RELEASE_CODE) && (version- > >majorVersion <= LOG_MAJOR_VERSION)) { > - version->majorVersion = LOG_MAJOR_VERSION; > - if (version->minorVersion != LOG_MINOR_VERSION_0) > - version->minorVersion = LOG_MINOR_VERSION; > - } else { > - TRACE("version FAILED, required: %c.%u.%u, supported: > %c.%u.%u\n", > - version->releaseCode, version->majorVersion, version- > >minorVersion, > - LOG_RELEASE_CODE, LOG_MAJOR_VERSION, > LOG_MINOR_VERSION); > - version->releaseCode = LOG_RELEASE_CODE; > - version->majorVersion = LOG_MAJOR_VERSION; > - version->minorVersion = LOG_MINOR_VERSION; > + if (!log_version_is_valid(version, &client_ver)) { > ais_rc = SA_AIS_ERR_VERSION; > goto done; > } > @@ -230,7 +251,7 @@ SaAisErrorT saLogInitialize(SaLogHandleT > memset(&i_msg, 0, sizeof(lgsv_msg_t)); > i_msg.type = LGSV_LGA_API_MSG; > i_msg.info.api_info.type = LGSV_INITIALIZE_REQ; > - i_msg.info.api_info.param.init.version = *version; > + i_msg.info.api_info.param.init.version = client_ver; > > /* Send a message to LGS to obtain a client_id/server ref id which is > cluster > * wide unique. > @@ -245,7 +266,13 @@ SaAisErrorT saLogInitialize(SaLogHandleT > /** Make sure the LGS return status was SA_AIS_OK > **/ [Lennart] This comment no longer describe what's going on here. The version information is also changed but why and to what? > ais_rc = o_msg->info.api_resp_info.rc; > - if (SA_AIS_OK != ais_rc) { > + if (ais_rc == SA_AIS_ERR_VERSION) { > + TRACE("%s LGS error response %s", __FUNCTION__, > saf_error(ais_rc)); > + version->releaseCode = LOG_RELEASE_CODE_1; > + version->majorVersion = LOG_RELEASE_CODE_1; > + version->minorVersion = LOG_RELEASE_CODE_1; /* TODO: > update when log server reply with version */ > + goto err; > + } else if (SA_AIS_OK != ais_rc) { > TRACE("%s LGS error response %s", __FUNCTION__, > saf_error(ais_rc)); > goto err; > } > @@ -265,7 +292,7 @@ SaAisErrorT saLogInitialize(SaLogHandleT > > /* pass the handle value to the appl */ > *logHandle = lga_hdl_rec->local_hdl; > - lga_hdl_rec->version = *version; > + lga_hdl_rec->version = client_ver; > > err: > /* free up the response message */ > diff --git a/src/log/apitest/tet_saLogFilterSetCallbackT.c > b/src/log/apitest/tet_saLogFilterSetCallbackT.c > --- a/src/log/apitest/tet_saLogFilterSetCallbackT.c > +++ b/src/log/apitest/tet_saLogFilterSetCallbackT.c > @@ -375,6 +375,58 @@ done: > systemCall(command); > } > > +void saLogFilterSetCallbackT_05(void) > +{ > + int ret; > + SaAisErrorT rc; > + struct pollfd fds[1]; > + char command[MAX_DATA]; > + const unsigned int serverity_filter = 7; > + > + SaVersionT old_version = { 'A', 0x02, 0x02 }; > + logCallbacks.saLogFilterSetCallback = logFilterSetCallbackT; > + rc = saLogInitialize(&logHandle, &logCallbacks, &old_version); > + if (rc != SA_AIS_OK) { > + test_validate(rc, SA_AIS_OK); > + return; > + } > + > + rc = saLogSelectionObjectGet(logHandle, &selectionObject); > + if (rc != SA_AIS_OK) { > + test_validate(rc, SA_AIS_OK); > + goto done; > + } > + > + rc = logAppStreamOpen(&app1StreamName, > &appStreamLogFileCreateAttributes); > + if (rc != SA_AIS_OK) { > + test_validate(rc, SA_AIS_OK); > + goto done; > + } > + > + cb_index = 0; > + sprintf(command, "immadm -o 1 -p > saLogStreamSeverityFilter:SA_UINT32_T:%u %s 2> /dev/null", > + serverity_filter, SA_LOG_STREAM_APPLICATION1); > + ret = systemCall(command); > + if (ret != 0) { > + rc_validate(ret, 0); > + goto done; > + } > + > + fds[0].fd = (int) selectionObject; > + fds[0].events = POLLIN; > + ret = poll(fds, 1, 1000); > + if (ret == 1) { > + fprintf(stderr, " poll log callback failed: %d \n", ret); > + rc_validate(ret, 0); > + goto done; > + } else { > + rc_validate(0, 0);; > + } > + > +done: > + logCallbacks.saLogFilterSetCallback = NULL; > + logFinalize(); > +} > > __attribute__ ((constructor)) static void > saLibraryLifeCycle_constructor(void) > { > @@ -383,4 +435,5 @@ done: > test_case_add(17, saLogFilterSetCallbackT_02, "saLogFilterSetCallbackT, > severity filter is changed for runtime stream"); > test_case_add(17, saLogFilterSetCallbackT_03, "saLogFilterSetCallbackT, > severity filter is changed for runtime & cfg streams"); > test_case_add(17, saLogFilterSetCallbackT_04, "saLogFilterSetCallbackT, > after closing stream"); > + test_case_add(17, saLogFilterSetCallbackT_05, "saLogFilterSetCallbackT, > if > client initialize with version < A.02.03, ER"); > } > diff --git a/src/log/apitest/tet_saLogInitialize.c > b/src/log/apitest/tet_saLogInitialize.c > --- a/src/log/apitest/tet_saLogInitialize.c > +++ b/src/log/apitest/tet_saLogInitialize.c > @@ -82,6 +82,27 @@ void saLogInitialize_09(void) > test_validate(rc, SA_AIS_ERR_VERSION); > } > > +void saLogInitialize_10(void) > +{ > + SaVersionT version = logVersion; > + > + version.minorVersion = logVersion.minorVersion + 1; > + rc = saLogInitialize(&logHandle, &logCallbacks, &version); > + logFinalize(); > + > + test_validate(rc, SA_AIS_ERR_VERSION); > +} > + > +void saLogInitialize_11(void) > +{ > + SaVersionT version = {'A', 2}; > + > + rc = saLogInitialize(&logHandle, &logCallbacks, &version); > + logFinalize(); > + > + test_validate(rc, SA_AIS_OK); > +} > + > extern void saLogSelectionObjectGet_01(void); > extern void saLogSelectionObjectGet_02(void); > extern void saLogFinalize_01(void); > @@ -100,6 +121,8 @@ extern void saLogDispatch_01(void); > test_case_add(1, saLogInitialize_07, "saLogInitialize() with too high > release > level"); > test_case_add(1, saLogInitialize_08, "saLogInitialize() with minor > version > set to 1"); > test_case_add(1, saLogInitialize_09, "saLogInitialize() with major > version > set to 3"); > + test_case_add(1, saLogInitialize_10, "saLogInitialize() with minor > version > is set bigger than supported version"); > + test_case_add(1, saLogInitialize_11, "saLogInitialize() with minor > version > is not set"); > test_case_add(1, saLogSelectionObjectGet_01, > "saLogSelectionObjectGet() OK"); > test_case_add(1, saLogSelectionObjectGet_02, > "saLogSelectionObjectGet() with NULL log handle"); > test_case_add(1, saLogDispatch_01, "saLogDispatch() OK"); ------------------------------------------------------------------------------ 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
