Sue , I updated the webrev with the changes based on your review comments. Please take a look at the webrev and let me know. The webrev is in the same place http://cr.opensolaris.org/~ysundar/installadm
Thanks, Sundar Sundar Yamunachari wrote: > Sue, > Thanks for the comments. > > Susan Sohn wrote: >> 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" >> > This is how Clay named the directories in AI webserver. I will check > with him. >> usr/src/cmd/Makefile.cmd >> ------------------------ >> 52 - same comment as Targetdirs >> > okay. >> usr/src/cmd/installadm/ai-httpd.conf >> ----------------------------------- >> 113 and throughout - Would /var/autoinstall be better than /var/ai? >> > If you have to type a long path, /var/ai might look better. >> usr/src/cmd/installadm/installadm-common.sh >> ------------------------------------------- >> 1. Add block comment at the top >> 2. Add block comment to functions without one >> > okay :-( >> 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. >> > The code in the installadm is done at different times. I will move all > the message to the header file. >> 139 - add a #define VERSION in installadm.h and use it instead of >> hardcoding here >> > Will do >> >> 210 - parse->parses, setup->sets up >> 213 - each of the "tasks". And also, setup->handle >> > okay for the above comments >> 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. >> > Yes. It is not supported now. I will add comments. >> 301 - the subcommand specific usage message is already passed in as >> "use", so you can just use it directly >> > okay. >> >> 313 - ditto >> 361 - Add code to handle an error if stat fails >> 376 - Use MSG_DIRECTORY_ACCESS, rather than MSG_MKDIR_FAILED >> > okay >> 408 - Why are you printing out an error if the named service is running? >> > Debug message >> 420-422 line up columns >> > okay >> 440 - You are saving the return from the system call in ret, but then >> not using it. >> > Currently all functions are declared as void. We will change that to > return error in the case of failure. The return codes will be used there. >> 553 - remove debug message >> > okay. >> 562 - Give usage here, rather than just returning? >> > It will be be detected by the caller if the usage is not correct. >> 573 - You set "ret", but then don't use it. >> > See response to 440 >> 579 - get_service_data returns boot_file and txt_record, but I am not >> seeing how these are used anywhere >> > Yes. We didn't implement cleaning up boot_file and removing data > correspond to the webserver that was started with the service. So > those will be used in the future. >> 614 - can just print usage passed in "use" >> > okay. >> 635 - You set "ret", but then don't use it. >> > See response to 440 >> 651-655 - delete_image, directory, boot_file, and txt_record all >> declared but not used >> > I will remove them >> 947 - Why are boot_file and txt_record passed in here? They are not used. >> > They are not used now but they will be used in the future since we > asume boot_file and txt_record in our current code. >> 939,1030 - missing word between will and this >> > The answer is "use". I will fix the comment >> 1060 - Shouldn't your malloc account for the semi-colons and \n in your >> fprintf? >> > Yes. What happened to "+6" I had there? There should be +6 there. It > is not there in the webrev and will be there next time I generate one. >> 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" >> > Is there any particular reason not to use 'Failed" >> 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. >> > okay. >> 5. Consider splitting the messages into a separate .h file, perhaps >> installadm_msgs.h >> > The messages are limited in number. We can do it when there are more > messages. >> 50 - add comment about why these port numbers are used >> > okay >> 53 - Add more space after DATALEN >> > okay. >> 58 - TEXT_DOMAIN "SUNW_INSTALL_TOOLS" doesn't match the MSG_DOMAIN >> SUNW_INSTALL_INSTALLADM in usr/src/cmd/installadm/Makefile. >> > Good catch. I will fix it. >> 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 >> > okay to all. >> usr/src/cmd/installadm/setup-dhcp.sh >> ------------------------------------ >> General: Need block comment at top >> >> 42 - Shouldn't there be "" around values? >> > You don't need it. I will add it just to be clear >> 59 - Remove TMP_DHCP since you're done with it >> > I kept it for debugging. I will remove it. >> 68 - macro+$1 -> macro=$1 >> > okay. >> 95 - if add_macro fails, should there be instructions given to user >> similar to what is done on line 90? >> > Yes. >> 110 - add blank line >> > okay. >> usr/src/cmd/installadm/setup-image.sh >> ------------------------------------- >> 38 - add blank line before functions >> > okay >> 42 - remove "Call" >> > okay. >> 59 - says it prints usage, but does not do so >> >> 68 - usage function does not print usage >> > Will fix >> 115 - remove "--" >> > okay. >> 147 - should there be a call to cleanup_and_exit instead of usage here? >> 154 - ditto >> > Yes. >> 272-3 - Move these 2 lines to the usage function and call usage here >> instead. Can also remove 274 in that case. >> > okay. >> 277 - add comment for validate_ui >> > okay >> 279 - this comment doesn't seem to belong here >> > Will remove >> 285 - ensure that you actually have 3 arguments first? >> > okay. >> 299 - shouldn't cleanup_and_exit be passed non-zero return code here? >> > Yes. >> usr/src/cmd/installadm/setup-service.sh >> ---------------------------------------- >> 25 - Please make a general block comment, rather than an item list. >> > okay >> 47 - add return info to block comment >> > okay >> 62 - do you really want to print this or is this a debug msg? >> > Debug message. May be there till everything is working properly. >> 83 - add return info to block comment >> 106 - add blank line after sleep >> 108 - for when is this rewrite planned? >> > okay >> 114 - if the service isn't registered, should you try again (in case 5 >> seconds wasn't enough)? >> > No. We want to replace this code with own own library. So I don't want > to add too much complexity here. >> 117 - Does the dns-sd process need to be killed as in the lookup case? >> > Yes. dns-sd is program with callback mechanism and never returns. It > is meant for plug and play where you constantly looking for new services. >> 123 - Change to "Remove the specified running service" (or similar) >> > okay. >> 128 - add return info to block comment >> 139 - how do you know 2 seconds is enough? >> > It is a guess. It works for me in most cases. >> 149 - any possibility that pid2 doesn't exist and you need an else clause? >> > It is possible but we have to continue and stop the webserver that is > associated with this service. >> 153 - add blank line above "Stop" comment >> 189 - add return info to block comment >> 219 - extra "webserver" >> > okay >> usr/src/cmd/installadm/setup-tftp-links.sh >> ------------------------------------------ >> General: Add block comment at top >> 48 - remove line >> > okay >> usr/src/pkgdefs/SUNWinstalladm-tools/pkginfo.tmpl >> ------------------------------------------------- >> 35 - Already Rev 2.0? >> > I though it means the version of OpenSolaris. >> usr/src/pkgdefs/SUNWinstalladm-tools/prototype_i386 >> --------------------------------------------------- >> 22 - Update copyright >> > okay >> usr/src/pkgdefs/SUNWinstalladm-tools/prototype_sparc >> --------------------------------------------------- >> 22 - Update copyright >> > okay. > > Thanks, > Sundar >> 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 >>> >> >> _______________________________________________ >> caiman-discuss mailing list >> caiman-discuss at opensolaris.org >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >> > -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://mail.opensolaris.org/pipermail/caiman-discuss/attachments/20081006/c65ca778/attachment.html>
