Hi! Some comments:
1) Have you considered using std::map instead of adding a new instance of NCS_PATRICIA_TREE? 2) At least when adding completely new files (lgs_clm.cc in this case), could you try to keep them "clean" from cppcheck and cpplint warnings (i.e. make sure there are no warnings in newly added files)? 3) You have added the new file lgs_clm.cc, but there is no corresponding lgs_clm.h header file. Could you also add the file lgs_clm.h and put the public API of lgs_clm.cc in it? 4) lgs_clm.cc has only one single #include statement, which includes "lgs.h". But it uses many interfaces that are not declared in lgs.h. You should not rely on header files being indirectly included from other header files. lgs_clm.cc ought to include all headers that it needs, for example: "saClm.h" for the CLM API, <cstdint> for uint32_t et.al., "logtrace.h" for the OpenSAF trace API, <cstdlib> for the exit() function and the EXIT_FAILURE constant, <pthread.h> for the pthreads API. 5) Isn't the variable name usr2_sel_obj misleading? There is another variable with the very similar name usr1_sel_obj, and it is a selection object which is used when the process receives the SIGUSR1 signal. I would have expected that usr2_sel_obj would correspondingly be used when the process receives the SIGUSR2 signal. However, it seems that instead it is actually used to signal an MDS up event for AMFD. I think it ought to be renamed to reflect this. Or maybe even get rid of this selection object and instead post a message in the mailbox when we get this MDS event? 6) Why is the usr2_sel_obj created only in the case the when the LOG service is started by NID? This only makes sense for the usr1_sel_obj which is used to signal AMF instantiation when the process has already been started by NID. To me it looks like the code currently doesn't work when it is not started by NID - in this case clmSelectionObject will be initialized to -1 and stay that way forever. 7) I suppose the whole reason for triggering CLM initalization after MDS up event from AMFD, using this usr2_sel_obj variable, is to solve some circular dependency problem. But is it at all needed when CLM initialization is taken care of in a separate thread? Wouldn't it work fine if you simply start this CLM initialization thread when you receive an AMF assignment (i.e. start the thread from inside the initialize_for_assignment() function)? Could you try it? :-) 8) In C++, please use nullptr instead of NULL and 0 (when the type is supposed to be a pointer). So e.g. clm_callbacks should have have nullptr instead if 0 in its first position, and lgs_clm_init_thread ought to return nullptr instead of NULL. thanks, Anders Widell On 07/26/2016 10:42 AM, [email protected] wrote: > Summary:lgsv: Log Service CLM integration [#1638] V1 > Review request for Trac Ticket(s): #1638 > Peer Reviewer(s): Vu, Lennart > Pull request to: <<LIST THE PERSON WITH PUSH ACCESS HERE>> > Affected branch(es): Default > Development branch: Default > > -------------------------------- > Impacted area Impact y/n > -------------------------------- > Docs n > Build system n > RPM/packaging n > Configuration files n > Startup scripts n > SAF services y > OpenSAF services n > Core libraries n > Samples n > Tests n > Other n > > > Comments (indicate scope for each "y" above): > --------------------------------------------- > > changeset 0637f0bfc39b5173776b3cdca088cffd28425432 > Author: A V Mahesh <[email protected]> > Date: Tue, 26 Jul 2016 14:07:35 +0530 > > lga: agent Cluster Membership (CLM) integration [#1638] V1 Description: > > Form CLM integration is supported from Log Service A.02.02. > > At-least a A.02.02 LGA client will check CLM membership status of > client's > node. old LGA clients A.02.01 are always clm member. > > This patch enhanced the log service for Unavailability of the Log > Service > API on a Non-Member Node which will fail with SA_AIS_ERR_UNAVAILABLE. > > After this patch the Log Service does not provide service to processes > on > cluster nodes that are not in the cluster membership. > > If the node rejoins the cluster membership, processes executing on the > node > will be able to reinitialize new library handles and use the entire set > of > Log Service APIs that operate on these new handles; however, invocation > of > APIs that operate on handles acquired by any process before the node > left > the membership will continue to fail with SA_AIS_ERR_UNAVAILABLE (or > with > the special treatment described above for asynchronous calls) with the > exception of saLogFinalize(), which is used to free the library handles > and > all resources associated with these handles. Hence, it is recommended > for > the processes to finalize the library handles as soon as the processes > detect that the node left the membership. > > Detailed README will be provide soon. > > Following are expected Log Service API behavior : > > Case1: On Non-Member Node, Log Service API will fail with code > SA_AIS_ERR_UNAVAILABLE (31) Case2: On Member Node after recovered from > Non- > Member Node, Log Service API will fail with code SA_AIS_ERR_UNAVAILABLE > (31) > Case3: Non-Member Node + (Headless) Log Service API will fail with code > SA_AIS_ERR_UNAVAILABLE (31) Case4: On Non-Member Node + (Headless) + > (Head > Joined) Log Service API will fail with code SA_AIS_ERR_UNAVAILABLE (31) > > changeset fed6d0af1a288640e078bdac58b00c0c79691e97 > Author: A V Mahesh <[email protected]> > Date: Tue, 26 Jul 2016 14:08:09 +0530 > > lgs: director Cluster Membership (CLM) integration [#1638] V1 > > changeset 6842daa277070d2ac2c80eb6c69271521a51d563 > Author: A V Mahesh <[email protected]> > Date: Tue, 26 Jul 2016 14:09:35 +0530 > > logtest: change related to Cluster Membership (CLM) integration [#1638] > V1 > Test : > > #amf-adm lock safNode=PL-4,safCluster=myClmCluster > > <saLogStreamOpen_2> failed with code SA_AIS_ERR_UNAVAILABLE (31). > > #amf-adm unlock safNode=PL-4,safCluster=myClmCluster > > <saLogStreamOpen_2> failed with code SA_AIS_ERR_UNAVAILABLE (31). > > #amf-adm lock safNode=PL-4,safCluster=myClmCluster + (SC-1 & Sc-2 > /etc/init.d/opensafd stop) > > <saLogStreamOpen_2> failed with code SA_AIS_ERR_UNAVAILABLE (31) > > # amf-adm lock safNode=PL-4,safCluster=myClmCluster + ((SC-1 & Sc-2 > /etc/init.d/opensafd stop)) + ((SC-1 & Sc-2 /etc/init.d/opensafd > start) + amf-adm unlock safNode=PL-4,safCluster=myClmCluster > > <saLogStreamOpen_2> failed with code SA_AIS_ERR_UNAVAILABLE (31) > > Expected but currently The Imm APIs are NOT working error - > saImmOmInitialize FAILED: SA_AIS_ERR_LIBRARY (2) > > > Complete diffstat: > ------------------ > osaf/libs/agents/saf/lga/lga.h | 2 + > osaf/libs/agents/saf/lga/lga_api.c | 37 > +++++++++++++++++++++++++++++++++++ > osaf/libs/agents/saf/lga/lga_mds.c | 75 > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > osaf/libs/agents/saf/lga/lga_util.c | 2 + > osaf/libs/common/logsv/include/lgsv_defs.h | 6 ++++- > osaf/libs/common/logsv/include/lgsv_msg.h | 17 ++++++++++++--- > osaf/services/saf/logsv/lgs/Makefile.am | 4 ++- > osaf/services/saf/logsv/lgs/lgs.h | 5 ++++ > osaf/services/saf/logsv/lgs/lgs_cb.h | 15 ++++++++++++++ > osaf/services/saf/logsv/lgs/lgs_clm.cc | 143 > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > osaf/services/saf/logsv/lgs/lgs_evt.cc | 142 > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > osaf/services/saf/logsv/lgs/lgs_evt.h | 1 + > osaf/services/saf/logsv/lgs/lgs_main.cc | 29 > +++++++++++++++++++++++++++ > osaf/services/saf/logsv/lgs/lgs_mds.cc | 41 > +++++++++++++++++++++++++++++++++++++- > osaf/services/saf/logsv/lgs/lgs_util.cc | 83 > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > tests/logsv/saflogtest.c | 2 +- > 16 files changed, 595 insertions(+), 9 deletions(-) > > > Testing Commands: > ----------------- > <<LIST THE COMMAND LINE TOOLS/STEPS TO TEST YOUR CHANGES>> > > > Testing, Expected Results: > -------------------------- > <<PASTE COMMAND OUTPUTS / TEST RESULTS>> > > > Conditions of Submission: > ------------------------- > <<HOW MANY DAYS BEFORE PUSHING, CONSENSUS ETC>> > > > Arch Built Started Linux distro > ------------------------------------------- > mips n n > mips64 n n > x86 n n > x86_64 y y > 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. > > > ------------------------------------------------------------------------------ > What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic > patterns at an interface-level. Reveals which users, apps, and protocols are > consuming the most bandwidth. Provides multi-vendor support for NetFlow, > J-Flow, sFlow and other flows. Make informed decisions using capacity planning > reports.http://sdm.link/zohodev2dev > _______________________________________________ > Opensaf-devel mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/opensaf-devel > ------------------------------------------------------------------------------ What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic patterns at an interface-level. Reveals which users, apps, and protocols are consuming the most bandwidth. Provides multi-vendor support for NetFlow, J-Flow, sFlow and other flows. Make informed decisions using capacity planning reports.http://sdm.link/zohodev2dev _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
