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) > > >> > 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. > > 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. > > 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. > > 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. > >>> 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 ;-) . > > > 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. > > prototype_i386, prototype_sparc > ------------------------------- > Both copyrights still say 2007. I will change it.
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 >>>> >>> >> >
