Hi, Some more comments from looking at libs/common
- I guess the tmp_common files should be removed? - d2nedu.c, memory is allocated with malloc in a couple of places. It is later freed from many locations by calling avsv_dnd_msg_free_TMP which then uses delete in a couple of places. There needs to be balance here. I guess it is easiest to only use new/delete everywhere? If the tmp_common files are removed there is no issue. So for now I think you could remove the tmp_common files and use the old functions for example avsv_dnd_msg_free() which is "balanced" with avsv_edp_dnd_msg. And in a separate patch change libs/comon to use new/delete Thanks, Hans On 23 October 2013 09:51, Hans Nordebäck <hans.nordeb...@ericsson.com> wrote: > Hi Hans, thanks, see comments below/BR HansN > > -----Original Message----- > From: Hans Feldt [mailto:osafde...@gmail.com] > Sent: den 23 oktober 2013 08:46 > To: Hans Nordebäck > Cc: Hans Feldt; praveen malviya; nagendr...@oracle.com; > opensaf-devel@lists.sourceforge.net > Subject: Re: [devel] [PATCH 0 of 1] Review Request for amf #94 > > Hi Hans, > > Nice work! > > I do have some comment that I think needs to be fixed. > > amfd: > - main.cc, function rda_cb, memory is allocated with malloc, this is later > returned with delete in avsv_d2d_msg_free() called from > avd_role_change_evh(). I think this malloc should be changed to new > [HansN] I missed this function, now corrected. > > - csiattr.cc, function csiattr_create, memory is allocated with calloc. This > memory is returned using delete in avd_csi_remove_csiattr (a delete which is > placed wrong?). I think csiattr_create should use new and the delete in > avd_csi_remove_csiattr should be moved so it returns memory in both cases. > [HansN] I missed this function, the delete is where free was before but the > string_ptr is never freed in the old code as I can see, > so I, added a delete of string_ptr before the delete attr. > > - nodegroup.cc, function ng_ccb_completed_modify_hdlr, memory is reallocated > with realloc. Previously it was allocated in ng_create with new, should > probably be changed to delete, new, copy etc unless there is some smarted way > with C++ > [HansN] I changed saAmfNGNodeList back to use malloc as it is realloced > later. I left all realloc out to be fixed in later patch. There are no > realloc in c++ but instead we may use e.g. std::vector. > The same for strdup, we should use std::string instead. This will be changed > in coming patches. > > - pg.cc, function avd_pg_csi_node_add, some calloc failed logging should be > removed and the alloc comment which is stupid > [HansN] thanks, I missed that, now removed. > > amfnd seems OK > > Now trying to understand the libs/common/amf code > > Thanks, > Hans > > On 18 October 2013 13:51, Hans Nordeback <hans.nordeb...@ericsson.com> wrote: >> Summary: amf: replace malloc/calloc with new and delete for amfd and >> amfnd [#94] Review request for Trac Ticket(s): #94 Peer Reviewer(s): >> hansf, nagendra, praveen Pull request to: >> 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): >> --------------------------------------------- >> <<EXPLAIN/COMMENT THE PATCH SERIES HERE>> >> >> changeset 250880ee3f1562f208ae32d3c30ac34553f90a47 >> Author: Hans Nordeback <hans.nordeb...@ericsson.com> >> Date: Fri, 18 Oct 2013 13:46:55 +0200 >> >> amf: replace malloc/calloc with new and delete for amfd and >> amfnd [#94] >> >> Replace malloc/calloc with new and delete for amfd and amfnd. strdup, >> realloc and the libs/common are temporary fixed, more work has to be >> done. >> >> >> Added Files: >> ------------ >> osaf/services/saf/amf/amfd/include/tmp_common.h >> osaf/services/saf/amf/amfd/tmp_common.cc >> osaf/services/saf/amf/amfnd/include/tmp_common.h >> osaf/services/saf/amf/amfnd/tmp_common.cc >> >> >> Complete diffstat: >> ------------------ >> osaf/services/saf/amf/amfd/Makefile.am | 1 + >> osaf/services/saf/amf/amfd/app.cc | 7 +- >> osaf/services/saf/amf/amfd/apptype.cc | 12 +- >> osaf/services/saf/amf/amfd/chkop.cc | 16 +- >> osaf/services/saf/amf/amfd/comp.cc | 7 +- >> osaf/services/saf/amf/amfd/compcstype.cc | 27 ++- >> osaf/services/saf/amf/amfd/comptype.cc | 7 +- >> osaf/services/saf/amf/amfd/csi.cc | 22 +- >> osaf/services/saf/amf/amfd/csiattr.cc | 30 +-- >> osaf/services/saf/amf/amfd/cstype.cc | 25 +- >> osaf/services/saf/amf/amfd/ctcstype.cc | 9 +- >> osaf/services/saf/amf/amfd/dmsg.cc | 8 +- >> osaf/services/saf/amf/amfd/imm.cc | 634 >> +++++++++++++++++++++++++++++++++++------------------------------------------ >> osaf/services/saf/amf/amfd/include/amfd.h | 2 + >> osaf/services/saf/amf/amfd/include/imm.h | 63 +++++++- >> osaf/services/saf/amf/amfd/include/tmp_common.h | 36 ++++ >> osaf/services/saf/amf/amfd/main.cc | 32 +-- >> osaf/services/saf/amf/amfd/mds.cc | 20 +- >> osaf/services/saf/amf/amfd/ndfsm.cc | 25 +- >> osaf/services/saf/amf/amfd/ndmsg.cc | 29 +-- >> osaf/services/saf/amf/amfd/ndproc.cc | 18 +- >> osaf/services/saf/amf/amfd/node.cc | 21 +- >> osaf/services/saf/amf/amfd/nodegroup.cc | 13 +- >> osaf/services/saf/amf/amfd/pg.cc | 10 +- >> osaf/services/saf/amf/amfd/role.cc | 13 +- >> osaf/services/saf/amf/amfd/sg.cc | 7 +- >> osaf/services/saf/amf/amfd/sgproc.cc | 25 +- >> osaf/services/saf/amf/amfd/sgtype.cc | 13 +- >> osaf/services/saf/amf/amfd/si.cc | 34 ++- >> osaf/services/saf/amf/amfd/si_dep.cc | 48 +--- >> osaf/services/saf/amf/amfd/siass.cc | 8 +- >> osaf/services/saf/amf/amfd/sirankedsu.cc | 7 +- >> osaf/services/saf/amf/amfd/su.cc | 13 +- >> osaf/services/saf/amf/amfd/sutcomptype.cc | 7 +- >> osaf/services/saf/amf/amfd/sutype.cc | 13 +- >> osaf/services/saf/amf/amfd/svctype.cc | 25 +- >> osaf/services/saf/amf/amfd/svctypecstypes.cc | 7 +- >> osaf/services/saf/amf/amfd/timer.cc | 5 +- >> osaf/services/saf/amf/amfd/tmp_common.cc | 142 +++++++++++++++++ >> osaf/services/saf/amf/amfd/util.cc | 164 ++++--------------- >> osaf/services/saf/amf/amfnd/Makefile.am | 1 + >> osaf/services/saf/amf/amfnd/cbq.cc | 17 +- >> osaf/services/saf/amf/amfnd/chc.cc | 5 +- >> osaf/services/saf/amf/amfnd/ckpt_updt.cc | 14 +- >> osaf/services/saf/amf/amfnd/clc.cc | 22 +- >> osaf/services/saf/amf/amfnd/comp.cc | 38 +--- >> osaf/services/saf/amf/amfnd/compdb.cc | 134 ++++++++-------- >> osaf/services/saf/amf/amfnd/cpm.cc | 7 +- >> osaf/services/saf/amf/amfnd/di.cc | 279 >> +++++++++++++++------------------ >> osaf/services/saf/amf/amfnd/evt.cc | 17 +- >> osaf/services/saf/amf/amfnd/hcdb.cc | 20 +- >> osaf/services/saf/amf/amfnd/include/avnd.h | 2 + >> osaf/services/saf/amf/amfnd/include/tmp_common.h | 41 +++++ >> osaf/services/saf/amf/amfnd/mbcsv.cc | 8 +- >> osaf/services/saf/amf/amfnd/mds.cc | 18 +- >> osaf/services/saf/amf/amfnd/mon.cc | 22 +- >> osaf/services/saf/amf/amfnd/pg.cc | 33 +--- >> osaf/services/saf/amf/amfnd/pgdb.cc | 26 +-- >> osaf/services/saf/amf/amfnd/proxy.cc | 91 ++++------ >> osaf/services/saf/amf/amfnd/proxydb.cc | 37 +-- >> osaf/services/saf/amf/amfnd/sidb.cc | 36 +-- >> osaf/services/saf/amf/amfnd/su.cc | 3 +- >> osaf/services/saf/amf/amfnd/sudb.cc | 10 +- >> osaf/services/saf/amf/amfnd/susm.cc | 4 +- >> osaf/services/saf/amf/amfnd/tmp_common.cc | 315 >> ++++++++++++++++++++++++++++++++++++++ >> osaf/services/saf/amf/amfnd/util.cc | 19 +- >> 66 files changed, 1517 insertions(+), 1307 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. >> >> >> ---------------------------------------------------------------------- >> -------- October Webinars: Code for Performance Free Intel webinars >> can help you accelerate application performance. >> Explore tips for MPI, OpenMP, advanced profiling, and more. Get the >> most from the latest Intel processors and coprocessors. See abstracts >> and register > >> http://pubads.g.doubleclick.net/gampad/clk?id=60135031&iu=/4140/ostg.c >> lktrk _______________________________________________ >> Opensaf-devel mailing list >> Opensaf-devel@lists.sourceforge.net >> https://lists.sourceforge.net/lists/listinfo/opensaf-devel ------------------------------------------------------------------------------ October Webinars: Code for Performance Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register > http://pubads.g.doubleclick.net/gampad/clk?id=60135991&iu=/4140/ostg.clktrk _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
