Hi Spoorthy, Have looked into your latest webrev at http://cr.opensolaris.org/~spoorthy/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/ > > 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. >> 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) >> 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. - 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? >> 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. >> 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. >> >> 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. 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. Thanks, Srirama -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://mail.opensolaris.org/pipermail/sfwnv-discuss/attachments/20090325/77416460/attachment.html>
