Clay Baenziger wrote:
> 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
>> -----------------------------------
>> 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).
okay.
>
>> 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.
Okay, can you please go ahead and make this change then. My
concern was that if our goal for this code is for it to approach
something portable, then naming something that wasn't necessarily
Unix specific with that name could cause confusion down the line.
>> 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?
Sorry, that's what I meant to ask about, the earlier return at 105ish.
Let me restate the comment then...
At 107, it returns the empty string to denote to the caller that we're
out of lines, but if we're returning the empty string at 105 for the
case above, then won't the caller get confused?
i.e. if we're processing a file with an empty line in the middle of
the file somewhere, and its being processed with newlines = False and
blanklines = True, won't this wad of code (readlines() + readline() )
stop processing in the middle of the file?
>>
>> 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.
>
That's part of the point. Why should the caller need to know if the
vfstabObj is a list or an array? Secondly, your current implementation
has the caller extracting something from the object, just to pass it
back into the object. That something is the idx in this case, and
there doesn't seem to be any value or meaning for the caller to
ever have to handle that. So my hinted suggestion was to build a
member function for the object to hide that detail. For example,
the caller code could end up looking like this:
try:
vfstabObj.delentry_by_mntpnt(boot_archive)
# or if we want to parameterize the field out ...
# (this is probably not correct python syntax, but I know
# its possible somehow.)
# vfstabObj.delentry(com.VFSTAB.fields.MOUNT_POINT, boot_archive)
except (ValueError, IndexError):
except IOError, e:
except Whatever Else:
Is this over java-fying the code?
Lastly, as I noted in the original comment, this is not something
I am not pushing for this in this putback, but since you're asking
I'm just clarifying what my comment was.
Based on Jan's comment and your filing of 11537, the vfstab
looks like it may be handled differently altogether in our AI
server tools, correct?
>
>> 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.
Did you check whether or not the [homedir] argument can float?
Based on the synopsis, [homedir] looks like it can be put anywhere
in the command line, not necessarily always at the end. Can you
verify whether or not this is the case?
>
>> 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"))
Okay, that's fine.
Comment on new delete_service.py file:
471-473 - Can you update this error message to be more clear?
I couldn't tell from the message that something doesn't exist.
Here's a suggested rewrite of the comment
"The tftp root directory (%s) found from the configuration
file (%s) does not exist. Using default: %s\n"
482-483 - Same comment.
"The tftp root directory (%s) does not exist.\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.
Since you're not taking my original comment for 474-478 above,
nevermind this comment.
>
>>
>> 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.
Okay thanks.
>
> 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.
You've got a large amount of delta here for the differential though.
You can't expect to make this much change again and not get an
initial-like review for the parts that are freshly written.
>
>> 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
That would be a good start.
thanks,
-ethan
>
>> 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
>>>>
>>
>>
>>
