Hi Jack,
Thank you for getting through this new bundle! My comments are
in-line.
Thank you,
Clay
On Wed, 30 Sep 2009, Jack Schwartz wrote:
> 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?
These were for something I was thinking about, but should have been
stripped. The functions just all return None as they did before.
> 202 redundant returns
Similarly, this was part of the oversight and is gone now.
> 209: The comments in () are redundant with previous line.
Ah yes, pulled out the extraneous part in the parens
> 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.
Checks to see if /etc/netboot/wanboot.conf is a dangling symlink and if
so returns its path. Further, returns /etc/netboot/<service name> too. If
removing the last entry under /etc/netboot, also return /etc/netboot.
All of these paths are compiled into a list.
Otherwise, returns None
> 270/271: Nit: parentheses are more natural when expressing a condition than
> using a \ at the end of 270.
For me the parens muddle the logic potentially since there's a not and
some other stuff. Thank you for the info though as to other eyes and I'll
try to write to that as I can.
> 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.
I agree, this is bug 4402 (Pull fixed strings in A/I server python code
out to a separate module) as I'd like to look into some various solutions
for this. I don't think I've yet found a clean way to do this as I'd like.
> 425: Does the microroot-less vfstab file get written somehow?
The function on 418 updates the backing-store (the /etc/vfstab file):
del(vfstabObj.fields.MOUNT_POINT[idx])
> 512: executible -> executable
Thank you
> 650/651: Please add periods to the ends of each sentence. Without them, it
> looks like one sentence.
Darn English grammar; good point
> 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?
Good point this was rather vague. How's this for clarification?
# no identifying SMF properties were found, nor were SPARC files found,
# so try looking for X86 files.
# both X86 clients and services need equivalent files removed
# from tftpboot (so, look for /tftpboot/<service or client name>)
> 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?
No, unfortunately. The error reporting would go off warning of
non-existent files for any files which are not found; this was something
I had not anticipated in the code-walk through. I think having
create-client do the check for a pre-existing client is best. Otherwise,
the admin will have to run delete-client twice (once for each
architecture). And as this is a corner case (not a functional config, not
logical, etc.) it seems like it's too risky to address further here.
> 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
Yes, certainly in Python I would not like to see if(xxx == True), however,
this is how installadm.c was written and I figure I should fit into its
style. I hope this file's days are numbered quite low.
>
