John,
John Fischer wrote:
> Sundar,
>
> Thanks for the review!! Comments below.
>
> John
>
>
> Sundar Yamunachari wrote:
>> John,
>>
>> usr/src/cmd/installadm/Makefile:
>>
>> 98, 105: The target 'all' defined twice. Can you combine them?
>
> corrected.
>
>> usr/src/cmd/installadm/installadm.c:
>>
>> 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.
>
>> usr/src/cmd/installadm/installadm.h:
>>
>> 55: Just a nit -- move the right side of the definition so that it is
>> aligned with existing definitions.
>
> corrected.
>
>> 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.
>
>> 64-67: Why we need a version number for list?
>
> removed.
>
>> 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.
>
>> 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?
>
>> 91-109: installadm.c uses the existence of platform/sun4v or
>> platform/i86pc to determine the architecture. Please use the same
>> method to find the architecture of the image. platform/{arch} is
>> more stable than boot/multiboot and also it will be consistent with
>> the rest of installadm code.
>
> Ah, ok. Corrected.
>
>> 154: get_manifest_name has two input args but the comment in 156
>> indicates there are 3 input.
>
> Will be corrected when the comments for the functions are beefed up.
>
>> 164: Assumes that the port is always 5 characters (digits). This may
>> be true now but may fail if it changes. Use the method at line 143.
>
> The code at this line was before I knew about the find method. ;-)
> Corrected.
>
>> 184: The comment at 156 indicates that the functions returns void
>> where as it returns service dictionary.
>
> Will be corrected when the comments for the functions are beefed up.
>
>> 215: same comment as 164 above
>
> Will be corrected when the comments for the functions are beefed up.
>
>> 211, 223: Width is initialized to 0 twice
>
> Well that is just silly. I think this is one of those things where
> it wasn't defined outside block and so I added it to a higher level
> and forgot to remove the second one.
>
>> 236: same comment as 164 above
>
> Same response. :-)
>
>> 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.
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
>>
