Hi Canh
My comments are very old, October 2015 and they are mostly about list handling
(see attached mail). Since then the log server has been refactored to C++ so
C++ tools for lists can be used.
There seems to be a lot of mail conversations with included patches etc. and
since I haven't been following this I have some problem understanding what to
review. Can you please send out a review request containing the latest patches
that shall actually be reviewed.
Thanks
Lennart
> -----Original Message-----
> From: A V Mahesh [mailto:mahesh.va...@oracle.com]
> Sent: den 21 juni 2016 08:49
> To: Canh Van Truong <canh.v.tru...@dektech.com.au>; Vu Minh Nguyen
> <vu.m.ngu...@dektech.com.au>; Lennart Lund
> <lennart.l...@ericsson.com>
> Cc: opensaf-devel@lists.sourceforge.net
> Subject: Re: [PATCH 1 of 1] log: fix issue with priority of messages adding to
> mailbox in the server[1396]
>
> Hi Canh,
>
> It seems Lennart already provide comments please check.
>
> -AVM
>
>
> On 6/21/2016 12:07 PM, Canh Truong wrote:
> > Hi Mahesh,
> >
> > Thanks for your comments. I will send new version after Lennart
> comments.
> > I have checked your idea about multiple thread test case. The idea may be
> same with patch that I sent it before.
> >
> > Regards,
> > Canh.
> >
> > -----Original Message-----
> > From: A V Mahesh [mailto:mahesh.va...@oracle.com]
> > Sent: Tuesday, June 21, 2016 11:47 AM
> > To: Canh Truong; vu.m.ngu...@dektech.com.au;
> lennart.l...@ericsson.com
> > Cc: opensaf-devel@lists.sourceforge.net
> > Subject: Re: [PATCH 1 of 1] log: fix issue with priority of messages adding
> > to
> mailbox in the server[1396]
> >
> > Hi Canh,
> >
> > It will be good to send V2 patch with Lennart comments and adding new
> test cases.
> >
> > -AVM
> > On 6/21/2016 9:46 AM, A V Mahesh wrote:
> >> Hi Canh,
> >>
> >> Some how we were not able to open your attachment , so my self
> >> written
> >>
> >> a multi threaded test case ( attached `thread_LogInitializeFinalize.c`
> >> ) and reviewed/tested it is working fine.
> >>
> >> ACK from me with following :
> >>
> >> - Please add such as attached multi threaded test case to logtest,
> >> before pushing
> >>
> >> - add # before ticket number in commit message #1396
> >>
> >> -AVM
> >>
> >>
> >> On 6/14/2016 2:47 PM, Canh Truong wrote:
> >>> Hi Mahesh,
> >>>
> >>> I have created new test case for multiple threads. Please help check
> >>> new patch in attachment.
> >>>
> >>> Thanks,
> >>> Canh.
> >>>
> >>> -----Original Message-----
> >>> From: A V Mahesh [mailto:mahesh.va...@oracle.com]
> >>> Sent: Tuesday, June 14, 2016 11:03 AM
> >>> To: Canh Van Truong; vu.m.ngu...@dektech.com.au;
> >>> lennart.l...@ericsson.com
> >>> Cc: opensaf-devel@lists.sourceforge.net
> >>> Subject: Re: [PATCH 1 of 1] log: fix issue with priority of messages
> >>> adding to mailbox in the server[1396]
> >>>
> >>> Hi Canh Van Truong,
> >>>
> >>> I hope this patch will work for multiple threads doing
> >>> saLogInitialize() , saLogStreamOpen_2() and saLogFinalize()
> >>> concurrently in a single process and reaches lga_use_count = 0 ,
> >>> againg restarting
> >>> saLogInitialize() , saLogStreamOpen_2() and
> >>> saLogFinalize()
> >>> without process exist.
> >>>
> >>> If so let us have a test case like for multiple threads doing
> >>> saLogInitialize() , saLogStreamOpen_2() and saLogFinalize()
> >>> concurrently
> >>>
> >>> -AVM
> >>>
> >>>
> >>> On 6/13/2016 12:37 PM, Canh Van Truong wrote:
> >>>> osaf/libs/agents/saf/lga/lga_api.c | 3 ++-
> >>>> osaf/libs/agents/saf/lga/lga_util.c | 21 ++++++++++++---------
> >>>> tests/logsv/logtest.h | 1 +
> >>>> tests/logsv/tet_saLogStreamOpen_2.c | 29
> >>>> +++++++++++++++++++++++++++++
> >>>> 4 files changed, 44 insertions(+), 10 deletions(-)
> >>>>
> >>>>
> >>>> Sometimes, Log service can handle LGSV_INITIALIZE_REQ before
> handle
> >>>> LGSV_LGS_EVT_LGA_DOWN, although LGSV_LGS_EVT_LGA_DOWN
> was sent first
> >>>> whenever
> >>>> finalize() is call for last client. Because lgs can not distinguish
> >>>> MDS address between new connection (initializing new client) and old
> >>>> connection (finalizing last client before). This patch fixes to keep
> >>>> MDS connection when last client finalized, and re-use it when next
> >>> initialize() called.
> >>>> diff --git a/osaf/libs/agents/saf/lga/lga_api.c
> >>>> b/osaf/libs/agents/saf/lga/lga_api.c
> >>>> --- a/osaf/libs/agents/saf/lga/lga_api.c
> >>>> +++ b/osaf/libs/agents/saf/lga/lga_api.c
> >>>> @@ -41,7 +41,8 @@
> >>>> /* The main controle block */
> >>>> lga_cb_t lga_cb = {
> >>>> .cb_lock = PTHREAD_MUTEX_INITIALIZER,
> >>>> - .lgs_state = LGS_START
> >>>> + .lgs_state = LGS_START,
> >>>> + .mds_hdl = 0
> >>>> };
> >>>> static bool is_well_know_stream(const char* dn) diff --git
> >>>> a/osaf/libs/agents/saf/lga/lga_util.c
> >>>> b/osaf/libs/agents/saf/lga/lga_util.c
> >>>> --- a/osaf/libs/agents/saf/lga/lga_util.c
> >>>> +++ b/osaf/libs/agents/saf/lga/lga_util.c
> >>>> @@ -308,21 +308,17 @@ unsigned int lga_startup(lga_cb_t *cb)
> >>>> osaf_mutex_lock_ordie(&lga_lock);
> >>>> TRACE_ENTER2("lga_use_count: %u", lga_use_count);
> >>>> - if (lga_use_count > 0) {
> >>>> - /* Already created, just increment the use_count */
> >>>> - lga_use_count++;
> >>>> - goto done;
> >>>> - } else {
> >>>> +
> >>>> + if (cb->mds_hdl == 0) {
> >>>> if ((rc = ncs_agents_startup()) != NCSCC_RC_SUCCESS) {
> >>>> TRACE("ncs_agents_startup FAILED");
> >>>> goto done;
> >>>> }
> >>>> if ((rc = lga_create()) != NCSCC_RC_SUCCESS) {
> >>>> + cb->mds_hdl = 0;
> >>>> ncs_agents_shutdown();
> >>>> goto done;
> >>>> - } else {
> >>>> - lga_use_count = 1;
> >>>> }
> >>>> /* Agent has successfully been started including
> >>> communication @@
> >>>> -331,6 +327,9 @@ unsigned int lga_startup(lga_cb_t *cb)
> >>>> set_lga_state(LGA_NORMAL);
> >>>> }
> >>>> + /* Increase the use_count */
> >>>> + lga_use_count++;
> >>>> +
> >>>> done:
> >>>> osaf_mutex_unlock_ordie(&lga_lock);
> >>>> @@ -361,8 +360,12 @@ unsigned int lga_shutdown_after_last_cli
> >>>> /* Users still exist, just decrement the use count */
> >>>> lga_use_count--;
> >>>> } else if (lga_use_count == 1) {
> >>>> - lga_destroy();
> >>>> - rc = ncs_agents_shutdown();
> >>>> + /*
> >>>> + * Disable MDS_UNINSTALL and ncs agent shutdown to keep MDS
> >>> connection.
> >>>> + * Fix ticket 1396. Msg NCSMDS_DOWN and Initialize are out
> >>> of order in Mbx.
> >>>> + */
> >>>> + /* lga_destroy(); */
> >>>> + /* rc = ncs_agents_shutdown(); */
> >>>> lga_use_count = 0;
> >>>> }
> >>>> diff --git a/tests/logsv/logtest.h b/tests/logsv/logtest.h
> >>>> --- a/tests/logsv/logtest.h
> >>>> +++ b/tests/logsv/logtest.h
> >>>> @@ -27,6 +27,7 @@
> >>>> #include <utest.h>
> >>>> #include <util.h>
> >>>> #include <sys/wait.h>
> >>>> +#include <unistd.h>
> >>>> #include <osaf_time.h>
> >>>> #include <logtrace.h>
> >>>> diff --git a/tests/logsv/tet_saLogStreamOpen_2.c
> >>>> b/tests/logsv/tet_saLogStreamOpen_2.c
> >>>> --- a/tests/logsv/tet_saLogStreamOpen_2.c
> >>>> +++ b/tests/logsv/tet_saLogStreamOpen_2.c
> >>>> @@ -704,6 +704,34 @@ done:
> >>>> }
> >>>> }
> >>>> +/*
> >>>> + * Ticket 1396
> >>>> + * Verify that saLogInitialize() then saLogFinalize() multiple time
> >>>> +OK */ void saLogMultipleInitialize(void) {
> >>>> + SaLogStreamHandleT logStreamHandle1;
> >>>> + int count = 100;
> >>>> + SaAisErrorT rc = SA_AIS_OK;
> >>>> +
> >>>> + while (count--) {
> >>>> + rc = saLogInitialize(&logHandle, &logCallbacks,
> >>> &logVersion);
> >>>> + if (rc != SA_AIS_OK)
> >>>> + break;
> >>>> +
> >>>> + rc = saLogStreamOpen_2(logHandle, &app1StreamName,
> >>> &appStream1LogFileCreateAttributes,
> >>>> + SA_LOG_STREAM_CREATE,
> >>> SA_TIME_ONE_SECOND, &logStreamHandle1);
> >>>> + safassert(rc, SA_AIS_OK);
> >>>> +
> >>>> + rc = saLogFinalize(logHandle);
> >>>> + if (rc != SA_AIS_OK)
> >>>> + break;
> >>>> +
> >>>> + }
> >>>> + test_validate(rc, SA_AIS_OK);
> >>>> +
> >>>> +}
> >>>> +
> >>>> extern void saLogStreamOpenAsync_2_01(void);
> >>>> extern void saLogStreamOpenCallbackT_01(void);
> >>>> extern void saLogWriteLog_01(void); @@ -790,5 +818,6 @@ extern
> >>>> void saLogStreamClose_01(void);
> >>>> test_case_add(2, verFixLogRec_Max_Err, "saLogStreamOpen_2
> >>>> with
> >>> maxLogRecordSize > MAX_RECSIZE, ERR");
> >>>> test_case_add(2, verFixLogRec_Min_Err, "saLogStreamOpen_2
> >>>> with
> >>> maxLogRecordSize < 150, ERR");
> >>>> test_case_add(2, saLogStreamOpen_2_50, "saLogStreamOpen_2
> >>>> with stream number out of the limitation, ERR");
> >>>> + test_case_add(2, saLogMultipleInitialize, "saLogInitialize()
> >>>> + then
> >>>> + saLogFinalize() multiple time. keep MDS connection, OK");
> >>>> }
--- Begin Message ---
Hi Giang Do
I have some comments:
1.
Your list is a unnecessarily complicated a simpler list handling can be used
also search code is duplicated.
Can be found in both search function and remove function.
2.
I saw that you have used lga_ as prefix for the list handling functions. This
is not correct. The prefixes used
have a specific meaning.
lga_ Is used for files and external functions used with the log agent
lgs_ Is used for files and external functions used with the log server
If the log service instead of a server should have a director and a node
director the corresponding prefixes would have been:
lgd_ For director
lgnd_ For node director
3.
There is another list which seems to be used for a similar purpose in standby
state. To avoid redundant code this list should
be replaced with the new list handling. This will remove the incorrectly used
global variables in the lgs_cb structure, duplicated
inline list handling code and also implement a better list handling that can be
unit tested.
Thanks
Lennart
> -----Original Message-----
> From: giang do [mailto:giang.t...@dektech.com.au]
> Sent: den 26 oktober 2015 09:59
> To: mathi.naic...@oracle.com; Vu Minh Nguyen; Lennart Lund
> Cc: opensaf-devel@lists.sourceforge.net
> Subject: [PATCH 0 of 1] Review Request for log: issue with priority of
> messages adding to mailbox in the server [#1396]
>
> Summary: 1396 log: issue with priority of messages adding to mailbox in the
> server [#1396]
> Review request for Trac Ticket(s): 1396
> Peer Reviewer(s): mathi.naic...@oracle.com,
> vu.m.ngu...@dektech.com.au, lennart.l...@ericsson.com
> Pull request to: Lennart
> Affected branch(es): 5.0
> Development branch: <<IF ANY GIVE THE REPO URL>>
>
> --------------------------------
> Impacted area Impact y/n
> --------------------------------
> Docs n
> Build system n
> RPM/packaging n
> Configuration files n
> Startup scripts n
> SAF services n
> OpenSAF services y
> Core libraries n
> Samples n
> Tests n
> Other n
>
>
> Comments (indicate scope for each "y" above):
> ---------------------------------------------
> <<EXPLAIN/COMMENT THE PATCH SERIES HERE>>
>
> changeset 7e1d429bd182875b26badad3b58cf3fb0f380e65
> Author: giang do<giang.t...@dektech.com.au>
> Date: Mon, 21 Sep 2015 17:03:25 +0700
>
> log: issue with priority of messages adding to mailbox in the
> server [#1396]
>
> Sometimes, Log service can handle LGSV_INITIALIZE_REQ
> before handle
> LGSV_LGS_EVT_LGA_DOWN, although
> LGSV_LGS_EVT_LGA_DOWN was sent first.
>
> When Log service handle down message later, it will destroy
> every log agents
> (include new created log agent). From now, handle of this log
> agent in log
> user is not valid anymore.
>
> Solution: Log service need to prevent log user create new log
> agent when Log
> service have not handled down message completely. Just tell
> user try again.
>
>
> Added Files:
> ------------
> osaf/services/saf/logsv/lgs/lgs_mdsdown.c
> osaf/services/saf/logsv/lgs/lgs_mdsdown.h
>
>
> Complete diffstat:
> ------------------
> osaf/services/saf/logsv/lgs/Makefile.am | 3 ++-
> osaf/services/saf/logsv/lgs/lgs_evt.c | 14 ++++++++++++++
> osaf/services/saf/logsv/lgs/lgs_mds.c | 7 +++++++
> osaf/services/saf/logsv/lgs/lgs_mdsdown.c | 133
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> +++++++++++++++++
> osaf/services/saf/logsv/lgs/lgs_mdsdown.h | 56
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> tests/logsv/tet_saLogInitialize.c | 26 ++++++++++++++++++++++++++
> 6 files changed, 238 insertions(+), 1 deletions(-)
>
>
> Testing Commands:
> -----------------
> logtest
>
> Testing, Expected Results:
> --------------------------
> all testcases passed
>
> Conditions of Submission:
> -------------------------
> Acked from peer reviewers
>
> Arch Built Started Linux distro
> -------------------------------------------
> mips n n
> mips64 n n
> x86 n n
> x86_64 n n
> powerpc n n
> powerpc64 n n
>
>
> Reviewer Checklist:
> -------------------
> [Submitters: make sure that your review doesn't trigger any checkmarks!]
>
>
> Your checkin has not passed review because (see checked entries):
>
> ___ Your RR template is generally incomplete; it has too many blank entries
> that need proper data filled in.
>
> ___ You have failed to nominate the proper persons for review and push.
>
> ___ Your patches do not have proper short+long header
>
> ___ You have grammar/spelling in your header that is unacceptable.
>
> ___ You have exceeded a sensible line length in your
> headers/comments/text.
>
> ___ You have failed to put in a proper Trac Ticket # into your commits.
>
> ___ You have incorrectly put/left internal data in your comments/files
> (i.e. internal bug tracking tool IDs, product names etc)
>
> ___ You have not given any evidence of testing beyond basic build tests.
> Demonstrate some level of runtime or other sanity testing.
>
> ___ You have ^M present in some of your files. These have to be removed.
>
> ___ You have needlessly changed whitespace or added whitespace crimes
> like trailing spaces, or spaces before tabs.
>
> ___ You have mixed real technical changes with whitespace and other
> cosmetic code cleanup changes. These have to be separate commits.
>
> ___ You need to refactor your submission into logical chunks; there is
> too much content into a single commit.
>
> ___ You have extraneous garbage in your review (merge commits etc)
>
> ___ You have giant attachments which should never have been sent;
> Instead you should place your content in a public tree to be pulled.
>
> ___ You have too many commits attached to an e-mail; resend as threaded
> commits, or place in a public tree for a pull.
>
> ___ You have resent this content multiple times without a clear indication
> of what has changed between each re-send.
>
> ___ You have failed to adequately and individually address all of the
> comments and change requests that were proposed in the initial review.
>
> ___ You have a misconfigured ~/.hgrc file (i.e. username, email etc)
>
> ___ Your computer have a badly configured date and time; confusing the
> the threaded patch review.
>
> ___ Your changes affect IPC mechanism, and you don't present any results
> for in-service upgradability test.
>
> ___ Your changes affect user manual and documentation, your patch series
> do not contain the patch that updates the Doxygen manual.
------------------------------------------------------------------------------
_______________________________________________
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
--- End Message ---
------------------------------------------------------------------------------
_______________________________________________
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
