Hi! See replies inline.
/ Anders Widell On 05/23/2014 01:12 PM, Mathivanan Naickan Palanivelu wrote: > Hi Anders, > > Good work. > > Please find some comments(we have already discussed couple of them) > > 1) Iam thinking if we can make the **Lend and **Borrow to somehow (optimally) > be included > as a part of all service libraries. > This would mean, the extended length handling(tunneling) is initialized as a > part of > sa<svc>Initialize(). I think what I meant when we discussed this was that instead of putting the lend and borrow functions in just libSaImmOm, we could put them in all SAF libraries as weak symbols. This way, the application will get access to them no matter what SAF librar(ies) it is linked with. Is that what you are referring to? This is possible to implement, and I can take a look at how to add it to the patches. > > This way, the sa**Lend and sa**Borrow that is being proposed here would not > be necessary > as everything is implicitly handled when that service user calls > sa<svc>Initialize(). The borrow and lend functions would still be needed, no matter if they are in all libraries or just in libSaImmOm. > > 2) I am also thinking whether we should check and see if we can avoid setting > the environment variable > SA_ENABLE_EXTENDED_NAMES before starting the application. > > I mean, on top of turning on the build option, the additional environment > variable sounds > a crude approach! You might have thought/considered this, but still. Yes I have considered adding something that gets inserted / executed automatically when the preprocessor macro is defined. I think it may be techincally possible to do, but would depend on some sophisticated trick. It is not trivial, but I think it would be possible. I can take a look at it again, but I am a bit sceptical to doing too much magic. The environment variable solution is very simple. > > 3) We had discussed about fixing a upper limit for the extended length. > Otherwise, it would > be a security vulnerability. These patches contain a constant defining the upper limit to 2048, but the utility functions do not check the limit themselves. It is intended to be done by the agent code, but yes I can consider if the utility functions should also contain such a validation. > > > I used the prototype and things did come up. Things also looked to work > properly. > I will try to write from scratch an application and get back. > > Cheers, > Mathi. > > >> -----Original Message----- >> From: Anders Widell [mailto:[email protected]] >> Sent: Friday, May 02, 2014 4:21 PM >> To: Mathivanan Naickan Palanivelu; [email protected] >> Cc: [email protected] >> Subject: [PATCH 0 of 3] Review Request for Extended Name Type [#191] >> >> Summary: Extended Name Type [#191] >> Review request for Trac Ticket(s): 191 >> Peer Reviewer(s): Mathi, AndersBj >> Pull request to: >> Affected branch(es): default(4.5) >> Development branch: default >> >> -------------------------------- >> Impacted area Impact y/n >> -------------------------------- >> Docs y >> Build system n >> RPM/packaging n >> Configuration files n >> Startup scripts n >> SAF services y >> OpenSAF services n >> Core libraries y >> Samples n >> Tests n >> Other n >> >> >> Comments (indicate scope for each "y" above): >> --------------------------------------------- >> These patches add new AIS API functions and definitions for the extended >> SaNameT format, used to tunnel NUL-terminated strings through SaNameT. >> They also add support library functions to be used in agent libraries. >> >> changeset f3cf7ffb53b9a4dd6026cf58c262a46eac8bca3a >> Author: Anders Widell <[email protected]> >> Date: Fri, 02 May 2014 12:44:08 +0200 >> >> osaf: Add saAisNameLend() and saAisNameBorrow() [#191] >> >> Add declarations of saAisNameLend() and saAisNameBorrow() to >> saAis.h. Update >> 00-README.conf with description of how to enable the extended >> SaNameT type. >> >> changeset 84def835e0192244ba98eb6c80f192d2db26cd7d >> Author: Anders Widell <[email protected]> >> Date: Fri, 02 May 2014 12:44:12 +0200 >> >> osaf: Add library functions for handling the extended SaNameT format >> [#191] >> >> These library functions are primarily intended to be used in agent >> libraries, to handle the old SAF APIs that still are using the SaNameT >> type. >> >> changeset 99143446a43030c140d0ae95192546108b7639e2 >> Author: Anders Widell <[email protected]> >> Date: Fri, 02 May 2014 12:44:16 +0200 >> >> imm: Add implementations of saAisNameLend and saAisNameBorrow >> [#191] >> >> The functions saAisNameLend() and saAisNameBorrow() are defined >> in >> saAis_B_5_14.h, but their implementation is placed in libSaImmOm >> since there >> is no libSaAis library. >> >> >> Complete diffstat: >> ------------------ >> 00-README.conf | 18 +++++++++ >> osaf/libs/agents/saf/imma/Makefile.am | 1 + >> osaf/libs/agents/saf/imma/aisa_api.c | 131 >> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> ++++++++ >> osaf/libs/core/common/Makefile.am | 1 + >> osaf/libs/core/common/include/osaf_extended_name.h | 232 >> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> + >> osaf/libs/core/common/osaf_extended_name.c | 184 >> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> +++++++++++++++++++++++++++++++++++ >> osaf/libs/core/leap/sysf_def.c | 3 + >> osaf/libs/saf/include/saAis.h | 4 ++ >> osaf/libs/saf/include/saAis_B_5_14.h | 17 ++++++++ >> osaf/libs/saf/libSaImm/libSaImmOm.map | 2 + >> 10 files changed, 593 insertions(+), 0 deletions(-) >> >> >> Testing Commands: >> ----------------- >> Build and start OpenSAF. >> >> >> Testing, Expected Results: >> -------------------------- >> OpenSAF should build and start successfully. >> >> >> 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 ~/.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. >> ------------------------------------------------------------------------------ "Accelerate Dev Cycles with Automated Cross-Browser Testing - For FREE Instantly run your Selenium tests across 300+ browser/OS combos. Get unparalleled scalability from the best Selenium testing platform available Simple to use. Nothing to install. Get started now for free." http://p.sf.net/sfu/SauceLabs _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
