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?
> 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?
> 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.
> 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?
> 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.
> 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
>
