Hi Minh, Some comments:
TEST: Test does not give correct info for manual testing and as far as I can see there is no information about how to run it in a test environment like Osaftest. 1. Incorrect an not needed information saying that rda_ge_role failed. Payload node. Run CLM tests. This information should not be printed at all since it gives no information that is needed for handling the test case. Also it is incorrect, seems to be some cut and paste from the CLM test suite. 2. Information about node start/stop and switch over is incorrect. After standby node is stopped it must be started again before a switch over can be done Also there is no information about what is actually tested. This information is for example needed when documentation about what's tested in OpenSAF is created DESIGN: When a new feature is implemented it is always based on some sort of design. This design should be described. If the design concerns several files it can be described in a README file. The description should tell what the new feature is, the general idea that is implemented, what is added and changed and maybe what files that are affected etc. In this case; Is anything changed in the current handling and why? What is added and why and how is it working in general terms e.g. is there a new protocol for cold sync? What does the new protocol looks like etc. IMPLEMENTATION: New functions are added where it is not obvious what they are doing, when they are used, if they are dependent on anything etc. Some functions do have a function header but the header contains the wrong information, not enough information or there is no header. This means that the code is not easy to review, will be hard (costly) to maintain and it is not easy to get things right (see example for possible bug). We should not introduce any new functions that are not correctly documented and if old functions are used and they are not correctly documented they have to be analyzed and base on that documented. This activity will help to prevent possible mistakes as in the example below. It will also make the code easier to maintain. An example: When writing this I found that notifications maybe now are logged twice? In that case a bug is introduced /** * A cached notification is received in Cold Sync * * @param clientId Node-wide unique id for the client who sent the notifiaction. * @param notificationType * Type of the notification (alarm, object change etc.). * @param sendNotInfo * Pointer to the struct that holds information about the * notification. */ void NtfAdmin::cachedNotificationReceivedColdSync(... This header says that this function receives a cached notification in Col Sync. This is not what this function is doing. Also the name of the function is misleading It is impossible to know what this function is doing without analyzing the code and not only the code in this function. What it is actually doing is that it creates a notification object on the heap and the pointer to this object is a shared (old type that should no longer be used) pointer. This pointer is given to a function, "logger.log(notification, false)" which is logging the notification using the log service but that is not what we want to do here since I assume this " cachedNotificationReceivedColdSync()" function is called on the standby node? If that's the case the notification will be logged twice. The "logger.log()" function also adds the given pointer to some list of notifications that I assume is used with the reader API and that is probably what we want to do. Some guidance for how to document code can be found in the Google style guide Thanks Lennart > -----Original Message----- > From: Minh Chau [mailto:[email protected]] > Sent: den 4 januari 2018 07:36 > To: Lennart Lund <[email protected]>; > [email protected]; Canh Van Truong > <[email protected]> > Cc: [email protected]; Minh Hon Chau > <[email protected]> > Subject: [PATCH 0/2] Review Request for ntf: Support cold sync for cached > alarms [#2375] > > Summary: ntfd: Add cached alarms for cold sync [#2735] > Review request for Ticket(s): 2735 > Peer Reviewer(s): Lennart, Srinivas, Canh > Pull request to: *** LIST THE PERSON WITH PUSH ACCESS HERE *** > Affected branch(es): develop > Development branch: ticket-2735 > Base revision: 60f929be8d7414257ecf90461f923b6f40ed3ac7 > Personal repository: git://git.code.sf.net/u/minh-chau/review > > -------------------------------- > 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 > > NOTE: Patch(es) contain lines longer than 80 characers > > Comments (indicate scope for each "y" above): > --------------------------------------------- > *** EXPLAIN/COMMENT THE PATCH SERIES HERE *** > > revision 725139c5df3cc0e208679f18a73ef874c5302755 > Author: Minh Chau <[email protected]> > Date: Thu, 4 Jan 2018 17:28:25 +1100 > > ntftest: Add new test case for cold sync of cached alarms [#2735] > > Patch add new test case of new suite 41 to test saNtfNotificationReadNext > after the cold sync of cached alarm > > > > revision 2e45ed8d798857c0613954a77f7b7169ca74f3bf > Author: Minh Chau <[email protected]> > Date: Wed, 3 Jan 2018 15:35:19 +1100 > > ntfd: Add cached alarms for cold sync [#2735] > > Patch adds the alarm and security alarm notifications to cold > sync towards the standby NTFD, increase mbcsv version to avoid > confusion with old NTFD > > > > Added Files: > ------------ > src/ntf/apitest/tet_coldsync.c > > > Complete diffstat: > ------------------ > src/ntf/Makefile.am | 3 +- > src/ntf/apitest/tet_coldsync.c | 211 > ++++++++++++++++++++++ > src/ntf/apitest/tet_ntf.h | 2 + > src/ntf/apitest/tet_ntf_common.c | 97 +++++++++- > src/ntf/apitest/tet_ntf_common.h | 2 + > src/ntf/apitest/tet_ntf_main.c | 3 + > src/ntf/apitest/tet_scOutage_reinitializeHandle.c | 77 +------- > src/ntf/ntfd/NtfAdmin.cc | 33 +++- > src/ntf/ntfd/NtfAdmin.h | 3 + > src/ntf/ntfd/NtfLogger.cc | 16 ++ > src/ntf/ntfd/NtfLogger.h | 1 + > src/ntf/ntfd/NtfNotification.cc | 15 ++ > src/ntf/ntfd/NtfNotification.h | 1 + > src/ntf/ntfd/ntfs_com.c | 11 ++ > src/ntf/ntfd/ntfs_com.h | 5 + > src/ntf/ntfd/ntfs_mbcsv.c | 45 ++++- > src/ntf/ntfd/ntfs_mbcsv.h | 3 +- > 17 files changed, 446 insertions(+), 82 deletions(-) > > > Testing Commands: > ----------------- > Run legacy ntftest and new test of suite 41 > > > Testing, Expected Results: > -------------------------- > All pass > > > Conditions of Submission: > ------------------------- > ack from reviewers > > > 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 ~/.gitconfig file (i.e. user.name, user.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. ------------------------------------------------------------------------------ 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
