Hi Jean,
Thank you for the clarification on the phone yesterday. I think we
came to a consensus on all the various points of the code review. All
changes we discussed were made.
I think the biggest issues comes to removing files in the
individual functions which find the files versus removing them all at one
in a list (as is done).
Pros to removing them in each function:
*Files are removed immediately
*No return from the function has to be dealt with
(eases debugging to some)
Pros to removing them in a list:
*Feedback to user is separated into two stages:
Finding files and Removing files
*List of files to delete provides a clean way to debug ordering of file
removal and seeing all files which are to be removed (akin to a dry-run)
I think we came to an agreement that both approaches are closely equal in
usefulness and largely a stylistic difference.
Thank you,
Clay
On Wed, 30 Sep 2009, jeanm wrote:
> Clay Baenziger wrote:
>> 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.
> And line 26 which is a general overview of the file. The comment you just
> made might be nice somewhere since it does explain why
> delete-service is in the delete-client code.
>
>>
>>> 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 makes sense then. If eventually it will be just one function call with
> parameters then I'm fine with leaving it in for now.
>>
>> 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
> Thanks.
>>
>>> 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"
> So you're saying that the delete-service functionality might need this,
> correct? In which case I'm fine with it.
>>
>> 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.
> But if this was really the only reason we would just return a boolean. If
> however as stated above, delete-service would use it then you have it right.
>>
>> 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.
> No and the code review utility didn't either. In this particular case I'm
> fine with leaving this. Other cases it becomes much more distracting. Usually
> where you have multi line list comprehensions.
>>
>>> 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.
> Great. Thanks.
>>
>>> 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().
> Thanks.
>>
>>> 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?
> Well this went with the comment much further down about parsing the files in
> general. The idea is that if you got rid of all of the code it would be
> simpler. However from your response below it appears that having this is good
> so I'll shut up. ;-)
>
>>
>>> 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.
> The point was that a lot of people aren't that familiar with reg ex's. Very
> prone to maintenance mistakes so explaining what it's doing
> and maybe even how would be nice.
>>
>>> 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)]
> OK.
>>
>>> 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:
> Same response as above wrt to mountpoint in /etc/vfstab
>>
>>> 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.
> OK. Seems like something would be wrong it it wasn't executable but it's nice
> you're fixing the situation since you can. Why abort if you can fix as you're
> doing.
>>
>> 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.
> I'd have to disagree. I'd love to see things removed in the appropriate
> functions. So then you would know if you return from function
> removeTFTPBootFiles that all the tftpboot files are gone. I'd be confused it
> they weren't.
>>
>> 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
> Can they be removed in the functions?
>>
>> As well as (for services):
>> check_wanboot_conf (for SPARC)
>> - or -
>> removeTFTPBootFiles (X86)
>>
> But if these actually removed the files then you wouldn't need them to be
> gone here.
>> 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?
> I think so. Right now you're checking to see if you need to remove a file and
> placing it in a list. The list is later parsed and the file removed. That's
> extra computation and complexity. Why not check to see if you need to remove
> the file and remove it? Unless the file is needed later it's much simpler.
>>
>>> 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
> The greater extensibility answer was appropriate for my question. Guess I
> wasn't clear. Sorry. It was a long code review.
>
> Jean
>
>>
>>> Jean
>>>
>>>
>>>
>>>
>>>
>
>
