Hi Ethan,
Comments in-line. New differential webrev at
http://cr.opensolaris.org/~clayb/delete_service/webrev3.ethan/
Thank you,
Clay
On Fri, 25 Sep 2009, Ethan Quach wrote:
> Clay,
>
>
> I have some additional comments on the updated webrev ...
>
>
> usr/src/cmd/installadm/installadm.c
> -----------------------------------
> 889 - Upon successful creation of an install service, it's
> enabled by default isn't it? So we shouldn't need the call
> to check_for_enabled_install_services() here it seems...
Good point, I've removed check_for_enabled_install_services()
> 946 - Don't we need to check to see if we just deleted the
> last service somewhere around here, and if so, disable our
> smf service? Or is this logic somewhere else that I'm missing?
No, apparently sending a SIGTERM to Apache was causing the service to
restart and then stay in maintenance. I now, instead check if we're the
last service and set the service to maintenance and no-longer kill the
apache process (as the SMF stop method does that).
> usr/src/cmd/installadm/installadm_common.py
> -------------------------------------------
> 36 - What's the significance of using the name "Unix" here?
None much. It was suggested in the code walk through by someone. However,
it has no meaning and perhaps should just be File_ as it just trivially
extends the file class.
> 65 - In this method, if newlines = False, and blanklines = True,
> what will get returned at 107 when processing a blankline in
> from a file? An empty string '' ??
An empty string would be returned but from line 104 not 107. Why, are you
thinking a particular use should be warned against in the docstring?
> 198-200 - So it seems this caching doesn't save any effort on rereading
> the entire file from disk, but just contents processing?
> Could we cache based on modtime of the file or something
> like that?
I had thought of the common practice of disabling atime and confused that
with mtime. Yes I think checking mtime is far more appropriate and
certainly quicker. I have made this change.
>
> usr/src/cmd/installadm/delete-service.py
> ----------------------------------------
> 413 - I dislike how you have to visit an additional time just
> to check existence. I thought there existed some NO_IDX
> exception when searching a list. If so, you can then just
> wrap 415 in a try
Hi Ethan, there is a ValueError thrown. I think we're getting into pretty
micro-style/optimization here. However, I can change the code from:
To find out this is a value error, try:
>>> a=[1,2]
>>> a[3]
Traceback (most recent call last):
File "<stdin>", line 1, in ?
IndexError: list index out of range
(This is what vfstabObj.fields.MOUNT_POINT[idx] could in all fairness
raise)
>>> a.index(3)
Traceback (most recent call last):
File "<stdin>", line 1, in ?
ValueError: list.index(x): x not in list
(This is what vfstabObj.fields.MOUNT_POINT.index(boot_archive) would
raise)
Thus the code now reads:
# look for filesystem in /etc/vfstab
try:
# calculate index for mount point
idx = vfstabObj.fields.MOUNT_POINT.index(boot_archive)
try:
# remove line containing micro root
del(vfstabObj.fields.MOUNT_POINT[idx])
except IOError, e:
sys.stderr.write(str(e) + "\n")
# microroot was not found in /etc/vfstab
except (ValueError, IndexError):
sys.stderr.write (_("Microroot for service %s " +
"not in vfstab.\n") % service.serviceName)
return
> try:
> idx = vfstabObj.fields.MOUNT_POINT.index(boot_archive)
>
> try:
> # remove line containing micro root
> del(vfstabObj.fields.MOUNT_POINT[idx])
> except IOError, e:
> sys.stderr.write(str(e) + "\n")
>
> except NO_INDEX_OR_WHATEVER_THE_EX_IS, e:
> sys.stderr.write (_("Microroot for service %s " +
> "not in vfstab.\n") % service.serviceName)
>
>
> (Ideally, I would have like to have seen the caller not
> need to handle the idx at all, but I won't push for that
> at this point.)
Dude, I can't make it magically delete things, one has to tell it what you
want to delete. How else would one specify what to delete? This is a list
after all, not an associative array (aka dictionary) as otherwise the
DBFile would have to have knowledge of what columns contain unique data
and which can have repeated data.
> 424 - It would be more useful if the error message could also
> include the value of what wasn't found in the vfstab.
> i.e. boot_archive
Done
> 453 - This doesn't look correct. The manpage for in.tftpd
> does not say that 'homedir' is the argument to the -s
> option. In fact, based on the synopsis, it looks like
> 'homedir' can float.
>
> SYNOPSIS
> in.tftpd [-d] [-T rexmtval] [-s] [homedir]
>
>
> So it seems you have to parse fields.SERVER_ARGUMENTS,
> looking for the argument that's not an option, and not
> an option argument.
Thank you for catching this! I've fixed it to take the last argument --
much easier to parse out too.
> 468 - misspelled "Directory"
Sorry the differential webrev was against the pre-aspell'd file
> 474-478 - I really think we should leave it up to the caller
> to check this, so that the interface of findTFTProot()
> is defined as always to return something, with the default
> being /tftpboot
>
> (Looking ahead at your code, the callers already do this
> checking)
Actually this is broken both ways. The callers try to build a path off an
unset baseDir variable. I added a meaningful error for the user that we
can't do much deleting without a TFTProot (this egregious enough I'm
tempted to do a SystemExit, but right now I've added the following checks
after line 501:
else:
sys.stderr.write (_("Unable to remove the grub executable, boot "+
"archive, or menu.lst file\nwithout a valid "+
"tftp root directory.\n"))
> 500-501, 688-689 - Umm, can you please make findTFTProot()
> cache its answer then. The chances of someone changing
> /etc/inetd.conf in between time is negligible afaic.
>
> And actually, if you do my previously suggested comment,
> you can remove the if()'s at 500 and 688.
As the inetd class will be caching the data this is trivial in code
runtime. And not easily coded (as findTFTProot is a function not a class)
further 500-501, 688-689 are in different scopes (well one's a nested
function of the other but that would be very ugly to understand where
those vars are coming from to get it to persist). If you think this will
save more than 500ms off the runtime of this program I'll profile it to
see but this is trivial and very over-optimizing to concern ourselves
with.
>
> usr/src/cmd/installadm/delete_client.py
> ---------------------------------------
> 41 - this comment isn't needed
> anymore.
I've changed from:
"Parse and validate options when called as delete-client"
to:
"Parse and validate options"
and for further concern I've changed from parse_client_options to
parse_options.
Please I'd like to stop working on nits and ensure the program works this
has been out for code review for greater than nearly a month.
> 43 - is this comment still accurate?
It is, but perhaps changing service to client would cause less confusion
as it returns a client_Data class
> usr/src/pkgdefs/SUNWinstalladm-tools/prototype_com
> --------------------------------------------------
> 45-48 - Is there a benefit for doing this symlinking? Why don't
> we just deliver two binaries, delete-service and
> delete-client?
The symlinks are just to conform to how the other binaries appear in
/usr/lib/installadm. For python to be able to import it needs module names
to conform to [a-zA-Z0-9_]*.py and thus we need to at least deliver
delete_service.py for delete client to import. It seemed prudent to start
moving things that way thus the reason for doing delete_client.py.
> thanks,
> -ethan
>
>
> Clay Baenziger wrote:
>> Hi again all,
>> I got a way to get webrev to work for me to do a differential including
>> delete_service.py (still known as delete-service.py in the differentail
>> webrev). See:
>> http://cr.opensolaris.org/~clayb/delete_service/webrev2.diff2/
>>
>> Thank you,
>> Clay
>>
>> On Wed, 23 Sep 2009, Clay Baenziger wrote:
>>
>>> Hi again all,
>>> Well the addition of a thousand lines of code and 50+ pages of
>>> comments I think I've got this re-spun for everyone's enjoyment, please
>>> see:
>>>
>>> http://cr.opensolaris.org/~clayb/delete_service/webrev2.diff/
>>> (Unfortunately I can't get webrev to track that delete-service.py became
>>> delete_service.py (so that it can be imported by delete_client))
>>>
>>> Or full webrev:
>>> http://cr.opensolaris.org/~clayb/delete_service/webrev2/
>>>
>>> The bug list has grown to include:
>>> 4526 delete-service is not deleting service as described in section
>>> 4.3.2
>>> ai_design_doc
>>> 6587 delete-service shouldn't remove the source image if there's other
>>> services actives 'linked' to the same source image
>>> 8666 create-service: prints out SMF messages no matter what's going on
>>> 8773 create-service followed quickly by delete-service hangs
>>> 10740 Need way to interact with SMF from Python for installadm
>>> components
>>> in Python
>>> 11292 delete-client: should remove SPARC clients too
>>> 11486 delete-service/delete-client: should check inetd.conf for tftp
>>> root
>>>
>>>
>>> To Drew:
>>> --------
>>> To address the ps(1) pain, I consolidated the function down and filed
>>> 11524 - Should look to using PSI (Python System
>>> Information) for Python process management
>>>
>>> I looked into Bill's bootadm work but I don't fit an "alternate root"
>>> environment and I'd still need to provide a lot of parsing anyways.
>>>
>>> To Sundar:
>>> ----------
>>> I think our phone call Thursday cleared up your questions?
>>>
>>> For those in the code walk-through:
>>> -----------------------------------
>>> I chose to append our findings of being able to have both a SPARC and X86
>>> client to a bug on create-client rather than address finding all possible
>>> nooks for a client and spewing lots of not found messages to a user (or
>>> having to catch the messages in funky ways).
>>>
>>> Jack:
>>> -----
>>> Per the agreement between Drew's coding style suggestions, those of PEP8's
>>> hanging indents and Google's Python style guide I've followed
>>> PEP8/Google's Style guide, however, I hope next Tuesday we'll have time to
>>> come to a consensus on Python style there as this should expand past this
>>> one push, of course. Thank you for getting me to think about this so much!
>>>
>>> Thank you,
>>> Clay
>>>
>
>
>
