Hi Jean,
Thank you for the review! My comments are in-line.
Thank you,
Clay
On Mon, 28 Sep 2009, jeanm wrote:
>
> 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?
I only see delete_service mentioned when the delete_service module is
called for various functions. This is a shared code approach as
delete_client is a subset of the functionality exported by delete_service.
> 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.
This is just to make it more modular as these commands will hopefully all
collapse down to installadm as soon as create-service becomes Python.
Until then this just prevents hard coding bits. Yes, prog should be
delete-client actually to mimick how delete-client reports errors today.
This is achieved using the /usr/lib/installadm/delete-client symlink which
points to /usr/lib/installadm/delete_client.py. The reason for printing
the program name is just since it has these two execution possibilities.
The reason for having the delete_client is so that the module is
importable by Python (modules needing to be [a-zA-Z0-9_].py.
> line 43: Wouldn't it be more helpful to say a tuple representing....
You want me to be helpful now, that's just too much! Thanks for the
catch, I've changed it to:
Returns: A tuple of a class object representing client to delete
and an options object
> line 62: Why did you hang this off of options?
This is an option the functions expect to be set. This comes from
delete_service being able to delete an image, so as the comment says:
"we do not deleteImage for a delete-client so set it False"
Further, looking at line 89 (the only place options is really used) we see
that indeed deleteImage is used there, and we can correlate from line 60
(the comment) image deletion is hard coded off.
Note: I swapped line 60 (the comment for line 62) and line 61. I
accidentally got them out of order.
> usr/src/cmd/installadm/delete_service.py.html
>
> lines 837-840 : same comment as lines 78-81 in delete-client.py
Same reasons as with delete_client
> line 64: this really returns a tuple. Same comment as line 62 of
> delete_client.py
Same fix as delete_client
> 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))]
Hrm, I know we talked about this. I find this breaks up the logic so I can
see the intention behind it. I see the list files consists of only two
elements:
image_path
os.path.join('var/ai/image-server/images/',image_path.strip(os.sep))
Further, following better formatting allowed by PEP8 I think this should
be formatted as follows which more clearly shows that it's two separate
elements in the list:
files = [image_path,
# must strip the leading path separator from image_path as
# os.join won't concatenate two absolute paths
os.path.join('/var/ai/image-server/images/',
image_path.lstrip(os.sep))]
Also, do you use an editor or viewer which does syntax highlighting? I
just realized since my vi is configured for syntax highlighting, comments
are lighter than code, so I don't even visually notice them -- unless I
question what the code's doing and this lessens the distraction immensely.
> 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.
Yes, I'm not fond of them. This is tracked in 4402 (Pull fixed strings
in A/I server python code out to a separate module) which is a holder for
finding an elegant way to pull out these various paths. It seems we should
have a module we import for various install components and their paths but
perhaps there's better or different -- it's too big to design that here
though.
> 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.
This was accidentally left in from some tinkering I could've sworn I'd
gotten them.
This is further incredibly inexcusable, as I'm returning -1, 0, 1 which is
asinine in the days of Python. It should have been something
transparently obvious to a developer and user (e.g. "failure", "okay",
"human assistance needed"). I've now ensured these returns are stripped
from stop_service() and remove_DHCP_macro().
> line 403-425: Seems like there should be a lot simpler way of just removing a
> line from vfstab.
> Is there?
Um, this seems pretty short?
We've gotta open the file and handle the file not existing, opening, etc.
(first except on IOError).
Next, we've gotta look for the entry (which will throw a
ValueError/IndexError depending on where we fail - if we fail).
Lastly, we need to remove the line and need to check that the write took
(second IOError exception).
Am I missing how this could be streamlined though?
> 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.
Thanks to Ethan's code review comments this was changed. Please see
457-458 from
http://cr.opensolaris.org/~clayb/delete_service/webrev3.ethan/usr/src/cmd/installadm/delete_service.py.html
However, this was just a simple regex looking for:
"<anything>-s <anything up to a space, stored as path><anything to EOL>"
and replaced that whole string with what was found and stored as path.
> 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.
Sure, this was two lines but I was asked to expand fn to filename by
another reviewer. How does it work if just pulled out of the for loop. It
is three lines still but we get a much cleaner presentation (as it's only
three statements in three lines):
# build a list of grub menus from baseDir
menus = [filename for filename in os.listdir(baseDir) if
# only select files which start with grub menu prefix
filename.startswith(grub_menu_prefix) and
# do not select the menu file the service uses
filename != os.path.basename(menuLst.file_name)]
> line 584: Realistically, could this be in any other field than mountpoint? If
> not, could this code be simplified?
I'm not sure what code there is to simplify. Line 584 is a comment and the
next line of code seems trivial to the extreme (just seeing if a string is
in a list):
(line 587)
if boot_archive in com.MNTTab().fields.MOUNT_POINT:
> line 612: why? It's not already able to be executed? Are we fixing something
> here that should be fixed elsewhere?
That's the problem is that it's not executable, we're setting it so that
we can run it. To set the <tftpdir>/rm.<service name> script so we can run
it, we have to chmod it.
Yes, create-service should get rid of the rm.<service name> scripts. See
bug: 10853 (create-service:Should consider removing
/tftpboot/rm.<service|client> scripts). Until then it's an interface were
someone might stash something so let's honor it until we get rid of them.
As an aside, in the meantime - partially thanks to the 30 create-service
bugs, the majority of this file is to give us a working foothold from
which to clean things up. See Drew's comments about how bizarre it is to
have a script which reverse engineers the entirety of an AI service.
> 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.
Ah thanks I've fixed the comment. The intention is to not remove anything
except in the main function body, as this aids in debugging since it
provides a central list and order, as well as allowing breadcrumbs until
we've gathered all we can since we're piecing together what's left of a
service.
The comment now reads:
Handle file removal in /tftpboot by building a list of files to remove:
First, adds pxegrub.<directory pointed to by /tftpboot/<service
name> i.e. pxegrub.I86PC.OpenSolaris-1
Unmounts directory which is boot archive (will be something like
I86PC.OpenSolaris-4) and removes /etc/vfstab entry for mount point
Calls /tftpboot/rm.<service name> which should remove:
/tftpboot/<service name>
/tftpboot/menu.lst.<service name>
(if the above aren't removed by the rm.<service name> script they
are added to the remove list)
Adds /tftpboot/rm.<service name>
Returns: If unable to find tftp root - None
Success - A list of file paths to remove
> 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.
We need to remove the files provided by the list filesToRemove which
has the paths returned by:
find_service_directory
find_image_path
As well as (for services):
check_wanboot_conf (for SPARC)
- or -
removeTFTPBootFiles (X86)
So 714+ gets rid of these collections, not just check_wanboot_conf or
removeTFTPBootFiles. Further, 696 and 699 are not the same; one is for
SPARC and thus check_wanboot_conf, while the other is for X86 and thus
removeTFTPBootFiles.
I'm perhaps missing something in your suggestion here?
> usr/src/cmd/installadm/installadm_common.py
>
> I lied. I say Ethan's comment about Unix. I agree.
I've changed UnixFile() to File_()
> 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....
Yup, we only use a few fields from each file but this is pretty simple
now. Many people felt this functionality would be good to have available
for general extensibility (and as the inetd.conf was a nit request which
took ten minutes to add (the time for me to man -s 4 inetd.conf and type)
it seems pretty trivial now (one just needs to give field names and a
file name).
I'm not sure what you're asking here, other than is this easy? Yes
> Jean
>
>
>
>
>
