I found time to do this when I thought I wouldn't. Using the following
webrev:
http://cr.opensolaris.org/~clayb/delete_service/webrev2. I haven't
checked to see
if others have commented on these items. Pardon me if they have.
Also, if you just want to discuss this in the office instead of an email
thread I'm fine with that.
Sometimes it's easier.
Jean
usr/src/cmd/installadm/delete_client.py.html
General:
This is the delete-client file but there are references to
delete-service throughout. That causes me to be confused
since I know they are different and have different functionality. Is
this a cut and paste issue? Or similar functionality
between the two?
line 78-81: Since you're hardcoding the first %s to installadm why
bother with the %s in the first place? I actually
don't think you even need the 2nd %s. This is run in delete-client. If
prog is something other than that, this message
would be extremely confusing.
line 43: Wouldn't it be more helpful to say a tuple representing....
line 62: Why did you hang this off of options?
usr/src/cmd/installadm/delete_service.py.html
lines 837-840 : same comment as lines 78-81 in delete-client.py
line 64: this really returns a tuple. Same comment as line 62 of
delete_client.py
General comment: I find that putting comments into the middle of a group of
code that is line continued makes the code difficult to read.
Ex.
377 files = [image_path,
378 # must strip the leading path separator from image_path as
379 # os.join won't concatenate two absolute paths
380 os.path.join('/var/ai/image-server/images/',
381 image_path.lstrip(os.sep))]
Another general comment. There are lots of hard coded paths. This makes
changes
to these paths in the future very difficult to maintain in the code.
line 196:
It would be cleaner to just do
if not return_val
return 1
return return_val
But you should be sure that return_val is None at the top. Certainly is
easier to read and cleaner.
line 403-425: Seems like there should be a lot simpler way of just
removing a line from vfstab.
Is there?
line 455-456: Boy that looks unmaintainable. If there is a more
maintainable way to do this I
would suggest it. If not, you need a big old comment there.
lines 556-559: Seems a little long for list comprehension. I believe we
had discussed anything past about
2 lines should be considered for breaking up. Could this be done? It
would make the code easier to read
and probably easier to maintain.
line 584: Realistically, could this be in any other field than
mountpoint? If not, could this code be simplified?
line 612: why? It's not already able to be executed? Are we fixing
something here that should be fixed elsewhere?
lines 625-634: Since the point of this function is to remove the
tftpboot files, why don't you just remove these files
if the rmCmd hasn't? It certainly would be more efficient that appending
them to files now and removing them later.
And clearer as to the state of the system after this function is called.
Plus, the header comments say the function removes them.
Why not have check_wanboot_conf also make sure everything is cleaned up.
Then you don't need the code at 696 ad 699 to
add onto the fileToRemove list. Could we also then get rid of 714+? I
think all that would be left is then explicit files which
could then just be rm'd. In general this seems more complicated than it
needs to be.
usr/src/cmd/installadm/installadm_common.py
I lied. I say Ethan's comment about Unix. I agree.
lines 393-494: do we really need to do this in this manner? For example
VFSTab creates a database of the vfstab file. So far I've seen the
mountpoint field looked at for the boot_archive. Seems like overkill for
that. If we foresee needed more detailed parsability in the future I'm
fine with it. Otherwise I wonder....
Jean
