Sundar,
Thanks again!!
John
sundar Yamunachari wrote:
>>> 994-1006: The comment indicates that a new error message is not
>>> printed if the return code is 1 but the code at line 1000 checks for
>>> return value '256'. Which is correct?
>>
>> Right. You are referring to the following comment and code:
>>
>> /*
>> * Ensure we return an error if ret != 0.
>> * If ret == 1 then the Python handled the error, do not print a
>> * new error.
>> */
>> if (ret != 0) {
>> if (ret == 256) {
>> return (INSTALLADM_FAILURE);
>> }
>> (void) fprintf(stderr, MSG_SUBCOMMAND_FAILED, argv[0]);
>> return (INSTALLADM_FAILURE);
>> }
>>
>> This is the same code format and error as do_add, do_remove including
>> function call and comments about the return value. Other subcommands
>> simply check to see if the return value is non-0 and return
>> INSTALLADM_FAILURE. And in actuality this part of the code has not
>> changed. This is for python errors only which return 1 to the shell.
>> The code should be changed to indicate that WEXITSTATUS(ret) == 1
>> then the error was handled. Again this is the same for several
>> functions within the code. Should I correct those as well?
> If it is only this file, you can change it. If it is more than one file,
> you can file
> a bug for this issue.
Will correct in rest of code as well.
>>> usr/src/cmd/installadm/list.py:
>>>
>>> General: It is assumed that tftp directory is /tftpboot and it may
>>> not be true. This directory is defined with tftp service in
>>> /etc/inetd.conf. The default value is /tftpboot but it could be some
>>> other directory too. So please check /etc/inetd.conf for the tftp
>>> directory. Check usr/src/cmd/delete_service.py:findTFTProot() for
>>> more information.
>>
>> OK. findTFTProot() is not within a class or otherwise reusable. Should
>> I simply define it within list?
> I don't understand what you mean here. If you are going to define
> findTFTProot() with in a class and make it usable outside the class then
> my answer is yes. We don't need to duplicate the code.
So findTFTProot() is an embedded function of remove_files() within the
delete_services file in /usr/lib/installadm. remove_files() is not a
class nor part of a class so I can not use it that way. The simple
function/embedded function example attached shows what I am talking
about. I can not figure out if Python allows one to call the embedded
function. Use callsubfunc.py to see the problem that I am trying to
solve.
If Python does not allow this sort of thing then I will need to create
another python file to share some common functions that can be imported
by other installadm subcommands. This seems like the cleanest way to
share the code.
Alternatively, I could move the findTFTProot() embedded function out
of remove_files() which would allow me to import it into list to use.
This would share the code between the two. I would also add several
comments to it so that list does not break if someone modifies the
code. Not as clean as above because someone could break list when
changing delete_services.
>>> 68: Can you change name --> service (or service name) so that it will
>>> be clearer
>>
>> dest = 'name' changed to dest = 'service' and name changed several other
>> places to match service. -n and --name are defined as options so I am
>> assuming that you were not referring to those. If this is what you were
>> referring to then I will need to change the design document as well.
> Only with in the code and not the options.
Good, corrected.
>>> 50-89: In line 60, the usage indicates that there is an option '-h'
>>> but the parser doesn't process '-h'
>>
>> This is a side effect of the parse_args() method for the OptionsParser
>> class. Therefore, shouldn't the -h be listed in the usage output?
> Does the usage display -h? What happens if give -h as an option to list?
If one passes installadm list -h it will always give the help
information from list. This is because the OptionsParser class
already defines what to do with the -h/--help options. This is
also true of any other subcommand that uses the OptionParser class,
for example delete-service. So because of the OptionsParser class,
'installadm list -h' will display:
usage: installadm list [-n <servicename>] [-c] [-m]
Lists all enabled install services on a system. Or, with -n option,
lists a
specific install service. Or, with -c option, lists information about
clients
of install services. Or, with -m option, lists the manifest information.
options:
-h, --help show this help message and exit
-n SERVICE, --name=SERVICE
list information about named service
-c, --client list client information
-m, --manifest list manifest information
Thus I think that there should be a '| [-h]' at the end of the usage
line (i.e., usage: installadm list[-n <servicename>] [-c] [-m] | [-h]).
>>> 340: Only x86 information is stored in menu.lst. The sparc
>>> information under /etc/netboot directory. If you are looking for
>>> install_service and install_svc_address for sparc, they are under
>>> $image/install.conf. Also in sparc, these values are service specific
>>> rather than the client specific and a bug 7152 filed for this issue.
>>> I think it is better to store this information in SMF install service
>>> when a service/client is created and retrieve it from there for list.
>>
>> I did not realize how different the 2 platforms were. Hmmm... I'll
>> see what I can do.
> We discussed yesterday about the sparc location and how to get those
> information. If you need more help, let me know.
And thanks to you, it now works properly for both Sparc and x86 clients.
So corrected.
> Thanks,
> Sundar
>>
>>> 393: The comment above applies to get_clients(). The sparc clients
>>> are under /etc/netboot.
>>
>> OK.
>>
>>> 485: This may not be the right check since the service menu.lst is
>>> also in the same directory. If there is an X86 service, the directory
>>> should exist.
>>
>> OK.
>>
>>> usr/src/man/installadm.1m.txt:
>>>
>>> The diff doesn't look right. The old one is txt and new one is troff?
>>
>> Correct. I am converting it to nroff to get a little bit more clarity
>> into the output. It will now highlight command names, italicize option
>> parameters like <service name> for devices that can handle this type of
>> output (xman for example). It will underline option parameters for
>> devices that can not handle bold and italics (terminal window for
>> example).
>>
>> I will be doing this for beadm and distro_const as well shortly.
>>
>>> Thanks,
>>> Sundar
>>>
>>> John Fischer wrote:
>>>> All,
>>>>
>>>> Updated webrev for this set of changes at:
>>>>
>>>> http://cr.opensolaris.org/~johnfisc/list
>>>>
>>>> These changes include feedback during a phone conversation from Ethan
>>>> as well as the installadm man page change to document the list
>>>> subcommand.
>>>>
>>>> Thanks,
>>>>
>>>> John
>>>>
>>>> John Fischer wrote:
>>>>> Ethan, Clay and Sundar,
>>>>>
>>>>> When you get a chance can you review my webrev at:
>>>>>
>>>>> http://cr.opensolaris.org/~johnfisc/list
>>>>>
>>>>> These changes are for the installadm list subcommand fix and
>>>>> will address:
>>>>>
>>>>> 4330 - installadm list should show which server provide the
>>>>> install
>>>>> services.
>>>>> 4113 - nice to have an option to show what service the client is
>>>>> using
>>>>> 4298 - installadm list -n should give different output if there is
>>>>> no custom manifest
>>>>> 4597 - installadm list: expect usage statement when giving service
>>>>> name without "-n" flag.
>>>>> 4646 - list: showing added manifest for a non running service.
>>>>> 5300 - list: does not show informational message for non running
>>>>> service.
>>>>> 8496 - list: no verbiage indicating that a service does not exist
>>>>> when a non-existent service is given.
>>>>> 8529 - 'installadm list' command lists same service three times
>>>>> 6811 - list: should have similar output between list and list -n
>>>>> 8015 - list-manifests output is hard to read
>>>>> 9094 - installadm list: prints colons for empty MAC fields, and
>>>>> doesn't account for colons or periods in MAC field's width
>>>>> 4175 - install list error slips out
>>>>>
>>>>> The changed and added files are:
>>>>>
>>>>> usr/src/cmd/installadm/Makefile
>>>>> usr/src/pkgdefs/SUNWinstalladm-tools/prototype_com
>>>>> usr/src/cmd/installadm/installadm.c
>>>>> usr/src/cmd/installadm/installadm.h
>>>>> usr/src/cmd/installadm/list.py
>>>>>
>>>>> These changes also include a change to the Makefile and prototype_com
>>>>> file for the delete_service and delete_client to be consistent with
>>>>> the other python scripts within the /usr/lib/installadm directory.
>>>>>
>>>>> The remote service listing is being postponed due to a timing issue
>>>>> with multiple remote services that grows with the number of services
>>>>> within the domain.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> John
>>>>> _______________________________________________
>>>>> 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 --------------
A non-text attachment was scrubbed...
Name: function.py
Type: text/x-python
Size: 277 bytes
Desc: not available
URL:
<http://mail.opensolaris.org/pipermail/caiman-discuss/attachments/20091204/28653785/attachment.py>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: callsubfunc.py
Type: text/x-python
Size: 389 bytes
Desc: not available
URL:
<http://mail.opensolaris.org/pipermail/caiman-discuss/attachments/20091204/28653785/attachment-0001.py>
