Sundar Yamunachari wrote: > Susan Sohn wrote: >> Sundar Yamunachari wrote: >>> 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 >> >> >> Hi Sundar, >> >> Good job making all of those changes. >> >> Here are a few comments: >> >> installadm.c >> ------------ >> >> 361 - Add code to handle an error if stat fails >> >> I don't see any changes for this one. > See lines 365-370 (It displays an error message and return)
Yep, fine. >> >> > 939,1030 - missing word between will and this >> >> > >> >> The answer is "use". I will fix the comment >> >> The second one is fixed, but the first one is still missing "use". > I will fix that one. thanks. >> installadm.h >> ------------ >> >>3. Consider renaming failure messages with name that suggests a >> problem, >> >> Just one more - MSG_DELETE_IMAGE. > This is not used. There is a MSG_DELETE_IMAGE_FAIL, which is the one > that is used. > Then perhaps remove MSG_DELETE_IMAGE? >> setup-dhcp >> ---------- >> >>> 95 - if add_macro fails, should there be instructions given to user >> >>> similar to what is done on line 90? >> >>> >> >>Yes. >> >> Were changes made to do this? > I have modified setup_x86_dhcp_macro() to display the instructions if > there is no DHCP server or no permissiom to access the DHCP server (line > 104). If adding/modifying macro fails, then there is a problem with the > command and it is a bug. We shouldn't give instructions to the user to > add manually. ok >> setup-image >> ----------- >> >>> 277 - add comment for validate_uid >> >>> >> >>okay > I thought this comment for the function validate_uid(), which clearly > says that it is checking for root. I can add the similar comment here. ok >> >>> 279 - this comment doesn't seem to belong here >> >>> >> >>Will remove > Sorry. I will remove it. >> >> These were skipped, I think. Also, remove one "enough" from 274. > one "enough" is enough ;-) . indeed. :) >> setup-service.sh >> ---------------- >> >>> 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. >> >> Were you planning to add code to do this in register_service? > The dns-sd process to register the service should not be terminated. > This will get terminated when delete-service is called. ok >> prototype_i386, prototype_sparc >> ------------------------------- >> Both copyrights still say 2007. > I will change it. ok. Assuming the above changes are made, I am satisfied with the code and don't need to see another webrev. Thanks, Sue > Thanks, > Sundar >> >> That's it. >> >> Sue >> >>> 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 >>>>> >>>> >>> >> >
