Hi Hoa Le, Ack. See my comments. I think at least 1. has to be fixed but think about doing something about the other things as well No re-review is needed
For detailed comments see the attached diff files [Lennart]. Comments summary: 1. Remove all out-commented code. Is still available in older versions if needed again. 2. As a general rule, improve code documentation for at least functions touched in fixes. For example rename function to give them descriptive names, explain hard to understand functions etc. 3. The test case documentation should be done as a comment in the beginning of each test case and in a general way describe what's tested. It shall also be very clear what functions that are test cases and which are helper functions. Should be clearly separated. Helper functions shall be given descriptive names. A descriptive name gives brief information about WHAT the function is doing. The descriptive name is most useful where the function is called, makes that code easier to read. Test case numbers are not needed in comments an printouts (holds no interesting information). Printouts in the test case are normally not needed. The printout given by the text string in the test_suite_add() and test_case_add() is enough for knowing test progress. Thanks Lennart > -----Original Message----- > From: Hoa Le <hoa...@dektech.com.au> > Sent: den 23 augusti 2018 04:26 > To: Lennart Lund <lennart.l...@ericsson.com>; Minh Hon Chau > <minh.c...@dektech.com.au>; Hans Nordebäck > <hans.nordeb...@ericsson.com> > Cc: opensaf-devel@lists.sourceforge.net; Hoa Le <hoa...@dektech.com.au> > Subject: [PATCH 0/2] Review Request for mdstest: correct timing issues in > mdstest V3 [#2798] > > Summary: mdstest: identify svcs using svc_id and mds_dest when storing > event info [#2798] > Review request for Ticket(s): 2798 > Peer Reviewer(s): Hans, Lennart, Minh > Pull request to: Hans, Lennart, Minh > Affected branch(es): develop > Development branch: ticket-2798 > Base revision: 998c5b0f0aa14900c9408bc703ea3c2e99b854e8 > Personal repository: git://git.code.sf.net/u/xhoalee/review > > -------------------------------- > Impacted area Impact y/n > -------------------------------- > Docs n > Build system n > RPM/packaging n > Configuration files n > Startup scripts n > SAF services n > OpenSAF services n > Core libraries n > Samples n > Tests y > Other n > > NOTE: Patch(es) contain lines longer than 80 characers > > Comments (indicate scope for each "y" above): > --------------------------------------------- > > > revision c8ef4483ece445e281cb7bdeac3879cfd0a2c878 > Author: Hoa Le <hoa...@dektech.com.au> > Date: Thu, 23 Aug 2018 09:21:09 +0700 > > mdstest: correct timing issues in mdstest [#2798] > > In some bad thread scheduling situations, the API service request > in the testing thread may be executed before the corresponding > event being received on the MDS thread. This will lead to the > unexpected behavior of the service request and cause the failure > in this test case. > > This patch helps avoid the above issue by waiting for the expected > event being received on MDS thread before invoking the testing > service request. > > > > revision a67972060346225931e8c9fd89589ea9bc52e332 > Author: Hoa Le <hoa...@dektech.com.au> > Date: Thu, 23 Aug 2018 09:21:09 +0700 > > mdstest: identify svcs using svc_id and mds_dest when storing event info > [#2798] > > Currently, when updating the last event info, mdstest identify services > using their svc_id. This will cause confusion when several services was > installed with the same svc_id (on different mds_dest-s). If a service > subscribes to this svc_id, the service will retrieve several event info > with the same svc_id. When storing these event info to svcevt array, the > info are overwritten one by one and only the last info will be stored. > > This patch helps avoid the above situation by identifying these > services using both their svc_id and mds_dest. This helps the event > info, from different service, be separatedly stored to svcevt array. > subscr_count value will also be updated in accordance with these > event info. > > > > Complete diffstat: > ------------------ > src/mds/apitest/mdstipc_api.c | 410 +++++++++++++++++++++++++++---- > ---------- > src/mds/apitest/mdstipc_conf.c | 164 +++++++++++++++-- > 2 files changed, 421 insertions(+), 153 deletions(-) > > > Testing Commands: > ----------------- > mdstest 4 10 > mdstest 4 12 > mdstest 5 1 > mdstest 5 9 > mdstest 10 1 > mdstest 10 2 > mdstest 14 5 > mdstest 14 6 > > > Testing, Expected Results: > -------------------------- > No failure appears. > > > Conditions of Submission: > ------------------------- > ACK from reviewer. > > > 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.
mds_2798_elunlen_comments_v3.diff
Description: mds_2798_elunlen_comments_v3.diff
------------------------------------------------------------------------------ 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 Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel