Hi , The updated webrev is at http://cr.opensolaris.org/~spoorthy/conmanupd2/ Please review it. Note: I have not added the comments for files in webrev as <bugid> < synopsis> since I have not got approval from Valerie Bubb and Scott Rotando to add bug category in /solaris/utitlity/<pkgname. I will add comments in that form once I get approval from them. Comments inline ...
Srirama Sharma wrote: > Hi Spoorthy, > > Have looked into your latest webrev at > http://cr.opensolaris.org/~spoorthy/conmanupd1/ > <http://cr.opensolaris.org/%7Espoorthy/conmanupd1/> > Please see my response inline and also few more comments at the end. > > Spoorthy H.S said the following on Tuesday 24 March 2009 02:54 PM: >> Hi, >> New webrev is at >> http://cr.opensolaris.org/~spoorthy/conmanupd/ >> <http://cr.opensolaris.org/%7Espoorthy/conmanupd/> >> >> Also, please see inline for updates. >> >> >> Srirama Sharma wrote: >>> Hi Spoorthy, >>> >>> Below are my comments: >>> >>> 1. In METADATA file, >>> - Please remove below line. >>> 11 PSARC: >>> - Add BUGTRAQ field. >>> >> Done . > > The BUGTRAQ field in METADATA should have the bugster > "product/category/subcategory" entry corresponding to conman. Had sent a request for scott.rotondo and valerie.bubb to approve me in creating bug category. Didnt get the reply still... I will include the value in the field in METADATA once I get the approval from them. > >>> 2. Please add the new product's entry in usr/src/cmd/Makefile under >>> 'COMMON_SUBDIRS' in alphabetical order. This is to ensure that >>> nightly script builds the new component from the toplevel dir. >>> >> Done. It was in alphabetical order before also. > > Previously it wasn't present in the wbrev. Now I see it there. > But you will have to resync your workspace and resolve conflicts in > this file. Currently you seem to be removing changes done in the > recent integrations (like convmv, lua, nano, openwsman. powerman, > sblim-wbemclie etc) > > Resync is done and conflicts are resolved... > >>> 3. In Makefile.sfw, >>> - Instead of using two command 'gzip' and 'tar' you could use >>> gtar with '-z' option. Something like >>> $(GTAR) xzpf $(TARBALL) --no-same-owner; >>> - What CFLAGS and CXXFLAGS are you using while building? >>> - In install target, you could also pass on the VER info as below: >>> PKGVERS=$(VER) $(SHELL) ./install-sfw >>> With this you could remove the hard coding of version >>> information done in install-sfw script. >>> >> Done . > > - You could use the already defined variable GTAR instead of > hardcoding its absolute path as '/usr/sfw/bin/gtar' > > - Again, what CFLAGS and CXXFLAGS are you using while building? > > - Change > 51 CXX="$(CCC)" \ > > to > 51 CXX="$(CCC) -norunpath" \ > > > >>> 4. usr/src/cmd/conman/conman.1 and usr/src/cmd/conman/conman.conf >>> - Are these Man pages already available in the source tarball >>> or are these newly written man pages ? >>> If they are newly written man pages, then they need to have >>> CDDL headers, Sun Copyright and the sunman-stability. >>> If they are available along with source tarball then there is >>> no requirement to check them into the workspace. Instead you could >>> just install them from the build dir after adding the >>> sunman-stability. >>> >> These files can come from source tarball. Forgot to remove them from >> here before sending webrev. Now, removed them. > > - In sunman-stability, you need to change the package name to SUNWconmanu. > > >>> 5. usr/src/cmd/conman/conmand.xml >>> - The CDDL headers are not in proper format. Please check other >>> manifest files under '/var/svc/manifest' dir. You could also refer >>> to recent integration/reviews of pen or openwsmand. >>> - In line 43, the package name should be SUNWconmanr as below: >>> 43 <service_bundle type='manifest' name='*SUNWconmanr*:conmand'> >>> - Manifest file doesn't seem to have the exec_method to >>> stop/disable the service. Is this intended ? If not, a new exec >>> method to stop the service needs to be added. >>> - Also curious to know if this service can be managed by any >>> normal user or does it require any special privileges ? >>> >> Done with the changes. But, Is there any source script to know how to >> write the exec_method to stop/disable the service ? If yes, please >> give the pointer. > > You could refer to already existing manifest files under > /var/svc/manifest dir. > or > You could also refer to below links to understand more about SMF > manifest/method files > http://opensolaris.org/os/community/smf/faq/ > http://www.sun.com/bigadmin/content/selfheal/sdev_intro.jsp > > - As specified above, the CDDL headers are not in proper format. It > should follow xml style for manifest file. > Done > - Also would like to know if this service can be managed by any normal > user or it can only be managed by root/super user? > I want it to be managed by any normal user also.. > >>> 9. usr/src/pkgdefs/SUNWconmanr/Makefile >>> - There is no 'ident' string. Please add it. >>> - The depend file seems to be the default one. So you could >>> remove the depend file for SUNWconmanr and add "DATAFILES = depend" >>> entry in the Makefile. >>> - Remove the extra '#' from line 33. >> Done > > The version on this file is 1.3. So you may have to do a 'wx redelget > -m' to drop the new files to their initial 1.1 delta. > > >>> 12. In usr/src/pkgdefs/SUNWconmanr/pkginfo.tmpl, please replace >>> 46 CLASSES="manifest" >>> >>> with >>> 46 CLASSES="none manifest" >>> Also please remove the extra line from the end of the file. > This comment still holds good. The CLASSES field needs the above change. > > Also, please add the version info at the end of DESC line as below: > 42 DESC="conman - Console Manager (0.2.3) (root)" > > >>> >>> 13. usr/src/pkgdefs/SUNWconmanu/Makefile >>> - Please add the 'ident' string to the Makefile. >>> - Are there no other dependencies other than SUNWconmanr? >>> Please check by running the dependency checker script. > > This comment still holds good. The ident string needs to be added. > > Done ... >>> 16. In almost all the files, CDDL header requires cosmetic changes. >>> Please follow the format for the CDDL headers and Sun Copyright >>> lines as specified in the link >>> http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/prototypes/ >>> >> I had followed the same link to add the CDDL header in makefiles. Let >> me know where I should change ? > > The CDDL and Copyright lines in your Makefile.sfw looks fine except > for below change (a space between # and ident keyword in line 25 along > with the line space change suggested by Paul ). You could take it as > reference and change the other Makefiles/install-sfw/scripts. > Change > 25 #ident "@(#)Makefile.sfw 1.3 08/03/25 SMI" > to > 25 # ident "@(#)Makefile.sfw 1.3 08/03/25 SMI" > > >>> 17. Please make sure all the files in your webrev has comments of >>> the format "<bugid> <synopsis>" > > Above comment still holds good. > > Will add the comment with <bugid><synopsis> once I create it. >>> >>> 18. Please ensure that you run a clean nightly build of your >>> workspace before regenerating the webrev and make sure there are no >>> errors in the nightly build log. >> Nightly build has completed succesfully ... > > Did you see any errors/warnings in the mail you would have got after > the nightly completed? Please check if you have permission warnings in > the log since > > - install-sfw has 444 permission for /etc/conman.conf where as > SUNWconmanr/prototype_com has 0555 for it. > > - For /etc/init.d/conman & /etc/logrotate.d/conman, install-sfw has > 555 permission where as SUNWconmanr/prototype_com has 0744. > After all modifications and changing the permissions, I had given for nightly from morning. Its completed successfully. > > Below are few more comments: > > - Please recreate the webrev to include changes done to > usr/src/Targetdirs also. Currently your webrev doesn't have > usr/src/Targetdirs file. > > - In install-sfw and SUNWconmanu/prototype_com file change the > permissions for README to 0444 > > - In the manifest file conmand.xml, You have specified the service > FMRI to be "system/conmand" as below > 45 <service > 46 name='system/conmand' > 47 type='service' > 48 version='1'> > But in SUNWconmanr/prototype_com file, you are installing the > manifest file under /var/svc/manifest/application dir as below: > 65 f manifest var/svc/manifest/application/conmand.xml 444 root sys > If the FMRI specified in conmand.xml is correct (i.e > "system/conmand") then you should be installing the manifest file > under "/var/svc/manifest/system" dir. Please check. > > Done. Thanks Spoorthy > Thanks, > Srirama
