Hi Clay.
Here are most of my re-review comments. I will send any comments on
installadm_common.py which I may have later, by lunchtime today (13:30 MT).
If it matters, line numbers are based on the diffs webrev.
Thanks,
Jack
usr/src/cmd/installadm/Makefile
38: nit: indentation
delete-service.py:
150: I would like to see a consistent type for return. Instead of
None/-1/1,
how about 0/-1/1 ? Then there is no need for the caller to test for return
value existence before testing for -1 or 1?
202 redundant returns
209: The comments in () are redundant with previous line.
check_wanboot_conf: please correct the comment to say that you are
building a list of files to delete, not that you are deleting files.
270/271: Nit: parentheses are more natural when expressing a condition
than using a \ at the end of 270.
380, 397: /var/ai/image-server/images is in these two places at least.
Please define a common string somewhere for these. Better yet, /var/ai
is used in even more places. If the string /var/ai/image-server/images
would have to change if the /var/ai string did, please define /var/ai
somewhere and use in all places the variable which carries that string.
425: Does the microroot-less vfstab file get written somehow?
512: executible -> executable
650/651: Please add periods to the ends of each sentence. Without them,
it looks like one sentence.
685: I don't understand the comments here. I think "again" may be
causing confusion. Also, what SMF tests were run? Can 686/687 be a
separate comment from 685?
700: Can't a complete list of both SPARC and X86 files to delete be
added to filesToRemove and removal attempted? Only the relevant files
for the approrpriate architecture will actually be there for removal and
therefore be removed, right?
installadm.c
------------
Many places where "if (xxx == B_TRUE)". Why not just say "if (xxx)" ?
(Note: PEP8 suggests this too...)
421, 459, 714, 801,934, 1009,1034, 1145, 1162, 1338, 1445
