Hi Sundar, Below are my comments.
Thamks, Sue usr/src/Targetdirs ------------------ 170 make "ai" used in dirnames or files be consistently either "ai" or "AI" usr/src/cmd/Makefile.cmd ------------------------ 52 - same comment as Targetdirs usr/src/cmd/installadm/ai-httpd.conf ----------------------------------- 113 and throughout - Would /var/autoinstall be better than /var/ai? usr/src/cmd/installadm/installadm-common.sh ------------------------------------------- 1. Add block comment at the top 2. Add block comment to functions without one usr/src/cmd/installadm/installadm.c ----------------------------------- General: i18n is inconsistent. It is partially inline and partially through use of installadm.h. Would be better to make it consistent. 139 - add a #define VERSION in installadm.h and use it instead of hardcoding here 210 - parse->parses, setup->sets up 213 - each of the "tasks". And also, setup->handle 245 - You set make_service_default and publish_as_unicast for the -d and -u switches, but then these variables are not used. If this is delayed functionality, please add comments saying so. 301 - the subcommand specific usage message is already passed in as "use", so you can just use it directly 313 - ditto 361 - Add code to handle an error if stat fails 376 - Use MSG_DIRECTORY_ACCESS, rather than MSG_MKDIR_FAILED 408 - Why are you printing out an error if the named service is running? 420-422 line up columns 440 - You are saving the return from the system call in ret, but then not using it. 553 - remove debug message 562 - Give usage here, rather than just returning? 573 - You set "ret", but then don't use it. 579 - get_service_data returns boot_file and txt_record, but I am not seeing how these are used anywhere 614 - can just print usage passed in "use" 635 - You set "ret", but then don't use it. 651-655 - delete_image, directory, boot_file, and txt_record all declared but not used 947 - Why are boot_file and txt_record passed in here? They are not used. 939,1030 - missing word between will and this 1060 - Shouldn't your malloc account for the semi-colons and \n in your fprintf? usr/src/cmd/installadm/installadm.h ----------------------------------- General: 1. References to dhcp should be either all in caps or all in lower case 2. Consider replacing the messages that start with "Failed to" with "Unable to" 3. Consider renaming failure messages with name that suggests a problem, i.e., instead of MSG_GET_HOSTNAME, use MSG_GET_HOSTNAME_FAIL or MSG_GET_HOSTNAME_ERR or similar 4. Add periods to end of messages. 5. Consider splitting the messages into a separate .h file, perhaps installadm_msgs.h 50 - add comment about why these port numbers are used 53 - Add more space after DATALEN 58 - TEXT_DOMAIN "SUNW_INSTALL_TOOLS" doesn't match the MSG_DOMAIN SUNW_INSTALL_INSTALLADM in usr/src/cmd/installadm/Makefile. 64 - lower case Directory 66 - lower case Delete 68 - lower case Directory 82 - Create->Please create 84 - Assign manually->Please assign the macro manually. 86 - tftp->TFTP usr/src/cmd/installadm/setup-dhcp.sh ------------------------------------ General: Need block comment at top 42 - Shouldn't there be "" around values? 59 - Remove TMP_DHCP since you're done with it 68 - macro+$1 -> macro=$1 95 - if add_macro fails, should there be instructions given to user similar to what is done on line 90? 110 - add blank line usr/src/cmd/installadm/setup-image.sh ------------------------------------- 38 - add blank line before functions 42 - remove "Call" 59 - says it prints usage, but does not do so 68 - usage function does not print usage 115 - remove "--" 147 - should there be a call to cleanup_and_exit instead of usage here? 154 - ditto 272-3 - Move these 2 lines to the usage function and call usage here instead. Can also remove 274 in that case. 277 - add comment for validate_ui 279 - this comment doesn't seem to belong here 285 - ensure that you actually have 3 arguments first? 299 - shouldn't cleanup_and_exit be passed non-zero return code here? usr/src/cmd/installadm/setup-service.sh ---------------------------------------- 25 - Please make a general block comment, rather than an item list. 47 - add return info to block comment 62 - do you really want to print this or is this a debug msg? 83 - add return info to block comment 106 - add blank line after sleep 108 - for when is this rewrite planned? 114 - if the service isn't registered, should you try again (in case 5 seconds wasn't enough)? 117 - Does the dns-sd process need to be killed as in the lookup case? 123 - Change to "Remove the specified running service" (or similar) 128 - add return info to block comment 139 - how do you know 2 seconds is enough? 149 - any possibility that pid2 doesn't exist and you need an else clause? 153 - add blank line above "Stop" comment 189 - add return info to block comment 219 - extra "webserver" usr/src/cmd/installadm/setup-tftp-links.sh ------------------------------------------ General: Add block comment at top 48 - remove line usr/src/pkgdefs/SUNWinstalladm-tools/pkginfo.tmpl ------------------------------------------------- 35 - Already Rev 2.0? usr/src/pkgdefs/SUNWinstalladm-tools/prototype_i386 --------------------------------------------------- 22 - Update copyright usr/src/pkgdefs/SUNWinstalladm-tools/prototype_sparc --------------------------------------------------- 22 - Update copyright Sundar Yamunachari wrote: > Hi all, > > Please review the code for installadm > > 3640 Installadm tools to manage services > http://defect.opensolaris.org/bz/show_bug.cgi?id=3640 > > > 3641 installadm tools to manage clients > http://defect.opensolaris.org/bz/show_bug.cgi?id=3641 > > Webrev: > http://cr.opensolaris.org/~ysundar/installadm > > The code covers the following functionality: > > 1. create-service: > - Setup the netimage directory (from iso or another netimage) > - Create a DNS service > - Start a web server associated with the DNS service > - Create DHCP server if needed > - Create DHCP network and add addresses to the DHCP table (if needed) > - Create the DHCP macro for this netimage (and service) > - Create bootfile in the /tftpboot directory > - All the services (DHCP, TFTP, and mDNS) are setup and enabled if > they are not running > > 2. delete-service > - Remove the service > - Remove the image if needed > > 3. start-service > - Create a service with given name > > 4. stop-service > - remove the given service > > 5. create-client > - Create bootfile in the /tftpboot directory with the name derived > from MACaddress > - Create a DHCP macro for this client > - Enable tftp service if not running > > 6. delete-client > - Remove the tftp bootfile > > Additional features: > > 1. Apache webserver will be configured at a port 5555 to host images > (zlibs). When the image is setup, a link will be created under Apache > docroot to point to the net image directory. The client will access the > zlib through the link. An apache configured file will be shipped with > this package to start a webserver at port 5555. > > 2. For each service, an AI webserver will be started. The code for the > AI webserver will be reviewed separately but the webserver > (webserver.py) is used in this code. > > Code organization: > > - The main code is in installadm.c. It parses the user input and checks > the options. Based on the options, call one more scripts. Each service > or task is in a separate script. The scripts are: > > create-client.sh; Create a client > delete-client.sh; Remove a client > setup-image.sh: Add/Remove netimages and creates link to webserver > setup-dhcp.sh: DHCP table creation, Add network, add ip addresses, > create DHCP macro etc. > setup-service.sh: Register/lookup/remove service, start/stop aI web > server. Uses DNS-SD > setup-tftp-links.sh: creates bootfile under /tftpboot > installadm-common: Common functions between create-client.sh and > setup-tftp-links.sh > > Thank you, > Sundar & Sue > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
