Sue,
I have taken care of all the changes. The updated webrev is in the
same place http://cr.opensolaris.org/~ysundar/installadm/.
Thanks,
Sundar
Sue Sohn wrote:
> 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?
I already removed it. Check the installadm.h
>
>>> 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
>>>>>>
>>>>>
>>>>
>>>
>>
>
