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/20081003/7b5436ab/attachment.html>
