Hi all,
I believe the final webrev is now available. I'm running the final
regression tests (so please excuse the two non-collapsed in deltas in the
webrev) however, they are available at (diff and full):
http://cr.opensolaris.org/~clayb/delete_service/webrev3.diff
http://cr.opensolaris.org/~clayb/delete_service/webrev3
Please let me know if I failed to address anything.
Thank you,
Clay
On Mon, 28 Sep 2009, Ethan Quach wrote:
>
>
> 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
>>>>>
>>>
>>>
>>>
>
>
On Mon, 28 Sep 2009, Jack Schwartz wrote:
> Hi Clay.
>
> I can commit to re-reviewing the parts which I commented on earlier and maybe
> a little more. This is almost like a whole new review and my time is limited.
> I spent a few hours today, and will spend a few more probably tomorrow.
>
> Is anyone reviewing the new installadm_common.py? I thought I saw that Drew
> can't do it. If you want me to, please explain the syntax for 74 - 93 of
> installadm_common.py.
>
> Thanks,
> Jack
>
> On 09/28/09 09:41, Clay Baenziger wrote:
>> Hi Jack,
>> I was wondering if you had an ETA when you'd be able to get me your
>> delete-service code review comments back. Ethan and I would really like to
>> get this pushed as things just keep backing up behind this.
>> Thank you,
>> Clay
>
>
On Mon, 28 Sep 2009, Evan Layton wrote:
> Hi Clay,
>
> Just a few more comments on the "C" code only...
>
> installadm.c
>
> line 899 - This needs to be changed as we discussed earlier today so
> that we give a bit more description about what this function does. Also we
> need to add a bit more description on what type of thing
> SERVICE_DELETE_SCRIPT does. for example maybe saying something like "the
> remaining clean-up is done by SERVICE_DELETE_SCRIPT" ( if that's at all
> accurate... ;-) ) would help.
>
> line 1168 The comment here is a bit misleading. What this really does here is
> if there are no service instances with a state set to "on" the we put the smf
> service into maintenance mode. We should state that this is what is happening
> instead of saying it disables something.
>
> Also this points out the fact that the state can either be on or off but
> there can be a third state where we've temporarily disabled a service
> instance but still want it to start up the next time the SMF service is
> started. This transient state is not dealt with in here but is definitely
> outside the scope of this fix and I believe this is now covered in bug 11604
> that you filed earlier.
>
> ai_utils.c
> line 166 - It might be helpful to state here why no data out of the handle is
> needed here instead of just stating that we don't need anything out of the
> handle here. Maybe something along the lines of saying that the handle is not
> needed for this call since we not destroying the property stored in the
> handle.
>
>
> libaiscf_backend.c
> line 82 - This isn't really "bound" to our handle but is stored in the
> handle. It may make more sense to say "Allocate and initialize a property
> group and store it in the handle".
>
> line 87 - Same thing but with the instance.
>
> line 90 - maybe "Make sure we have what's needed to run SMF" ?
>
> libaiscf_instance.c
> line 264, 316 - Closure should be closure so that it matches the actual
> parameter.
>
> libaiscf_service.c
> line 259 - should be "static PyObject *AIservice_print(PyObject *self) {"
> Also this doesn't actually print anything out does it? Seems
> like a confusing name for this function. Not a big deal I
> guess but it could cause confusion. (sorry I didn't notice
> this before...)
>
>
> -evan
>
> 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
>>>
>
>
On Mon, 28 Sep 2009, Sundar Yamunachari wrote:
> 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
>> _______________________________________________
>> caiman-discuss mailing list
>> caiman-discuss at opensolaris.org
>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
> Clay,
>
> *delete_service.py:*
> 542, 544: microroot --> boot archive
> 749: Why we need the python2.4 here? I don't like the hard-coded value here.
>
> *installadm_common.py:*
> 53, 65, 109: Suggestion: change the name of the booleans to make it clearer.
> comments ->skipComments, newlines->removeNewlines and
> blanklines->skipBlanklines
> 288, 302: What is the difference between __getitem__() and gte()?
> 641: enpty --> empty
> 740-791: function clients(net)
> The output of pntadm -P <net> may have some entries missing some values. For
> example these four entries are from ins3525-svr. Check whether your code
> handles all the four entries.
>
> # pntadm -p 10.6.35.0
>
> Client ID Flags Client IP Server IP Lease Expiration Macro Comment
>
> 010003BA866375 03 10.6.35.180 10.6.35.8 Forever 010003BA866375 line2-280r
>
> 0100144F0057A8 03 10.6.35.103 10.6.35.8 Forever 0100144F0057A8 --> No
> comment field
>
> 00 05 10.6.35.247 10.6.35.8 Zero line2-x4100-sp --> No macro assigned
>
> 010007E923F7A6 05 10.6.35.179 10.6.35.8 06/28/2004 --> No macro or
> comment
>
>
> *SUNWinstalladm-tools/prototype_com:*
> 46, 47: Why we need these definitions?
>
> Thanks,
> Sundar
>
>
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
>>>
>>>
>>>
>>>
>>>
>
>
